Add a fallback classloader that can handle missing traits

Created on 28 January 2025, 2 months ago

Problem/Motivation

See πŸ“Œ Use alternate parsing method for attribute-based plugin discovery to avoid errors Active for some discussion. This doesn't replace that issue, it just provides a possible alternative way to handle missing traits, which is part of what that issue deals with.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

base system

Created by

πŸ‡¬πŸ‡§United Kingdom catch

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    Added comments to the MR.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    @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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    #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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡ΊπŸ‡ΈUnited States dww

    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. :)

  • Merge request !11072Draft: Resolve #3502913 "Test migrate source" β†’ (Closed) created by godotislate
  • Pipeline finished with Failed
    2 months ago
    Total: 138s
    #411494
  • Pipeline finished with Failed
    2 months ago
    Total: 239s
    #411500
  • Pipeline finished with Failed
    2 months ago
    Total: 146s
    #411511
  • Pipeline finished with Failed
    2 months ago
    Total: 486s
    #411529
  • 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.

  • πŸ‡¬πŸ‡§United Kingdom catch
    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    @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.

  • Pipeline finished with Failed
    2 months ago
    Total: 6001s
    #411579
  • 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.

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    πŸ“Œ Parse attributes before annotations Active is blocked on this.

  • πŸ‡¬πŸ‡§United Kingdom catch
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 1501s
    #423521
  • Pipeline finished with Failed
    about 2 months ago
    Total: 326s
    #423536
  • Pipeline finished with Failed
    about 2 months ago
    Total: 369s
    #423546
  • Pipeline finished with Failed
    about 2 months ago
    Total: 469s
    #423568
  • Pipeline finished with Failed
    about 2 months ago
    Total: 388s
    #423589
  • Pipeline finished with Failed
    about 2 months ago
    Total: 828s
    #423639
  • Pipeline finished with Failed
    about 2 months ago
    Total: 542s
    #423649
  • 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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 378s
    #435862
  • Pipeline finished with Failed
    about 1 month ago
    Total: 344s
    #435878
  • Pipeline finished with Failed
    about 1 month ago
    Total: 113s
    #435906
  • Pipeline finished with Failed
    about 1 month ago
    Total: 708s
    #435915
  • Pipeline finished with Success
    about 1 month ago
    Total: 1057s
    #435932
  • 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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    @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 .

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • Pipeline finished with Success
    about 1 month ago
    Total: 1546s
    #438077
  • 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.

  • Pipeline finished with Success
    about 1 month ago
    Total: 388s
    #438476
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    There is already a pull request with a fix.

  • Pipeline finished with Success
    about 1 month ago
    Total: 2239s
    #438779
  • 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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • 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?

  • πŸ‡¬πŸ‡§United Kingdom catch

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

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

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

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch
Production build 0.71.5 2024