nextcloud / password_policy

:lock: Let the admin define certain rules for passwords, e.g. a minimum length
GNU Affero General Public License v3.0
35 stars 19 forks source link

fix: Move from private `OC\HintException` to public `OCP\HintException` #514

Closed susnux closed 3 weeks ago

susnux commented 10 months ago

We should not use private API in apps, especially if there is a public alternative.

susnux commented 10 months ago

This breaks all the consumers of the API, because OCP does not extend the former exception, so any catch does not work anymore?🤔

No basically we can use OCP because users of the API that still throw the OC exception will still work -> OC extends OCP meaning as we catch OCP we also catch OC. Thus we still catch all exceptions.

Quick search did not show any API users, but you are right if that API is expected to be used by external apps, then this breaks. Still I think, especially if this is used externally, we should not enforce API users to use private Nextcloud API. So lets only change the documentation so that users catch OCP\HintException as OC extends from OCP, meaning catching OCP works even when throwing OC. (The OC exception is deprecated since NC23).

nickvergessen commented 10 months ago

Still I think, especially if this is used externally, we should not enforce API users to use private Nextcloud API. So lets only change the documentation so that users catch OCP\HintException as OC extends from OCP, meaning catching OCP works even when throwing OC. (The OC exception is deprecated since NC23).

This part is exactly the right one.

You don't find any consumers of the API because you look for the wrong one. Apps emit OCP\Security\Events\ValidatePasswordPolicyEvent which is listened to by this app in https://github.com/nextcloud/password_policy/blob/master/lib/Listener/ValidatePasswordPolicyEventListener.php

https://github.com/search?q=org%3Anextcloud%20ValidatePasswordPolicyEvent&type=code

Used in our org in 4 repos at least:

So the correct way would be to document it in the dev docs that OCP should be catched now and then we can switch to it in some releases

github-actions[bot] commented 9 months ago

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!

susnux commented 6 months ago

As @nickvergessen mentioned we can not replace this directly but need to deprecate first.

susnux commented 4 weeks ago

I checked all using apps in our organization, all migrated to OCP\HintException (or do not catch the exception at all). (Maybe because that exception was not documented?).

So I think we should be good for the Nextcloud 31 cycle.