-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
dev/core#2779 - Specify row fields to fetch in Api4 OAuthSysToken.Refresh #21224
Conversation
(Standard links)
|
Also if this stopped working in 5.40 then the base branch for this PR should be 5.41 (because that's the standard procedure for backporting to recent breakage). Sorry I forgot to mention that in the lab ticket. It might work to just change that in the PR settings here but it usually breaks it. I'll try it. |
Yep that broke it.
|
671ed43
to
afc100a
Compare
afc100a
to
1af0554
Compare
Thanks @demeritcowboy . I think third-time-lucky I may have just managed that cleanly :) |
Yep looks good - thanks! |
Merging based on @demeritcowboy 's review and @totten 's supportive comments in MM https://chat.civicrm.org/civicrm/pl/ie5d7s6is3dmfj5nycd81s6igo |
@@ -38,10 +38,15 @@ class Refresh extends BasicBatchAction { | |||
|
|||
private $syncFields = ['access_token', 'refresh_token', 'expires', 'token_type']; | |||
private $writeFields = ['access_token', 'refresh_token', 'expires', 'token_type', 'raw']; | |||
private $selectFields = ['id', 'client_id', 'access_token', 'refresh_token', 'expires', 'token_type', 'raw']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this change is good and necessary to address the regression.
Just to note that the select-list is now smaller than it was before.
$ cv api4 OAuthSysToken.getFields -T +s name
+---------------------+
| name |
+---------------------+
| id | yes
| client_id | yes
| token_type | yes
| access_token | yes
| expires | yes
| refresh_token | yes
| raw | yes
| tag | no longer
| grant_type | no longer
| scopes | no longer
| resource_owner_name | no longer
| resource_owner | no longer
| error | no longer
| created_date | no longer
| modified_date | no longer
+---------------------+
(Annotations: "yes" - restored as part of the list; "no longer" - would've matched *
but no longer matches the list)
AFAICS, this list does cover the most important fields for a typical refresh. I can't spot/think of any specific problems with the smaller list. Just noting the change in case there are edgey issues later or in case it triggers any thoughts.
Overview
Fix for https://lab.civicrm.org/dev/core/-/issues/2779
Before
Api4 action implementation was broken by this update to underlying BasicBatchAction class interface https://lab.civicrm.org/dev/core/-/commit/29ab318b684a79f4cdedde2bf0417775d4be5523
After
Previous functionality restored
Technical Details
The required fields from the OAuthSysToken table for the batch action are now explicitly specified using getSelect() method. Previously it fetched all using a wildcard "*" but I don't think this is possible any more. If the method needed additional fields they would need to be added to the selectFields array