-
-
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] OAuth - Move some API code to the BAO for better reusability #23725
Conversation
(Standard links)
|
FYI @highfalutin this refactors your api code to move logic around for better reusability. Hopefully I've kept your functionality the same during the refactor. |
0ef8024
to
8af7e41
Compare
I did two simple tests. Except for the above comment:
Have not done code review. |
Also FYI when testing make sure your test site supports https and CIVICRM_BASEURL is https. And also Google has made the oauth prompt about unverified apps even scarier than before, but will still let you continue. |
This has a merge conflict: ext/oauth-client/Civi/Api4/Action/OAuthContactToken/Create.php |
…I4 Get Putting permission checks in the BAO ensures that they are always enforced regardless of which layer accesses them (Api3, Api4, etc) and that they will be enforced even if this is not the primary entity of the api call (e.g. using joins).
This ensures permissions are checked regardless of the api version or action being performed. It also consolidates the code used to check access so that the API checkAccess action will stay consistently and always perform the same access checks as the other api actions.
@demeritcowboy I've rebased to fix the MC |
Hi @colemanw, getting a chance to look at this now. When you say this "Moves some code out of the API into the BAO so we get more use out of it", what uses are you thinking of? Search Kit? I support the code being refactored into a normal form as long as everything still works. I'm just curious about what the payoff will be. |
The standard practice is to put logic into the BAO instead of the API because our APIs come and go. v2, v3, now v4 all rely on the BAO to do the actual work, the API is just a wrapper to that functionality, not the functionality itself. |
I did my own I installed the code from this PR and ran the automated tests for a custom extension that depends on I also verified that the |
Merging based on review |
Thanks @highfalutin @colemanw |
Overview
Moves some code out of the API into the BAO so we get more use out of it.
Before
Lots of overridden API actions, which is not standard practice.
After
Instead of overriding API actions, use the standard actions and put the other logic in the correct place in the BAOs so it gets picked up automatically.
Comments
I haven't used this extension or tested any of this code, this is just what I think the structure ought to look like.