@godotislate

Removing an unnecessary conditional gate revealed a couple bugs it was hiding. Push commits to fix. This is ready to be looked at again.

Applied the suggestion and answered the Qs on the MR.

The follow up should be 📌 Add plugin class module dependencies to dependency calculation Active . It's a child issue, but I'll add it as a related issue as well.

Added test looks good (as well as the rest of it).

Changed the parameter name from $throw_exception to $exception_on_invalid, because that's what PluginManagerBase::getDefinition() uses, and makes sense to me to be consistent.

Looks like the change to Drupal\Hook\BlockHooks::modulesInstalled() does not have tests? Was this intentional?

If not, it probably could be set up as a similar kernel test where a the block modules_installed hook is invoked on a profile in both syncing and non syncing cases to make sure blocks are and aren't added to installed themes correctly.

If there's urgency to get this in, maybe that test can be a follow up (or the modulesInstalled change reverted), if technically scope is about theme install only.

I think I addressed or answered the outstanding MR comments.

Has anyone reviewed the change record?

@nicxvan reviewed and asked me for some revisions. If there are still things that need clarifying, I can make additional changes.

Dumb question, when you're on the uninstall confirmation screen won't it show that views are still going to be deleted?

Maybe, I never do things from the UI. But if so, what might be needed a core issue to allow exposed_form plugins on a View to react to a module being uninstalled.

Took a quick look at this, only really looking at the ViewsExposedForm plugin bef (Drupal\better_exposed_filters\Plugin\views\exposed_form\BetterExposedFilters). Handling the plugin types that BEF provides itself will have to be looked at separately.

So, unfortunately, "exposed_form" is not considered a handler type by core views, so when it loops through looking for handler plugins implementing DependentWithRemovalPluginInterface, the exposed form plugins are ignored.

What needs to be done looks like:

• Implement hook_uninstall()
• In the uinstall hook, load all viewconfig entities and filter for those with a dependency on better_exposed_filters
• Loop through all the relevant views and all the displays for each view
• In the display options for the display, if the exposed_form type is "bef", set it to "basic" and reset the exposed_form options to the default for basic
• Save the view

Have an MR up of what this can look like.

The only thing is whether we want to document on the interface that this will get a type in D12.

What's the alternative? Throw an exception on non-arrays in D12?

Documenting the type change seems straightforward: https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... → , and the signature will change anyway because $is_root_call is going away, though maybe that's not a breaking change.

It would be nice to add a return type at that point, too, if possible.

Pushed up MR 12148.

Based this on work done in the previous MR 10900, but I decided to create a new menu tree manipulator to inject the overview links instead. This provides a lot of different ways for contrib or custom modules to override this new behavior, whether by altering/decorating the new manipulator service, implementing hook_navigation_menu_link_tree_alter(), or altering the block render array.

Also, since there were at least a couple comments expressing concern about multiple links titled "Overview", I changed it so that the new child link's title is the same as the parent's, except with " overview" appended. I guess that will be awkward if someone titles a menu link "Something overview", so the child link title becomes "Something overview overview", but I think that is an edge case and the link can be altered as mentioned above.

Added a unit test for the new manipulator service. I think the unit test class has pretty exhaustive coverage, so while I did also update the navigation block kernel test as well, I added fewer cases there because I think the coverage would be mostly duplicative.

This should ready for review again. If the other approach is preferred, we can probably grab the kernel test changes in MR 12148 and add them to 10900.

godotislate → made their first commit to this issue’s fork.

My thought then is that not all \InvalidArgumentExceptions should be suppressed, but only ones with message "Field $name is unknown", but this seems brittle

Thinking about this gave me an idea that it could be convenient to do exception handling using enums instead of codes or messages, so I created ✨ Introduce Exception type that allows Enums be to used to set code Active . Not a blocker for this, but I think it could be useful for similar cases in the future.

godotislate → created an issue.

Had an idea, so opened a second MR (MR 12123) to do the deprecation with the expectation that the default will change from TRUE to FALSE. I moved the deprecation warning so that it only triggers when an exception is thrown and the second parameter is not set, and updated relevant tests to expect the deprecation message.

For clarity:

MR 10406: $throw_exception default is TRUE with the expectation it will continue to default to TRUE

MR 12123: $throw_exception default is TRUE with the expectation it will be changed to default to FALSE in Drupal 13.

Two things came up resolving tests that need answers though:

1. ContentEntityBase::set() calls get(). We probably either need to do the same with set() and add an optional $throws_exception so it can be passed to get(), or we have set() call get() explicitly with $throws_exception as TRUE, so exceptions will always be thrown for invalid fields
2. get() is documented in FieldableEntityInterface to throw an InvalidArgumentException on invalid field names, but in ContentEntityBase::getTranslatedField, which is called by get(), it's also possible to throw an exception when "The entity object refers to a removed translation". My thought then is that not all \InvalidArgumentExceptions should be suppressed, but only ones with message "Field $name is unknown", but this seems brittle

For me, it's either default TRUE with D12 deprecation

Made the changes to this in the MR and CR. I think I prefer the other way, but I'd like to see this get in, and even doing , FALSE is better than hasField() checks.

There's probably also still a bit more time to do a deprecation to flip the default value before D13 if wanted as well, and I can update this MR again if others prefer it the other way.

I think we don't need to do that and keep it like that. That's a bigger change that's more likely to affect code. I think if we do that, we'd need to make it a D13 deprecation, but this seems to complicate things quite a bit.

My understanding was that there are a couple instances in core (and unknown how many in contrib/custom) where an exception is expected/handled on an invalid field passed to get(), so throwing an exception should continue to be the default.

But once we get to D12 (or 13), it'd be more convenient not to have to do get($field_name, FALSE) to get a NULL return, when it seems likely that the NULL return is the desired behavior most of the time. Doing this would mean that very no uses of get() need to change now, but in the major change, only usages of get() that have explicit exception handling would need to be changed to get($field_name, TRUE), and other existing usages of get() without explicit exception handling would not need to change.

I'll make the MR changes if it's still preferred to keep the behavior the same through the next major.

Since it's not likely this makes 11.2.0, bumped the deprecation version to 11.3.0.

Note that there may need to be a follow up to tie the uninstall of implicit dependencies like block_content in this example to dependency calculations for configurations using the plugin.

Follow up for this: https://www.drupal.org/project/drupal/issues/3524219 📌 Add plugin class module dependencies to dependency calculation Active

godotislate → created an issue.

IS updated.

Adding 11.2.0 release priority tag for #29 and bug in #34.

I guess we accidentally cross-pushed the reroll, so I re-added a change in .deprecation-ignore.txt to put the new rules under the right header.

Applied the MR suggestion and rebased. Since the change is minor and there were no other suggestions, going to push this back to RTBC.

godotislate → changed the visibility of the branch 3522505-new-sdc-components to hidden.

MR with fix and test is up. I injected the component plugin manager instead of the plugin cache clearer into the theme installer, since it seemed to be overkill to clear all the plugin caches, but should be easy enough to change if needed.

Here's a failing kernel test that shows a couple issues going on.

<?php

declare(strict_types=1);

namespace Drupal\KernelTests\Components;

use Drupal\Core\Theme\Component\ComponentValidator;
use Drupal\Core\Theme\Component\SchemaCompatibilityChecker;
use Drupal\Core\Theme\ComponentNegotiator;
use Drupal\Core\Theme\ComponentPluginManager;

/**
 * Tests discovery of components in a theme being installed.
 */
class ComponentPluginManagerDiscoveryTest extends ComponentKernelTestBase {

  /**
   * {@inheritdoc}
   */
  protected static $modules = ['sdc_test'];

  /**
   * {@inheritdoc}
   */
  protected static $themes = ['stark'];

  public function testComponentDiscoveryOnThemeInstall(): void {
    $definitions = \Drupal::service('plugin.manager.sdc')->getDefinitions();
    $this->assertArrayNotHasKey('sdc_theme_test:bar', $definitions);
    \Drupal::service('theme_installer')->install(['sdc_theme_test']);
    $definitions = \Drupal::service('plugin.manager.sdc')->getDefinitions();
    $this->assertArrayHasKey('sdc_theme_test:bar', $definitions);
  }

}

First thing is that because there is no container rebuild or component plugin cache clear on theme install, $definitions get cached in the first line before the theme is installed, and the cache is not invalidated after theme install, so the last line's assert fails.

However, even if we just add a plugin cache clear after the theme install like this:

  public function testComponentDiscoveryOnThemeInstall(): void {
    $definitions = \Drupal::service('plugin.manager.sdc')->clearCachedDefinitions();
    $this->assertArrayNotHasKey('sdc_theme_test:bar', $definitions);
    \Drupal::service('theme_installer')->install(['sdc_theme_test']);
    $definitions = \Drupal::service('plugin.manager.sdc')->getDefinitions();
    $this->assertArrayHasKey('sdc_theme_test:bar', $definitions);
  }

The last line still fails, because there is no container rebuild on theme install, so the component plugin manager has not changed, and the DirectoryWithMetadataPluginDiscovery $discovery object property in the component plugin manager does not get its $directories property updated to include the new theme's component directory. So as long as the component plugin definitions aren't cached again before the next request, the component plugins should be updated correctly, which can be seen if with the test rewritten to pass like this:

<?php

declare(strict_types=1);

namespace Drupal\KernelTests\Components;

use Drupal\Core\Theme\Component\ComponentValidator;
use Drupal\Core\Theme\Component\SchemaCompatibilityChecker;
use Drupal\Core\Theme\ComponentNegotiator;
use Drupal\Core\Theme\ComponentPluginManager;

/**
 * Tests discovery of components in a theme being installed.
 */
class ComponentPluginManagerDiscoveryTest extends ComponentKernelTestBase {

  /**
   * {@inheritdoc}
   */
  protected static $modules = ['sdc_test'];

  /**
   * {@inheritdoc}
   */
  protected static $themes = ['stark'];

  public function testComponentDiscoveryOnThemeInstall(): void {
    $definitions = \Drupal::service('plugin.manager.sdc')->getDefinitions();
    $this->assertArrayNotHasKey('sdc_theme_test:bar', $definitions);
    \Drupal::service('theme_installer')->install(['sdc_theme_test']);
    \Drupal::service('plugin.manager.sdc')->clearCachedDefinitions();

    // Create a new component plugin manager object to simulate a new plugin
    // manager object instantiated on the next request.
    $manager = new ComponentPluginManager(
      \Drupal::service('module_handler'),
      \Drupal::service('theme_handler'),
      \Drupal::service('cache.discovery'),
      \Drupal::service('config.factory'),
      \Drupal::service('theme.manager'),
      \Drupal::service(ComponentNegotiator::class),
      \Drupal::service('file_system'),
      \Drupal::service(SchemaCompatibilityChecker::class),
      \Drupal::service(ComponentValidator::class),
      \Drupal::getContainer()->getParameter('app.root'),
    );
    $definitions = $manager->getDefinitions();
    $this->assertArrayHasKey('sdc_theme_test:bar', $definitions);
  }

}

So I think there are two things going on:

1. If the component plugins have been cached before a new theme install, installing a new theme does not clear the cache, and the theme's plugins don't get found
2. Even if the plugin cache is cleared during theme install, rebuilding the component plugin cache within the same request will not pick up the new plugins because the manager's discovery directories are not rebuilt

MR failed tests.

I added some comments as well. I think the biggest issue on the MR is that there is a confusion between "precision", which is documented as the number of significant digits, (the number of all digits before and after the decimal), vs "scale", which is the number of digits after the 0.

Added CR https://www.drupal.org/node/3523753 → .

One thought that came up is that there maybe should be a follow up to get the module dependencies found here to be added to dependency calculations for plugin class instances used in config.

Also mentioned this in a comment in 📌 Add a fallback classloader that can handle missing traits Active (comment #72), but the MR here does address a small bug from that issue. It might makes to create another issue to address that bug specifically with a subset of work done here.

There's a small bug here in:

                  // If we arrive here a second time, and the namespace is still
                  // unavailable, ensure discovery is skipped. Without this
                  // explicit check for already checked classes, an invalid
                  // class would be discovered, because once we've detected a
                  // a missing trait and aliased the stub instead, this can't
                  // happen again, so the class appears valid. However, if the
                  // namespace has become available in the meantime, assume that
                  // the class actually should be discovered since this probably
                  // means the optional module it depends on has been enabled.
                  if (!isset($this->getPluginNamespaces()[$missing_class_namespace])) {
                    $autoloader->reset();
                    continue 2;
                  }

In Drupal\Component\Plugin\Discovery\AttributeClassDiscovery, $this->getPluginNamespaces() returns $this->pluginNamespaces;, which is the container namespaces, so this works correctly, with test coverage showing so. However, the subclass Drupal\Core\Plugin\Discovery\AttributeClassDiscovery (or its subclass AttributeDiscoveryWithAnnotations) is the class used by the DefaultPluginManager, and there $this->getPluginNamespaces() is generated from container namespaces with the expected plugin subdirectory appended to each. So instead of array keys like Drupal\node, Drupal\user, etc., instead the keys are (for block plugins) Drupal\node\Plugin\Block, Drupal\user\Plugin\Block, etc.

That means !isset($this->getPluginNamespaces()[$missing_class_namespace]) is always TRUE.

The changes in the MR for 📌 Move multiple provider plugin work from migrate into core's plugin system Active fix this, but since there are other more involved changes there, maybe a separate issue just to address the above is needed? It can be addressed with a subset of the 📌 Move multiple provider plugin work from migrate into core's plugin system Active work, though there might be explicit test coverage that needs to be added.

I think this is at NW based on latest comments.

We should be able to allow BC for the theme key in the plugin itself and throw a deprecation if it's set, and automatically translate it to themes at runtime?

I made suggestions to handle translating the value to "themes" in the form and deleting in the submit, but I forget about translation at runtime. I'm not sure how this can be done, since I'm not sure how to know which property/subproperty in a config entity would correspond to this. And similarly outside of the form submit, how can the property be identified in a config save?

Separately, it might make sense to create a follow up to add constraints to the config schema for the new property, reusing the ExtensionAvailable work in 📌 [PP-1] core.extension should be validatable Postponed once that's in.

Does there need to be a BC layer for the old theme property? I think this might be especially tricky because I don't think there's anything in core that uses this condition plugin's configuration, so there's nothing to write update hooks. Contrib/custom will have to write to own update hooks as needed, most likely.

Some comments on the MR. I'm also wondering if the mail theme classes shouldn't be in the `Drupal\Core\Mail` namespace instead of `Drupal\Core\MailTheme`? Though I think everything mentioned is personal preference and not necessarily a blocker.

godotislate → changed the visibility of the branch 3494634-compatibility-between-sdc to hidden.

Build failed. Mayb pipelines had a transient error, and the build needs rerunning?

Also, the MR should be moved out of draft if it is ready.

Rebased. Back to NR.

OK, after a couple failed attempts, I'm not sure that consolidating the cache set code is possible without a larger refactor of form processing.
The challenge is that between processForm() and rebuildForm(), in each we want to cache the form array as it is before the call to doBuildForm() with the $form_state object after the call to doBuildForm.

The flow looks like this:

processForm
  - save form array here
  - doBuildForm
  - process input
  - rebuildForm (if necessary)
  - cache form array + form state if not already cached in rebuildForm

rebuildForm
  - save form array here
  - doBuildForm
  - cache form array + form state

If the caching step is removed from rebuildForm is removed, and caching is done at the end of processForm regardless of whether the form is rebuilding, this works in most cases, except in batch processing.

Because for batch processing, there's this in buildForm():

    if ($request->getSession()->has('batch_form_state')) {
      // We've been redirected here after a batch processing. The form has
      // already been processed, but needs to be rebuilt. See _batch_finished().
      $session = $request->getSession();
      $form_state = $session->get('batch_form_state');
      $session->remove('batch_form_state');
      return $this->rebuildForm($form_id, $form_state);
    }

So rebuildForm() is called directly without a call to processForm(), and the form/form state don't get cached.

If all the caching is done within doBuildForm(), with the form array being saved at the beginning of the method, and the caching being done right before the return statement, the form state that gets cached does not include the changes made from input processing after the doBuildForm call in processForm, if the form is not rebuilding.

I think the MR is probably left as is, though I can remove this: @todo Find a way to avoid caching code duplication.

However since the test is all the same process, PHP already knows the enum/class exists after I created instances of test enum/classes and caches it (AFAICT), so modifying the autoloader after the fact wont work.

Ah, right. Once the class/enum is loaded by PHP into memory, it stays there.

Maybe there should be a comment in setModuleList that hook implementations can only be reset to be limited to a subset of installed modules. For module[s] being installed, hook implementations should be retrieved from the new module handler after container rebuild.

Note that the update hook should probably only target the default layouts (the entity view displays).

There could be thousands+ of nodes/entities with overridden layout data that have field blocks. Writing an update hook to load, check the nodes overridden layout for field blocks, then resave the nodes does not sound realistic. As long as the behavior for a field block that does not have any value for the new property is the same as the new property having empty value, I think this should be fine.

My question is really whether resetImplementations is useful anymore, since it's likely that the list of implementations should be pulled from the new container instance.

module_set_weight() was a candidate, but you've found that the order does not apply until after a container rebuild.

In Drupal\Core\Theme\Registry::get(), if the kernel is an UpdateKernel, it temporarily sets the module list to be just the system module.
So this might be the only remaining useful test case?

Currently, the module weights in configuration determine the order.
If you call module_set_weight(), the new order will become active for hooks after a container rebuild.

Sounds good.

Bringing it back to resetImplementations(), are there any uses left where it is called (either directly or via setModuleList) and there is not an impending container rebuild?

Not sure it can make it, but it would be nice to get in for 11.2 so that a couple classes being removed could be removed clean.

Took a shot at consolidating the cacheing code, but it broke other tests, so I reverted. Might try again tomorrow.

Actually this should have no effect on hook implementation order, which is determined at container build time.

Is this a regression or an intentional change? Before, module weights would be part of what determines the order in which hook implementations run.

Nice fix. I totally missed accounting for entries in $this->listenersByHook and the fact that the listeners are filtered by the current module list event after being retrieved from the event dispatcher in getFlatHookListeners().

I'm still not sure that that testing hook implementations like this around module install/uninstall is meaningful, because of the container rebuild. While the test now passes for uninstall, it would not on install. If I change the test like to look like this:

  /**
   * Tests that resetImplementations clears the invokeMap memory cache.
   *
   * @covers ::resetImplementations
   */
  public function testResetImplementationsClearsInvokeMap(): void {
    $moduleHandler = \Drupal::moduleHandler();
    /** @var \Drupal\Core\Extension\ModuleInstallerInterface $moduleInstaller */
    $moduleInstaller = \Drupal::service('module_installer');
    $moduleInstaller->install(['module_test']);
    $this->assertTrue($moduleHandler->hasImplementations('system_info_alter'));
    $moduleInstaller->uninstall(['module_test']);
    $this->assertFalse($moduleHandler->hasImplementations('system_info_alter'));
  }

The test will fail on $this->assertTrue($moduleHandler->hasImplementations('system_info_alter'));, because the old event dispatcher does not have hooks registered from the newly installed module_test.

I think for tests/code around container rebuilds, it doesn't make much sense to use service variables that point to the old container services. If anything, they're a source of a memory leak, since the references can't be garbage collected.

Not sure how else to test resetImplementations. I see that module_set_weight() has a called to

setModuleList()

. Are there tests around changing module weights and how that affects hook ordering?

godotislate → changed the visibility of the branch 3514197-fix-moduleHandlerGet to hidden.

Added change record.

Also in converation with @catch on Slack, moved the `cachetags` table truncation before clearing the cache bins, because having entries in the cachetags table doesn't hurt anything, but there could be an issue if the cache bin tables do without corresponding checksums in cachetags. Pushing back to NR for that.

Tried to POC this and went with a different approach of using a property in the .info.yml, since the module handler is available in DefaultPluginManager and can be passed into AttributeDiscoveryWithAnnotations. Unfortunately, extension objects in ModuleHandler do not have extra info data, ExtensionList is needed for that. DefaultPluginManager does have that as a property, but it is not instantiated except in subclasses, which is kind of a pain.

I'm at a loss for how to test this, as core/tests/bootstrap.php always adds all modules to the autoloader. I've tried a bunch of workarounds but I cant seem to get far. I'd appreciate if anyone can give that a go...

See Drupal\Tests\migrate_drupal\Kernel\I18nQueryTraitTest::disablePsr4ForUninstalledModules() for an idea on how this can be done, but it's only been used for a very limited test use case.

Made change to test based on feedback.

Added the first test mentioned in #109.

There's been back and forth about replacing usages of $this->container->get() with Drupal::service(), and/or avoiding service objects cached in test class properties in 🌱 Use \Drupal consistently in tests Needs work .

Put up MR over in 📌 Move multiple provider plugin work from migrate into core's plugin system Active to do the filtering mentioned in #24.

I touched on this in #11, but looking at it more, I don't think resetImplementations() does much on its own. If we look at ModuleHandler::add() (deprecated, I know), after the call to resetImplementations(), there's a call to HookCollectorPass::collectAllHookImplementations(), and the invokeMap property of ModuleHandler is updated after that. When a module is uninstalled, there's a call to setModuleList(), which calls resetImplementations(), but does not do anything after that as in add().

Otherwise, just calling resetImplementations() and then hasImplementations() subsequently just ends up pulling the implementations from the listeners list in the event dispatcher, which has not changed.

I think for resetImplementations() to work on its own, 🐛 HookCollectorPass registers event listeners but they do not follow the required calling convention Active needs to be done to decouple implementations from the event dispatcher, so that the list is managed directly by the module handler or another service where the list is similarly reset and rebuilt.

How much this is all necessary is also a question, because on module install/uninstall, the container is rebuilt with a new module handler and event dispatcher, so the lists should be up to date in the new service objects.

I think the only plugin type that's unconverted in core is migrate source. One way to proceed without waiting for migrate drupal to be moved out of core would be to the container parameter option mentioned in #18, and use it to specify plugin namespaces per module that don't need to be scanned for annotations. So there'd be a core.plugin_namespaces_converted (better name TBD), and for every module, {module_name}.plugin_namespaces_converted. The core and module parameters get merged to into global parameter, as is done with other similar parameters. The parameter definition would look something like this:

mymodule.services.yml

parameters:
  mymodule.plugin_namespaces_converted:
   - Drupal\mymodule\Entity
   - Drupal\mymodule\Plugin\Validation\Constraint
...

To declare that a module has all its plugins converted, could maybe do this:

parameters:
  mymodule.plugin_namespaces_converted:
   - *
...

Then in AttributeDiscoveryWithAnnotations::parseClass(), it'd be something like:

  protected function parseClass(string $class, \SplFileInfo $fileinfo): array {
    // Parse using attributes first.
    $definition = parent::parseClass($class, $fileinfo);
    if (isset($definition['id'])) {
      return $definition;
    }

    $provider = $this->getProviderFromNamespace($class);
    if (\Drupal::getContainer()->hasParameter("$provider.plugin_namespaces_converted")) {
      $converted_namespaces = \Drupal::getContainer()->getParameter("$provider.plugin_namespaces_converted");
      $namespace = implode("\\", array_slice(explode("\\", $class), 0, -1));
      if (array_intersect(['*', $namespace], $converted_namespaces)) {
        return ['id' => NULL, 'content' => NULL];
      }
    }

It might be a bit cumbersome to add lists of plugin namespaces to every module's services.yml, though. And also to inject the container parameter to AttributeDiscoveryWithAnnotations via its constructor.

MR https://git.drupalcode.org/project/drupal/-/merge_requests/12029 restoring the fix and adding an excludePaths entry for update-countries.sh to phpstan. Tests pass, but not completely sure that proves anything since the original MR PHPStan job passed too.

godotislate → made their first commit to this issue’s fork.

Re-opening because this looks like it's breaking HEAD:

phpunit core/tests/Drupal/KernelTests/Core/Extension/ModuleHandlerTest.php --filter=testResetImplementationsClearsInvokeMap
Clearing old webdriver sessions
jq: error (at <stdin>:5): Cannot iterate over null (null)
PHPUnit 10.5.38 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.19
Configuration: /var/www/html/core/phpunit.xml

F                                                                   1 / 1 (100%)

Time: 00:00.381, Memory: 4.00 MB

There was 1 failure:

1) Drupal\KernelTests\Core\Extension\ModuleHandlerTest::testResetImplementationsClearsInvokeMap
Failed asserting that true is false.

/var/www/html/core/tests/Drupal/KernelTests/Core/Extension/ModuleHandlerTest.php:52

FAILURES!
Tests: 1, Assertions: 2, Failures: 1.

I think the issue with the test is that on uninstall of module_test, the container is rebuilt. So the $moduleHandler variable is referencing an outdated module handler from the old container. Even with invokeMap set to empty, following the call stack of getHookListeners(), it goes to getFlatHookListeners() and then $this->eventDispatcher->getListeners(). Since $this->eventDispatcher is also referencing an outdated object from the old container, its listeners aren't refreshed, so AFAICT the list of hook implementations does not change.

Another option would be to re-convert

migrate_drupal

migrate source plugins to attributes and set the source_module property for them all in a hook_migrate_source_info_alter implementation in migrate_drupal. Effort probably would not be that high because all the plugin class changes could be brought back from an earlier commit, so it would just be purging the source_module property from them and writing the giant alter hook.

With the @todo comment in the trait updated to point at 📌 Deprecate AnnotatedClassDiscovery Active and the follow up for Migrate source as noted in #73, I think this is good to go back to RTBC.

Sorry, not sure how the status got changed to NR.

we could just note in that issue to make sure that trait gets looked.

Updated IS in 📌 Deprecate AnnotatedClassDiscovery Active to reference 3 classes mentioned in #54 and trait in #62.

Re #62: the @todo in the trait could probably be updated to point at 📌 Deprecate AnnotatedClassDiscovery Active , or we could just note in that issue to make sure that trait gets looked.

parseClass() in migrate's AttributeDiscoveryWithAnnotationsAutomatedProviders does need a similar change to what was done in core's AttributeDiscoveryWithAnnotations parseClass(). Nice spot.

(The migrate source plugin stuff is really annoying.)

A couple thoughts on issues that might need accounting for, without confirming in code:

• If someone has registered a class in their module's Hooks folder as a service in their *.services.yml, whether by mistake or holdover from D10 compatibility, probably need to make sure that that class method is not called twice for the hook
• If someone alter's an existing service using a ServiceModifier or service decorator, then presumably the hook methods in the original class, and for decorated services, hook methods in every class except the outermost decorator, should not run. So only the outermost class's hook methods should be invoked

If there are complications with either scenario that aren't easy to resolve, then maybe perhaps to reduce scope it can be documented that hooks aren't supported for altered services in whatever use cases and punt those to be done later.

No, you'd have to add the attribute with the link to all relevant entity types.

so if the idea is to make the entity type attributes smaller, I'd be a +1 for that idea.

Depends on what you mean by "smaller". There wouldn't need to be as much defined in the main attribute, but since that metadata does need to be defined somewhere, as a result there are a ton of other attributes that need to be set on the entity type. Just from a visual standpoint, it's just as imposing if not more so, which has been the pushback from others commenting so far. Examples of the conversion are in the MR for the Comment and Node entity classes.

OK, went ahead with the second argument to get() as mentioned in #7-10. Followed the steps for adding a method parameter from How to deprecate → . Since the method signature will need to be updated by subclasses anyway in D12, I think the $field_name parameter should be typehinted to string and the return value should be typehinted to FieldItemListInterface, but I'm not sure what the policy is on how to declare a return type addition.

CR https://www.drupal.org/node/3522223 →
Follow up for D12: 📌 [PP-1] Add typehints and $throw_exception parameter to FieldableEntityInterface::get() and default to FALSE Postponed

godotislate → created an issue.

Made the change per the suggestion. Also changed back to invalidating multiple tags. I'm not sure it makes any difference, but it was trivial to do just in case.

Rebased, but maybe this should stay in NW for #20.

I think all MR comments addressed.

Questions answered. +1 for RTBC.

Tests are fixed.

Couple comments on the MR: one nit that's not a blocker. One thing that is probably an out-of-scope bug without test coverage.

If the deprecation of those classes is being done here, should 📌 Deprecate AnnotatedClassDiscovery Active be used for their removal in 12.x?

Do the annotation-only discovery classes need to be deprecated? For any plugin managers out there that override getDiscovery() and use these classes specifically. I think last year there was an issue in Rules, because that module had a plugin manager like that. Though, the plugin manager would also need to not inherit from DefaultPluginManager or call its constructor, because the deprecation for the missing $plugin_definition_attribute_name property would be triggered for any that do.

Drupal\Component\Annotation\Plugin\Discovery\AnnotatedClassDiscovery
Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery
Drupal\migrate\Plugin\Discovery\AnnotatedClassDiscoveryAutomatedProviders