Add a fallback classloader that can handle missing traits for attribute discovery

Added comments to the MR. I dont see anything else in the code that stands out as 'is that right?'

It's illegal to throw an exception from class loading

True, yes this seems quite creative approach.

@catch Ah, okay. Thanks.

This approach is a clever idea, and it does decrease the chances of a fatal error.

Commented (#27) in #3490322: Use nickic/php-parser for attribute-based plugin discovery to avoid errors, where I tested a similar solution. A couple still outstanding points:

• This relies on traits having names that end in "Trait" (and no classes or interfaces that do), though it might be edge-casey to be using traits that don't
• Trait use statements that alias methods (and likely similar) will still fatal error. Example:
  

  use CustomTrait {
      theMethod as theTraitMethod;
  }
  

  Since StubTrait doesn't have "theMethod", reflection still fatal errors.

I made a suggestion that the "class_exists" check be moved to the top, even before trying to retrieve the definition from file cache. This is to skip over definitions that were previously stored in file cache but should be skipped now because modules the definitions have implicit dependencies on are now uninstalled.

IMO, given the relative simplicity of this solution, it's worth going forward on, even with the noted issues. It's better than existing without adding much complexity.

In principle, this all sounds good.

Initial cleanup of MR metadata and resolving threads that have already been addressed.

Need to do a more thorough review, but no time for that right now.

Or in Drupal\Component\Discovery?

I think this makes sense, but it's probably fine to leave it in the plugin namespace for now until it's needed for other discovery? Can even use the work from #3502882: Add a classloader that can handle class moves to move it later. :)

Started MR 11072 incorporating a minimal set of changes from the #3421014: Convert MigrateSource plugin discovery to attributes MRs to test the problematic source plugins using I18nQueryTrait, on top of the changes from MR 11030.

Have some test failures on that MR will look into later.

The debug classloader being unhelpful again.

Been debugging this, but don't understand what's going on yet.

The other failure is this one, which I don't think is exactly a problem with the changes introduced this issue:

Drupal\Tests\migrate\Kernel\Plugin\MigrationPluginListTest::testGetDefinitions
Failed asserting that an array is empty.

core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php:81

Basically, the Migrate source plugin manager filters out definitions using the ProviderFilterDecorator and the way that AnnotatedClassDiscoveryAutomatedProviders extracts multiple providers for source plugins based on used namespaces. Since the hope here is replace that multiple provider functionality specific to migrate source with the class_exists checks, during test runs plugins are not filtered out as expected because classes/traits/interfaces for all modules are loaded during test runs.

Fixed the namespace and all tests pass except the one in #18. That will be something to address #3421014: Convert MigrateSource plugin discovery to attributes, but everything should be good for this issue, at least functionally. I think MR 11030 will need another person to review, since I wrote the tests.

#3397318: Parse attributes before annotations is blocked on this.

I suggested it, but I wondered whether moving the class_exists() check in AttributeClassDiscovery::getDefinitions() to go before retrieving the definition from file cache can be avoided. While it does filter out definitions retrieved from cache from being discovered when their modules are not installed, it also means that every plugin class is loaded into memory in the request when the plugin manager gets the definitions from a cold cache. Separately, relying on class_exists to filter out plugins is difficult to test, since all classes are loaded in tests. I also wonder whether there may be a situation during cache rebuilds/module uninstalls where a plugin class in a module that is being uninstalled has already been loaded into memory earlier in the request, so erroneously does not get filtered out.

With this in mind, I tried moving that check back to after the file cache look up, and since Reflection in parseClass() should now be safe, I used reflection to loop through to get all class, interface, and trait dependencies for each plugin class. Then in Core AttributeClassDiscovery, I extracted the modules via namespace of those dependencies and checked whether the modules were installed. This check is done before including the plugin definition in the list returned from getDefinitions().

This broke a lot of tests, because there were usages of node, and entity_test entity types where the user module was not installed, and since those types implement Drupal\user\EntityOwnerInterface, those entity types were not being found without adding 'user' to the installed modules. (Separately there seems to be a lot deprecation warnings for I18nQueryTrait for some weird. reason, though there should not be any classes using the version of that trait in content_translation.)

Regardless, this filtering work probably is out of scope here and maybe belongs in #2786355: Convert multiple provider plugin work from migrate into plugin dependencies in core, but I wanted to see if it would work. See test MR 11072.

Separately there seems to be a lot deprecation warnings for I18nQueryTrait for some weird. reason, though there should not be any classes using the version of that trait in content_translation.

Have a feeling that running class_exists() on every file in the plugin directory means that the trait file is loaded and triggering this deprecation. Will look into it.

Should we revert here prior to moving the check up, and then open a follow-up for the filtering?

Sounds like a good way to go. We can either use #2786355: Convert multiple provider plugin work from migrate into plugin dependencies in core for the follow up or create a new one if that one doesn't fit.
Closing my test MR.

Should we add the conversion of \Drupal\layout_builder\Plugin\Block\InlineBlock into this issue? should be straight forward then and not require any workarounds that #3265945: Deprecate plugins using annotations and plugin types not supporting attributes currently has to do. And gives us real-world test coverage of this as well then.

Needs work as this apparently needs a rebase at least.

Rebased MR and did the following:

• Moved the class_exists check back below retrieving from file cache, per #26
• Converted InlineBlock to attributes, per #29
• Moved the StubTrait and class loader to Drupal\Component\Discovery, per #12

Also, the I18nQueryTrait deprecation warning came up because of the class_exists() checks. At first I wrongly moved the new Trait location out of the Plugin/migrate/source folder, which had no effect because the original deprecated trait was the one causing the issue. Instead of this, I set a custom error handler right before the class_exists() check to suppress any deprecation warnings, which I hope is kosher. Still, this is another reason to move non-plugin files out of the plugin discovery directories, and one thing to consider for #3397318: Parse attributes before annotations is whether to also use an error handler that suppresses deprecation warnings when instantiating Reflection in parseClass().

All tests pass now, so back to NR.

#3502882: Add a classloader that can handle class moves might fix the i18nQueryTrait issue

Yes, agreed. Should this issue be postponed on that one? Then maybe the InlineBlock conversion shouldn't be done here so it isn't a blocker for #3265945: Deprecate plugins using annotations and plugin types not supporting attributes?

Though one thing about I18nQueryTrait is that it was decided to keep the original trait intact and not a wrapper for the new trait so that the new trait could have type declarations. See #55 of #3258581: Move I18nQueryTrait from content_translation to migrate_drupal.

With #3502882: Add a classloader that can handle class moves now in, rebased and used that to remove I18nQueryTrait.php from content_translation.
Also added docblocks to the new classloader methods, and in doing so, missingClass didn't seem to fit, so renamed to missingTrait.

This should be ready again.

I created an issue against PHP - see https://github.com/php/php-src/issues/17959

There is already a pull request with a fix.

There is already a pull request with a fix.

They said several years back that traits and interfaces were much more difficult to deal with:

To support the same for interfaces/traits, the basic idea would be to have a separate opcode like COMMIT_CLASS, which separates the addition of the class to the hashtable from the binding procedure, in which case all binding errors could become exceptions. This part I would assume to be relatively straightforward. The issue is that the inheritance logic also modifies the class in-place, this means that it's not possible to run inheritance on a class entry twice (e.g. after defining a necessary interface that previously threw). Solving that part wouldn't be so simple.

Though they clearly starting throwing exceptions for missing interfaces as well sometime between then and now.

Nice!

We'd be able to put the classloader registration/unregistration behind a PHP version check in Drupal 11.4-ish though.

I wonder if the classloader, or a similar one, should be kept in place, just to set a flag whenever there is a missing class/interface/trait. The missing class/interface detection right now is based on '/(Class|Interface) .* not found$/' regexp match, which is brittle and will need to be expanded to include traits. Instead of that, the classloader could set a flag for any detected missing dependency, and when catching the exception, use that flag instead of the regexp match. Actually, I think that can be done now, with the removal of trait aliasing being the only necessary change future change once PHP 8.5 is required?

instead of TraitSafeClassLoader how about MissingClassDetectionClassLoader

Funny enough this is what I also came up with.

Applied suggestions from @quietone.

Applied two more suggestions. Two comments from @quietone are still open, though I added a comment/question to one.

This is pretty close I think. I have one suggestion for the comment describing what is happening.

I have one clarifying question that I couldn't quite understand.
One comment commiserating on the actual workaround bit lol.

Not a fan of final, it's already marked as internal which should be sufficient. I wouldn't block this issue on that opinion since it's not settled.

I think catch's involvement means we can remove the framework manager tag.

After some back and forth on slack the confusing bit has been clarified on why it's inverse.

I think this is unfortunate but necessary, and it's only temporary since the upstream issue was committed so quickly.

Looks good to me, just some ubernits. Fine to self RTBC after addressing

Applied two suggestions. Comment about test coverage is still open, but I wonder if something can be added to core/tests/Drupal/Tests/Component/Plugin/Attribute/AttributeClassDiscoveryCachedTest.php for that.

I actually added #3520811: Add test coverage for AttributeClassDiscovery for unexpected class_exists exceptions and pushed a commit with the @todo, but we can pick one to close as a dupe.

Pushing back to RTBC per #57.

I rebased and reworded a comment. It did not change the meaning, so I am leaving this Major issue at RTBC.

Credits

Committed to 11.x - thanks!

In #9/#10, there was talk of adding a coding standard for all traits to have names that end in Trait. Is this still needed? Seems like once Drupal's minimum PHP version 8.5, we won't need to do detection specifically for traits.

I think it is worth opening an issue in the coding standards queue to discuss

Opened coding standards #3521443: Traits should always have the suffix "Trait".

Put up MR over in #2786355: Convert multiple provider plugin work from migrate into plugin dependencies in core to do the filtering mentioned in #24.

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 #2786355: Convert multiple provider plugin work from migrate into plugin dependencies in core 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 #2786355: Convert multiple provider plugin work from migrate into plugin dependencies in core work, though there might be explicit test coverage that needs to be added.