-
-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REF] Convert _checkAccess to event listener #28890
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
f527a97
to
d5241fa
Compare
@colemanw I've read it through but not r-run it. I can see the tests pass, so insofar as we had test coverage for this then things look good. I'm not entirely sold on the event listener model for this: it seems to be a bit less clear (order of execution, reading the code is now a case of hunting for listeners) and it introduces a longer codepath to achieve the same thing, though I'm sure it's not enough to make a huge/noticeable difference, it's still more. |
I think the argument is that event listeners are standard across symfony-compatible php apps, whereas random-function-with-magic-name-that-starts-with-an-underscore-for-some-reason is just a weird little thing I made-up. So this takes us more in the standards-direction and also gets rid of the "two different ways of doing the same thing" motif which can be confusing to devs. And I'm not too attached to my weird little underscore-function to kill it off if it makes things more sane. |
@artfulrobot I wasn't sure about test coverage either so I did a couple test runs locally and a draft commit while writing this pr just to make sure that if any of these conversions wasn't working properly at least one test would fail somewhere... and one did. |
ok, over to you to merge then 👍 |
Overview
Fixes a FIXME that @totten left for me to fix in #20533
Also see https://lab.civicrm.org/dev/core/-/issues/4887
Before
// Note: $bao::_checkAccess() is a quasi-listener. TODO: Convert to straight-up listener.
After
Straight-up listener