Skip to content

Conversation

@pulsarf0x
Copy link
Contributor

@pulsarf0x pulsarf0x commented Jul 8, 2025

Description

This pull request updates the constructor of FilteredNavigationManager to accept an INavigationManager interface as its second argument, instead of the concrete NavigationManager class.

This change improves compatibility with proxy or decorated navigation manager implementations (such as custom proxies in other apps), and increases overall flexibility and interoperability.

Context

  • Previously, the constructor required a concrete NavigationManager, which caused issues when passing an implementation of INavigationManager (e.g., a proxy or decorator).
  • This PR allows any object implementing INavigationManager to be used.

Signed-off-by:
[Your Name] <[email protected]>

@skjnldsv skjnldsv enabled auto-merge July 11, 2025 06:20
auto-merge was automatically disabled July 11, 2025 07:22

Head branch was pushed to by a user without write access

@pulsarf0x pulsarf0x force-pushed the fix/allow-navigation-manager-interface branch from 9b036be to 62cf97b Compare July 11, 2025 07:22
@julien-nc
Copy link
Member

@skjnldsv How can we solve the Psalm issues?

  • The clear method is in OC\NavigationManager but not in OCP\INavigationManager
  • The getDefaultEntryIds's signature differs in OC\NavigationManager and OCP\INavigationManager
  • Psalm complains about OCA\Guests\Storage\DirMask::isCreatable but it seems fine to me, this class extends OC\Files\Storage\Wrapper\PermissionsMask which has the same method. Removing the string type of the $path param in lib/Storage/DirMask.php line 59 fixes the Psalm issue and possibly the PhpUnit tests

@skjnldsv
Copy link
Member

Not a psalm expert unfortunately

@nickvergessen
Copy link
Member

How can we solve the Psalm issues?

Add to ignore or type the member to the instance again.

This change improves compatibility with proxy or decorated navigation manager implementations (such as custom proxies in other apps), and increases overall flexibility and interoperability.

@pulsarf0x This is not meant to be implemented. The class here is already an exception. Can you point out other apps that try to do things around the navigation manager replacing the service in the server?

@julien-nc julien-nc force-pushed the fix/allow-navigation-manager-interface branch from 62cf97b to 0e9d812 Compare July 16, 2025 16:03
@julien-nc
Copy link
Member

  • Rebased on main
  • Update psalm baseline (using nextcloud/ocp dev-master)

The remaining failures are real. The methods that are not defined in the public interface are not defined in the OC\NavigationManager implementation either.

@pulsarf0x pulsarf0x force-pushed the fix/allow-navigation-manager-interface branch from 3930221 to 3330164 Compare July 21, 2025 07:42
@pulsarf0x
Copy link
Contributor Author

pulsarf0x commented Jul 21, 2025

Hello team,

I’ve adjusted the Psalm configuration for compatibility with psalm checks for stable30.
Specifically, I re-applied the @psalm-suppress MethodSignatureMismatch annotation on every method that shows up an psalm error on stable30 check like it was already done onthe setDefaultEntryIds() method in a previous commit by another contributor.

Thanks for you help guys.

@pulsarf0x pulsarf0x force-pushed the fix/allow-navigation-manager-interface branch from 3330164 to ddc33cf Compare July 22, 2025 09:14
@github-actions
Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@pulsarf0x
Copy link
Contributor Author

Hello,

It looks like the checks that are currently failing are not directly related to my changes, but seem to originate from elsewhere in the codebase.

Please let me know if there’s anything on my side that still needs to be addressed!

Thanks ! :)

@skjnldsv skjnldsv force-pushed the fix/allow-navigation-manager-interface branch from ddc33cf to 822e72d Compare August 13, 2025 13:44
@icewind1991 icewind1991 force-pushed the fix/allow-navigation-manager-interface branch from 822e72d to 8aa7eaa Compare August 14, 2025 15:55
@icewind1991 icewind1991 self-requested a review as a code owner August 14, 2025 15:55
pulsarf0x and others added 2 commits August 14, 2025 17:57
…edNavigationManager + cs fix

- The constructor of FilteredNavigationManager now type-hints INavigationManager as the second argument, instead of the concrete NavigationManager class.
- This increases compatibility with proxy and decorated navigation managers, improving app interoperability.

Signed-off-by: Pascal Prugna <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
@icewind1991 icewind1991 force-pushed the fix/allow-navigation-manager-interface branch 2 times, most recently from 29459db to cf6a60e Compare August 14, 2025 16:04
@icewind1991 icewind1991 force-pushed the fix/allow-navigation-manager-interface branch from cf6a60e to 4ab4e84 Compare August 14, 2025 16:08
@icewind1991 icewind1991 force-pushed the fix/allow-navigation-manager-interface branch from 4ab4e84 to 68e4b12 Compare August 14, 2025 16:12
@icewind1991 icewind1991 enabled auto-merge August 14, 2025 16:14
@icewind1991 icewind1991 merged commit fdb5a69 into nextcloud:main Aug 14, 2025
48 checks passed
@skjnldsv skjnldsv mentioned this pull request Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants