Add a fallback classloader that can handle missing traits

I borrowed the test coverage from 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active .

It doesn't pass yet, but I am able to prevent a missing trait from throwing a fatal error, and also still detect that a class/interface/trait was missing. It is quite evil but I think it will work.

Think I got it working.

It is a bit trickier than I thought although that is not suprising.

It's illegal to throw an exception from class loading, so we can't do that to make a fatal failure to load recoverable, it results in another fatal.

However, we can store if we swapped a trait in the classloader itself, then ask it after classloading whether that happened.

This works pretty nicely with one caveat:

The entire trick relies on class_alias to stub out missing traits.

The caveat is that because we stub out the trait, the class is successfully loaded in attribute parsing. So if that gets called twice, the second time, we can actually parse the attributes, which we don't want!

I added a class property in the attribute parser to store the classes we shouldn't try to parse again, just for the request, not in the filecache.

Another option would be to use class_uses() and look for our stub trait class, but that has to be done recursively and it would have to run on every class, so it seems a bit heavy.

Added comments to the MR.

I added a class property in the attribute parser to store the classes we shouldn't try to parse again, just for the request, not in the filecache.

Another option would be to use class_uses() and look for our stub trait class, but that has to be done recursively and it would have to run on every class, so it seems a bit heavy.

Third option would be to define a very uniquely named method on StubTrait, and then check methodExists(), and if it does, then we know the class_alias logic ran, and ignore the class then. This is a huge hack but it would be fast and not rely on static caching.

@oily thanks for the review, but I just copied the tests from the other issue to have a good testing base, and am not sure if the entire approach here would be accepted or not yet.

@catch Ah, okay. Thanks.

Commented (#27) in 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active , 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.

#9.1 yes and couldn't see any way around that. We could possibly add a coding standard to enforce this though, I think we already do with Interface

#9.2 that feels impossible to cover here. It might be very rare with the sorts of classes we're discovering but if not then it would still blow up.

Another problem here is that even though the autoloader is only used in this situation, any missing traits are aliased to StubTrait for the rest of the request, so if something called trait_exists() it would return true. Might be theoretical but worth no

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.

We could put it Drupal\Core\Classloader which is added by 📌 Add a classloader that can handle class moves Active maybe? I nearly did that, then I thought it would only be used for discovery, but you're right there is more discovery than plugins once we start adding routes etc.

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 📌 Add a classloader that can handle class moves Active to move it later. :)

Started MR 11072 incorporating a minimal set of changes from the 📌 Convert MigrateSource plugin discovery to attributes Active 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.

RuntimeException: The autoloader expected class "Drupal\migrate_plugin_config_test\Plugin\migrate\source\SimpleSource" to be defined in file "/builds/issue/drupal-3502913/core/modules/migrate/tests/modules/migrate_plugin_config_test/src/Plugin/migrate/source/SimpleSource.php". The file was found but the class was not in it, the class name or namespace probably has a typo.

The debug classloader being unhelpful again.

The debug classloader being unhelpful again.

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

The other issue 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.

@godotislate I may have unfairly maligned the debug classloader.

The namespace declaration for that file is:

namespace Plugin\migrate\source;

Which is definitely not the namespace of the test module and looks like copypasta. Didn't test if this is the problem but it would fit with the error message.

Fixed the namespace and all tests pass except the one in #18. That will be something to address 📌 Convert MigrateSource plugin discovery to attributes Active , 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.

godotislate → changed the visibility of the branch 3502913-test-migrate-source to hidden.

📌 Parse attributes before annotations Active 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: Move multiple provider plugin work from migrate into core's plugin system → , but I wanted to see if it would work.

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.

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.

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

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: Move multiple provider plugin work from migrate into core's plugin system → for the follow up or create a new one if that one doesn't fit.
Closing my test MR.

godotislate → closed merge request !11072

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 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work 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.

Yeah I think #29 is a good idea, although it would probably then make this issue a hard blocker of that one, but if people otherwise think it's close to ready, that might be fine too.

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 📌 Parse attributes before annotations Active 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.

@godotislate I think 📌 Add a classloader that can handle class moves Active might fix the i18nQueryTrait issue because we could physically remove the file from that folder then?

#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 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work ?

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 🐛 Move content_translation I18nQueryTrait to migrate module Active .

Let's postpone and just try to get all three issues done. Tagging as an 11.2.0 release priority since this is blocking full attribute conversion now.

With 📌 Add a classloader that can handle class moves Active 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.

https://github.com/php/php-src/issues/17959 landed making this properly temporary, which is a relief because it's a massive hack.

It looks like it will only be in PHP 8.5, so the earliest we can drop it is Drupal 12 (assuming core requires PHP 8.5 by then). We'd be able to put the classloader registration/unregistration behind a PHP version check in Drupal 11.4-ish though.

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?

#43 is a good idea, if we do that here we also need to rename it..

For implementing #43, instead of TraitSafeClassLoader how about MissingClassDetectionClassLoader ?

instead of TraitSafeClassLoader how about MissingClassDetectionClassLoader

Funny enough this is what I also came up with.

Rebased and implemented #46 / #47 and I think all of @alexpott's feedback.

Refactored things a bit and added additional comments to try to make it clearer what can be removed once we require PHP 8.5. It's not a lot of additional code to avoid the error message parsing once the trait workarounds are removed again.

Applied suggestions from @quietone.

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

I made a commit for one of the comments, and left a possibly unsatisfactory answer on the other.

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.

On final I think this is case where we'd prefer it never existed, let alone that someone tries to extend it or call it directly, so even more internal than controllers etc.

I applied the comment suggestion which I agree reads better.

Got turned around on the implementation question, turns out it was right in the first place, but in the process added a long comment to that.

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 tried to add test coverage for this, and the new test passed, but then I removed that hunk and it still passed, so either the test was wrong or the code is unnecessary, but to be honest I don't know which it is. So I opened 📌 Add test coverage for Errors thrown from autoloading classes Active .

I actually added 📌 Add test coverage for AttributeClassDiscovery for unexpected class_exists exceptions Active 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.