Add client_id field to the consumer entity

Exciting to see this! farmOS currently adds a client_id base field to Consumer entities itself, so I opened an issue in our queue to coordinate switching over to using this when it gets merged: #3311264: Coordinate upgrade of Consumers module to get client_id base field

We will start by pinning our Consumers module to 1.13 and releasing a new version of farmOS, so we can accommodate this change when this gets merged and 1.14 is released.

Thanks again for pushing this forward!!

I think you're good to go on this @e0ipso! (Pending any further changes from @bojan_dev regarding is_ascii and/or other constraints...)

We have a strategy to migrate to this new client_id coming together on the farmOS side, and we are going to pin to Consumers 1.13 in the meantime: https://www.drupal.org/project/farm/issues/3311264#comment-14709906

Merging!

Back to RTBC. It turns out the merge button is not working for some reason.

Setting back to NW. I suspect this needs a re-roll.

It turns out the merge button is not working for some reason.

Maybe it's just the Drupal/GitLab UI merge button that's not working? But a regular Git merge would work?

Alright, merging in GitLab's UI did the trick.

Thanks all!

Oops sounds like this broke simple_oauth :-/

#3316519: Token request broken consumer load by client id changed

We seem to be unaffected in farmOS because we were already patching simple_oauth via #3167287: Always load clients through the ClientRepository service AND overriding the service ourselves to use our own client_id field.

(@bojan_dev is already aware of this, just commenting to add the cross-reference and context.)

I confirm this broke simple_oauth but seems to be fixed in simple_oauth v6.0.0-beta1.
Only problem is there are some incompatibilities when attempting to update from simple_oauth 5.2.0.

When applying

drush entup

and

drush updb

, simple_oauth update 8604 tries to find table consumers__role but was already renamed to old_xxxxxconsumers__roles previously (I do not know from where).

Fixed by renaming the table back to consumer__roles directly via SQL.

ALTER TABLE old_xxxxxxxconsumer__roles RENAME TO consumer__roles;

Then, the same simple_oauth update tries to remove the field definition which did not exist so a Null exception is thrown.
Added a null-check on line 293.

if (!is_null($roles_field_definition))

After that, drush updb works fine and there doesn't seem to be any more problems.

@KenowaX - That sounds like an issue with simple_oauth v6, not Consumers. Can you open an issue over there with these details? Thanks!

See also this related issue for the 5.2.x branch of simple_oauth, which has been fixed. Just waiting for a 5.2.1 release of simple_oauth - but hopefully that will solve your issue too, before you need to upgrade to simple_oauth v6.

#3167287: Always load clients through the ClientRepository service

Still worth opening an issue for the v6 upgrade if there are upgrade path issues there (which may be separate from the other issue).

Done.
Adding link here in case anyone needs it as it is heavily related to consumers update.

https://www.drupal.org/project/simple_oauth/issues/3318917#comment-14771810

This caused problems for us as well, for a different reason. We have a _lot_ of oauth tokens and trying to resave the consumer entity attempted to invalidate all of them.

There is a way to set an initial value for a field definition, but I think that won't work for the uuid as it's in a different table. Also, this should either do direct SQL queries then or be a post update function. You should never save content entities in a regular update function as you have no idea about the state of your system, could be in the middle of a major update where other modules have not been updated yet.

Oof thanks for the heads up @Berdir. Glad we waited to update farmOS. We may try to work around this in our pending MR: #3311264: Coordinate upgrade of Consumers module to get client_id base field

I wonder if there is any way to "fix" this for other Consumers users though. I supposed the update hook could be fixed, which would prevent it from happening on sites that haven't updated yet. Too late for sites that have, I imagine.

Fixing the update hook and tagging a new release would avoid the need for us to add another workaround in farmOS though... so that's appealing for us as well. Worth doing?

The MR seems quite sensible.

Yes, nothing you can do about sites that already updated, if it worked for them then fine, if not then they can use a patch/wait for a new release.

Thanks for the quick fix @bojan_dev! I haven't tested but it LGTM! Simple. :-)

@paul121 and I just tested this and the tokens were not deleted during update.php. RTBC!

This looks good to me. My interpretation of the issue here is that we don't want to run a save operation on a consumer in the update hook. But we do need to populate the new client_id field with the value from the current UUID field. The new code does that via direct SQL queries rather than via the Entity API. I'm going to merge this in, and then I'll also tag a new release with this updated code.

Thanks for helping get this working.