Use a tagged service iterator for uninstall validators instead of individual lazy proxies

Now we can support tags in YAML parsing it would be good to add support for !service_closure in services.yml as per https://symfony.com/doc/current/service_container/service_closures.html - it is also doable by switching to autowiring but good to have both options and be more like Symfony.

Looking at the code actually I think this should just be use #[AutowireIterator] as per https://symfony.com/doc/current/service_container/service_subscribers_lo...

This gives you a collection of services that are lazily instantiated and that you can iterate over, which is exactly what we need in ::validateUninstall().

Still needs work to clean up the now-unused proxy classes but BookUninstallTest passes locally so let's see what happens.

Also wondering why the ModuleInstaller itself is lazy, it's not injected into anything?

OptimizedPhpArrayDumper does not dump iterators correctly, it just converts them to arrays, and the services within are not lazy.

OptimizedPhpArrayDumper can now dump and restore lazy service iterators and the unused proxy classes have been deleted.

📌 Support IteratorArgument in \Drupal\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumper::dumpValue RTBC added IteratorArgument to OptimizedPhpArrayDumper but it turns out the implementation was incomplete as the iterators were no longer lazy and just cast to arrays in the restored container, however this is currently otherwise unused in core so I think it's OK to fix here.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased, also added a test that was added over in 📌 Add support for tagged_iterator to YamlFileLoader Active

May need a rebase, but appears to have a unit test failure.

longwave → credited kim.pepper → .

Fixed unit test, thanks @kim.pepper for the fix over in 📌 Add support for tagged_iterator to YamlFileLoader Active

Rebased following 📌 Add support for tagged_iterator to YamlFileLoader Active

Added BC service for the $uninstallValidators argument to ModuleInstaller, probably should add a test for that, and also add back the addUninstallValidator() method and issue a deprecation as well just in case.

mglaman/phpstan-drupal doesn't like YAML tags, opened https://github.com/mglaman/phpstan-drupal/pull/743

#22 The fix was just tagged by @mglaman in a new release https://github.com/mglaman/phpstan-drupal/releases/tag/1.2.10

📌 Bump phpstan/phpstan and mglaman/phpstan-drupal to latest Needs review probably needs to land to make this MR work.

PHPStan update landed, updated the baseline to include the new deprecation for the BC layer.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased following book and forum removals.

MR appears to need manual rebase again

Rebased on 11.x

Hiding some files.

Idea is nice, code looks fine, tests are green.

The only thing that I wonder about if there's still time to deprecate/remove in 10.3.0/11.0.0 since the BETAs of both are already out.
I _think_ we're too late and need to deprecate/remove in 10.3.1/12.0.0, but am not sure
I know I've seen documentation on this before somewhere, but my search-fu is failing me now.

Putting this on RTBC so core committers can decide on this.

This should be deprecated in 11.1 for 12.0.0 at this point. We're probably going to have to deprecate some things in 10.4 for removal in 12.0.0 too at some point, but there's no new API for contrib to implement here so it should be fine in 11.1 as cleanup without affecting contrib compatibility with both major branches. Just needs versions changed in the deprecation messages so went ahead and made/applied suggestions to the MR.

Thanks @catch.

Since there's not yet a version-tag for 11.1.x in the dropdown (nor Git), I think the correct status for this issue would be postponed and we whack a [11.1.x] in the title to have some kind of visual queue?

@Spokje because there's the 11.0.x branch open, we can commit 11.1.x-specific stuff to 11.x now. Then we'll eventually branch 11.1.x off 11.x when we want to start committing 11.2.x-specific stuff.

@catch: Ah, of course, this eludes me every once in a while. Thanks for explaining.

Moving to correct version.

catch → closed merge request !7123

Committed/pushed to 11.x, thanks!

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

@deprecated in drupal:11.1.0 and is removed from drupal:11.1.0. Inject

Should this not be removed in 12.0.0?

Fixed the deprecation notice - thanks @mikelutz

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