Merge Simple OAuth Extras in Simple OAuth

When I try to upgrade to 4.0 it throws error due to simple_oauth_extras. I tried to apply the patch before upgrading but #3 fails with 3.16

Will there be an update path?

Not really, because this is a removed feature and even if it was still there it is of no help, because when moving configs around and parts of them a manual update is required.

In order to not break systems updating I think that it would be great if the commit is either reverted or the v3 branch remains the recommended one until the update path has been implemented.

No, I am sorry currently I don't have that much time, as I am working on higher priority issues in the Core entity API.

I had a short look at your patch and I've seen such changes, which is why I've made the suggestion in the previous comment. What about it?

An update path is definitely needed here. The removal of the automatic entity update system in Drupal 8.7.0 squarely necessitates it, as I discovered when Lightning API implemented an update hook to uninstall simple_oauth_extras and the update path test we wrote complained about out-of-date field storage definitions.

We worked on an update hook at
#3079468: Update [Simple OAuth] from ~3.0 to ~4.0
#3085671: Fix the update process to uninstall the Simple Oauth Extras module as it was merged into Simple Oauth to make it work without conditions
It was reported to me by Edgar Montes (jedgar1mx)

https://git.drupalcode.org/project/varbase_api/blob/8.x-7.x/varbase_api....

Varbase API is a fork of Lightning API with custom changes

Yup it works

You still need the proper updates of the field definitions.

Uninstall function will not work as the module will be removed on composer update

Delete the key value for simple_oauth_extras from the system schema collection.

  $query = \Drupal::database()->delete('key_value')
    ->condition('collection', 'system.schema')
    ->condition('name', 'simple_oauth_extras')
    ->execute();

Entity updates to clear up any mismatched entity and/or field definitions
And Fix changes were detected in the entity type and field definitions.

  // Entity updates to clear up any mismatched entity and/or field definitions
  // And Fix changes were detected in the entity type and field definitions.
  \Drupal::classResolver()
    ->getInstanceFromDefinition(VarbaseEntityDefinitionUpdateManager::class)
    ->applyUpdates();

Customized class from the Devel Entity Updates module
Development version of the entity definition update manager.

Support for automatic entity updates has been removed
https://www.drupal.org/node/3034742

  // Entity updates to clear up any mismatched entity and/or field definitions
  // And Fix changes were detected in the entity type and field definitions.
  \Drupal::entityDefinitionUpdateManager()->applyUpdates();

User deprecated function: EntityDefinitionUpdateManagerInterface::applyUpdates() is deprecated in 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityDefinitionUpdateManagerInterface::getChangeList() and execute each entity type and field storage update manually instead.

#3051884: Switch to use [Varbase Entity Definition Update Manager] and remove deprecated \Drupal::entityDefinitionUpdateManager()->applyUpdates()

You should not perform automatic entity updates, one of the reasons the support for this was removed from core.
You should correspondingly update the moved field definitions.

How workaround, I copied the simple_oauth_extras to my modules/custom and I wrote an update_N to disable it.

/**
 * Uninstall Simple OAuth Extras module.
 */
function mymodule_update_8001() {
  // Uninstall the module.
  \Drupal::service('module_installer')->uninstall(['simple_oauth_extras']);
}

I'll leave the code there, and in a future updates I'll remove it.

Upgrade path against 4.x. Except fixing field definitions, incorporated suggestion from #19 to remove schema version for simple_oauth_extras and uninstall it as in #22. But, given the codebase for extras is already removed, should this module be detected as installed and removed by corresponding services? Or it was better just to edit core.extension configuration? Not sure.

I think your patch fixes field definitions @gease.

Because the codebase of Simple Oauth Extras might not exist, you need to capture ModuleUninstallValidatorException on uninstall and delete the key_value afterwards or inside the catch.
\Drupal::service('module_installer')->uninstall(['simple_oauth_extras']);

Ideally core.extension and and config file need to be deleted too in case of non existing codebase.

\Drupal::configFactory()->getEditable('core.extension')->clear("module.simple_oauth_extras")->save(TRUE);
\Drupal::service('config.manager')->uninstall('module', 'simple_oauth_extras');

I would also advise to apply this patch before updating to D8.8 because due to the removal of Simple Oauth Extras, an exception is thrown during system_update_8803 preventing the required path_alias module to be installed and stooping the rest of the updates to run. It basically breaks the whole site.

Substantially rewritten the upgrade.
First, we need to install field definitions only when we need to, in fact, if simple_oauth_extras was on, definitions are already fine.
Then, there can be a configuration item from simple_oauth_extras that needs to be ported to simple_oauth config.
Third, it seems that if simple_oauth_extras was on and the codebase was removed, the module won't be detected as installed and entry will be somehow removed from core.extensions automatically. In case this did'nt happen, I still add actions to the end of update. And anyway, we'll need to remove schema number for simple_oauth_extras (if it was present, but there's no bad in running this query).

It works great for me.

Just needed to make sure this was applied before the system update hooks in D8.8.

Thanks!

Some more tweaks. Anyway, we need to update field provider for installed field definitions, even though entityDefinitionUpdateManager::getChangeList() doesn't seem to report this as change. And there is a difference between field definition and field storage definition (though somehow it worked with field definition as well).

You should not act according to the returned changes, but according to the changes in the module.

So if the module wasn't installed, then install the fields, otherwise update them. When installing a field you have to hard code its definition in the update, otherwise you might be installing charges made on the field by other modules through some hooks.

The update currently is mixing the concepts, but it should not and there should be a clear separation what is done if the module was installed and what if not.

Isn't moduleExists checking whether the module is installed instead whether it is present on the file system?

Also aren't there errors raised when Drupal tries to invoke cached method names of hook implementations in the removed module?

You should not act according to the returned changes, but according to the changes in the module.

There seems to be no way of detecting if module was installed before in update hook, if its codebase is removed. DbUpdateController::handle() seems to take care of keeping the module list correct. And so the cleaning of core.extensions in this patch seems not needed. As well as uninstalling the extras module.

When installing a field you have to hard code its definition in the update.

Implemented this, and changed a bit updating the field following core examples.

There seems to be no way of detecting if module was installed before in update hook, if its codebase is removed.

If there wasn't then you wouldn't have to do this in your update:

+  $query = \Drupal::database()->delete('key_value')
+    ->condition('collection', 'system.schema')
+    ->condition('name', 'simple_oauth_extras')
+    ->execute();

Why not use it for a check whether the module was enabled?

Updated patch based on previous comment.

Could you please remove the unused code?

Also define the definitions only in the condition branch where they are used.

If the extras module wasn't installed then you need to provide the default value for the use implicit setting.

Sorry, previous patch was posted erroneously.
Now also loading config setting from defaults if simple_oauth_extras was not installed.

Actually now thinking again about varbase there might be installations that have already installed the fields by themselves. Unfortunately we should account for that.

1. +++ b/simple_oauth.install
   @@ -46,3 +46,66 @@ function simple_oauth_update_8403() {
   +  $query = \Drupal::database()->delete('key_value')
   

   The result is not a query and therefore the variable should be named properly.

2. +++ b/simple_oauth.install
   @@ -46,3 +46,66 @@ function simple_oauth_update_8403() {
   +    $field_definition = $entity_definition_update_manager->getFieldStorageDefinition($field_name, 'consumer');
   +    $field_definition->setProvider('simple_oauth');
   +    $entity_definition_update_manager->updateFieldStorageDefinition($field_definition);
   

   Adjust the indentation level.

3. +++ b/simple_oauth.install
   @@ -46,3 +46,66 @@ function simple_oauth_update_8403() {
   +    // If simple_oauth_extras wasn't installed, definitions should be missing.
   

   "Unless they have been added already by a custom update." We then need to account for this use case here.

Trying to move this forward to address @hchonov comments.

For #36 3. if a different module took care of it I'd just do nothing, but not sure if that's the best approach.

Also, accounting for the case where simple_oauth_extras is installed but not the field storage definitions.

I don't have a complicated setup, so I haven't done extensive testing. When I updated from 3.15 to 4.4, I hit the missing module warning. The patch applies cleanly, gets rid of the warning, and nothing broke.

D8.8.4
Lightning 4.1.3
BLT 10.7.2

simple_oauth was, apparently supposed to update to 8.x-4.4 (see: https://www.drupal.org/project/lightning/releases/8.x-4.103). However, it never did with the last lightning update from 4.1.2 to 4.1.3. I didn't get any error alluding to this and, it's already on our production env with the older oauth version still.

NP, right? I just run composer require acquia/lightning:~4.1.3 and all is well?

No, I get this output and have been lost in wonderland for a 2 days now trying to upgrade the module:

Your requirements could not be resolved to an installable set of packages.
  Problem 1
    - Installation request for drupal/simple_oauth  ^3.3 -> satisfiable by drupal/simple_oauth[3.x-dev, 3.16.0, 3.15.0, 3.14.0, 3.13.0, 3.12.0, 3.11.0, 3.10.0, 3.9.0, 3.8.0, 3.7.0, 3.6.0, 3.5.0, 3.4.0, 3.3.0].
    - drupal/lightning_api 4.x-dev requires drupal/simple_oauth ^4.0 -> satisfiable by drupal/simple_oauth[4.x-dev].
    - drupal/lightning_api 4.5.0 requires drupal/simple_oauth ^4.0 -> satisfiable by drupal/simple_oauth[4.x-dev].
    - Conclusion: don't install drupal/simple_oauth 4.x-dev
    - Installation request for drupal/lightning_api ^4.5 -> satisfiable by drupal/lightning_api[4.x-dev, 4.5.0].
Installation failed, reverting ./composer.json to its original content.

I've received some help, but nothing gives way to a solution. I'm really lost. I'm probably considered OK on composer but, by all means, I look almost everything up still outside of basic commands.

I've tried the patch here. I applied it manually since, adding it to the patches in composer.json fails.

How can I get this module trued up with its latest release?

My solution was:

- removing the line from my composer.json require section: "drupal/simple_oauth":"^3.3"
running:

$ composer remove --no-update drupal/simple_oauth
$ composer require --no-update drupal/lightning_api:^4.5
$ composer update

I use BLT and after the successful updates above, my drush failed because it continued to think there is an extras module not present.

fix:
Created a /modules/contrib/simple_oauth/simple_oauth_extras/simple_oauth_extras.info.yml file placeholder (local env only) with the following:

name: Simple OAuth Extras
type: module
core: 8.x
core_version_requirement: '^8 || ^9'
package: Authentication

I did not commit the .info.yml file up.

Hope this helps!

All credit goes to phenaproxima. Thanks so much!!

While this is looking better, we really need to include a deprecicated shell module to keep errors from occuring when trying to update.
See #2429191: Deprecate entity_reference.module and move its functionality to core on how Core did it.

Specifically, a hidden info.yml was added. So my proposal wraps #38 and an info.yml so upgrading doesn't error out.

I didn't have any issue when running the updates and Authorization Code Grant seems to be working properly. I've attached screenshots.

I successfully updated the module from 3 to 4 using @japerry's patch from #43 and it seems to work fine - including config export and import! Now it's been in production a couple of days so thank very you much.

The only minor question I have is about the contrib/simple_oauth/simple_oauth_extras/simple_oauth_extras.info.yml file, which is just going to stay there I guess. I mean, it would be nice to get rid of the patch at some point or if this is merged to the module, can the next update hook remove it? From my understanding, it does no harm there, but it would be nice to get rid of it. :)

I don't think you should remove the file with an update hook as it can be restored in a following deployment.

I understand it will be removed on the next major version of the module.

Yup, similar to how core handled the entity_reference module, this module will need to retain that blank module for probably quite a while. Even though Drupal 8 deprecated and moved all the functionality of the entity_reference module out, it looks like the stub will remain for Drupal 9.

Why? because folks upgrading from D7 to D9 would break. This is the same problem if someone tried to update from simple_oauth 3.x to 5.x someday. Most likely, you'll just have to give it some time (years) and then eventually remove the code. Luckily it doesn't do much harm staying around.

Based on the comments above, marking this RTBC.

Yes, "does not do much harm staying around." is the answer I was waiting for. The legacy file just might one seem weird to someone who sees the code first time or until it does some harm. But I guess our brilliant module maintainers us save from that. :)

But upgrade path is there! Thank you guys!