Plans for Drupal 10 support?

here is my first attempt at a patch to add D10 compatibility... it is working for me for the features of the module I use.

Also I didn't see this issue 🐛 Uppercase columns errors on information_schema tables queries Fixed so I didn't use that patch but instead came up with a slightly different solution to the same problem... relly either will work.

Remove the composer.json change. That information gets overridden by the drupal.org packagist out of the contents of core_version_requirement in the .info.yml file. Having it in two places runs a high risk of those two lines being out of sync and confusing people. This patch was already introducing that discrepancy, and it's only one person involved.

Also, the constructor should not directly insert the service, as this should be done by the create() function that is added by the ContainerInjectionInterface that should be added to the System class.

I'm wondering also about the config.storage.staging -> config.storage.sync change... If this is a bug, it should really be in it's own issue. If it is not a bug, and a true change between D9 and D10, that would introduce a BC-breaking change that might affect the versions.

Likewise for the lowercase keys which I'd rather have decoupled from this issue and handled in 🐛 Uppercase columns errors on information_schema tables queries Fixed .

@jcnventura,
The reason for adding the "drupal/core" requirement in composer.json is because some Drupal users don't know when a module maintainer will get around to committing the changes for Drupal core compatibility and it is therefore easier to add the Git repository to the site's composer.json repositories list if there is a composer.json with version compatibilities in the Git repository. Nevertheless I have removed that addition per your request.

As for the constructor for Drupal\schema\Plugin\Schema\System directly inserting the ModuleHandler, I agree with you that it should be injected with a create method as part of the ContainerFactoryPluginInterface. unfortunately with the limited time I had to work on it I was unable to get my create method to be called... I don't yet know why. This is, however, better then the previous code which called \Drupal::moduleHandler() every time it wanted the to utilize the service.

As for the config.storage.staging -> config.storage.sync change... when I saw the error that the container could not find the "config.storage.staging" service I looked up the API documentation and saw that it was listed as "Alias of config.storage.sync".
Issues related to it:

• #2487588: Move CMI import/export directory "staging" to "sync", as it is confused with staging environments →
• #3115159: Properly deprecate config.storage.staging service →

Change Record

I have also remove the changes that would conflict with 🐛 Uppercase columns errors on information_schema tables queries Fixed and marked that issue a RTBC.

ok, I got the dependency injection working for Drupal\schema\Plugin\Schema\System

At this point, the module passes upgrade_status checks. The only 4 remaining deprecations are internal to the module itself, unrelated to Drupal core.

There are obviously other issues, such as not using Gitlab CI, but at this point with D11 soon to be released, I think the best decision is to merge this so we can tackle the Gitlab and D11 migrations on D10, which is still relevant while D9 has been EOL for 6 months.

There's a lot of unrelated fixes in that MR which make it harder to understand what's going on.

Things like code style fixes & DI should be done in separate issues, either before or after. (And this is one reason why git commits on feature branches should be small and atomic, because then doing this would be simply a matter of cherry-picking to a new branch, which isn't going to work here.)

Yeah, my point is that even though the MR is far from ideal, it works well enough in its current state, and it is probably more useful to just merge it and open a followup for all the needed improvements on a supported core release rather than one that is EOL, because we don't know how much longer it can take to reach a good status rather than "not worse but working on supported core" which seems to be the plan here.

Maybe the real point is that none of the maintainers have been able to review the MR and merge it because of the large number of unrelated changes.

Maybe we should just close this issue and start fresh to add Drupal 11 support?

We could. Or, since D9 is EOL anyway (so no one should be developing on it, hence no one will be affected), we could just accept it as is, and open one or more followups to address your comments in the original, working on D10 ? Since it doesn't work on D10 yet, no one on D10 will be affected by it not working either.

The thing is, that migration is not just about editing a few deprecations and namespace changes: some core functions were just removed without any direct equivalent, hence part of the PR size, AFAICS. Meaning restarting it for just the D10 port is likely to yield an only slightly smaller MR. I'll try to reduce its size later today, if that can help, like putting the moved function back in place and removing the CS fixes.

Done. The MR is now significantly smaller, with less than 150 new lines, and addresses most of your comments.

Can you also remove the changes to the following files:
* src/Migration/SchemaMigrator.php (undo all changes)
* src/SchemaDatabaseSchema_mysql.php (just change the use line and keep the "as DatabaseSchema_mysql", so that the rest of the file stays the same)
* src/SchemaDatabaseSchema_pgsql.php (same)
* src/SchemaManager.php (undo all changes)

I believe with that minimal set of changes preparing the way for Drupal 10 (and already some that are necessary for D11 like the Schema changes to the modules), this should be merged quite soon.

The move of the _schema_schema_initialize() function to the class was a good one, as it removes code in the .module that didn't need to be there. However you can probably now recognize that this method is only used once, so it would make a lot of sense to move that code inside the loadSchemaImplementations() method.

Finally, I fail to understand why core/includes/common.inc needs to be required_once, since there seems to be nothing in that method justifying that.

One last question. What makes this ^9.5 instead of ^9.4??

The mysql and pgsql modules were added in 9.4.0, so I understand that. Is there anything used in ModuleHandler that was only added in 9.5.0?

• If I remove the drush_log removals, this will trigger warnings in upgrade_status because that function is obsolete.
• Undoing the changes from DefaultFactory to ContainerFactory will drop support for the ContainerFactoryPluginInterface, breaking class System changes which depend on it
• No visible reason for the include common.inc, but since we are going for a minimal MR, this was already in, so it seems inappropriate to change it
• The change for 9.5 is because 9.5 is the last version in the 9.x series, required for 10.0 upgrades, hence appropriate to require since this MR is about preparing for D10: anyone still on 9.x should be on 9.5.x, not 9.4, especially since this is a development tool.

I applied the other changes you requested.

OK. Understood about Container Factory, and I agree with you.

However, I'd rather leave the drush_log calls. I understand they are deprecated. The hope is that someone will fix it (needs to use the Logger system, I believe), instead of deleting that altogether. And indeed supporting current versions of Drush should be done, but in another issue.

Why is the return type of createInstances being an array essential for Drupal 10 support?

The other thing with drush_log is that there is already a proper dependency injected $this->logger->info($message, $context); at the next line so it seems entirely redundant, does it not ?

For the array return I had forgotten to remove it. Removed.

jcnventura → closed merge request !1

Automatically closed - issue fixed for 2 weeks with no activity.