AttributeClassDiscovery fails while trying to include non valid plugin class

Thanks for the report.

This is similar to the issue we have in 📌 Hidden dependency on block_content in layout_builder Needs work where a layout_builder block uses a trait from block_content, but there is no guarantee that block_content will be installed. Discovery crashes with a fatal error when it autoloads the block plugin and the trait does not exist.

Not sure what the best solution is.

So in this case the file has no annotation (or attribute) at all:

https://git.drupalcode.org/project/commerce/-/blob/8.x-2.x/modules/produ...

Instead hook_block_alter() is used to swap the plugin class for previously discovered blocks:

https://git.drupalcode.org/project/commerce/-/blob/8.x-2.x/modules/produ...

Possible options are:

1. Scan files for #[ attribute markers before trying to autoload them, which would sidestep this case
2. Catch the autoloader error - but if we silently discard it, it will make debugging real issues harder

Bumping to critical as this will presumably prevent Commerce running on 10.2.0.

> Scan files for #[ attribute markers before trying to autoload them, which would sidestep this case

This is not great for performance, I think we should actively discourage that appraoch (putting plugin classes that are not discovered through attributes in that folder), maybe log a deprecation message or so and suggest to only put attributes. They can live anywhere else.

I think we should actually do both, just in case. We should log fatal errors and not silently ignore them.

Modules could provide plugins for modules without depending on them before. As enhancements if it were installed.

Did we just lose that capability?

For some cases of that, yes.

Annotation did a file_get_contents() and parsed that comment, we on purpose did not load the file with PHP.

For attributes, we have to.

It still works as long as you don't extend from a base class, use an interface or trait or so from that module.

In AttributeDiscoveryWithAnnotations, StaticReflectionParser::parse() already uses file_get_contents() to check for annotations before we try to look for attributes, so reading it again won't be much less performant.

Another option is to investigate https://github.com/olvlvl/composer-attribute-collector for discovery, unsure if this suffers from the same problem or not.

Yes, my point was the same as what you just said. Should this check be part of the BC layer or will we keep it? For now, we can keep it in the same class and just parse the same string.

It still works as long as you don't extend from a base class, use an interface or trait or so from that module.

Here's another example from Commerce. We support the Entity Print module. It'd be asinine to make users download a commerce_entity_print module for one plugin, or even a submodule within Commerce for this. I think the propose resolution of a try/catch. But with a silent fail and not a warning every time a plugin fails to be discovered.

I don't see a plugin related to entity_print, that's an entity handler, which technically part of the entity plugin definition, so it's an attribute too, but it's just the reference to your own class, it doesn't get loaded.

I'm aware of the problem space, I have plenty of contrib modules that provide plugins condition for other modules. monitoring for example has lots of them. And that will still work unless discovery results in executing php code that results in an Error. injected dependencies, properties, calling stuff inside methods is all OK, The main issue is base classes, interfaces and traits.

This is already an edge case because you already had to resort to dynamically registering the plugin instead of using the provider key. We tried to account for this by parsing annotations first, and falling back to attributes second. This specific case is a problem because it has neither Annotation nor Attribute (obviously, since that is new). And now we'll extend the BC layer to look for an attribute before parsing it.

Silently ignoring parsing errors is going to cause a lot of confusion when your plugin doesn't exist because you are missing a use statement for the base class or something like that.

I guess this would work for this case, but it is a performance hit to read the file again, because file_get_contents() doesn't have any cache built-in or so, and the other call is not really accessible to us here:

\Drupal\Core\Plugin\Discovery\AttributeDiscoveryWithAnnotations::parseClass:

    // Only call the Attribute discovery if the file contains an attribute
    if (str_contains(file_get_contents($fileinfo->getPathname()), '#[')) {
      return parent::parseClass($class, $fileinfo);
    }
    else {
      return ['id' => NULL, 'content' => NULL];
    }

There is one option that we could consider, and that is to then go back to calling attribute discovery first again, that might make up for it because we then save the extra call for converted plugins?

The other option is the try/catch, which I think we should combine with an error message that goes a bit like this:

Error during plugin discovery for @type in @file: @error. Attribute discovery only supports plugin classes that can be loaded. Classes that for example depend on interfaces and base classes from other modules should be registered dynamically and moved out of the discovery-namespace.

I think we should consider doing both for maximum safety. As time goes on plugins may start using attributes for other reasons and so the string check may have false positives. There also will be cases where the attribute has been converted but the class relies on an optional dependency. We probably should add some test coverage as well.

We could also check for the explicit attribute name as we already know it - but that relies on code using use statements without aliases, so we might miss some valid cases.

> There also will be cases where the attribute has been converted but the class relies on an optional dependency. We probably
should add some test coverage as well.

I really think we shouldn't support those cases anymore with attribute discovery (as in, log an error then), the commerce example already did use the hook, it just means those cases can't live in the plugin namespace anymore then. I hope it won't affect that many cases, you really have to use a trait or interface or base class for that. It's not problem to have a property for a service of that module that doesn't exist, PHP doesn't complain about that.

> We could also check for the explicit attribute name as we already know it - but that relies on code using use statements without aliases, so we might miss some valid cases.

Yes, that's what I was thinking as well

I guess this would work for this case, but it is a performance hit to read the file again, because file_get_contents() doesn't have any cache built-in or so, and the other call is not really accessible to us here:

We could fix that by creating a microservice that wraps around file_get_contents(). But then that service would become part of core and we'd need to support that for all eternity even after this root issue is resolved and we are now stuck with a service wrapper around file_get_contents() even though we no longer call it on the same file twice.

Either way, if performance is an issue we could mitigate that. I'm not saying we should, though.

It would be easier to fork or extend StaticReflectionParser and pass in the file contents directly; there is already this awkward MockFileFinder class that is a value object for a filename that would be good to refactor away.

Regarding the performance hit: try to measure it. I would bet you will find the operating system caches the read. I would even argue it is the role of the operating system to cache two subsequent reads of the same file. Also, this is a rebuild time performance hit so even if you find there is a performance hit, I would suggest a more careful evaluation whether it matters.

I would also cautiously suggest to split this issue into two: one is when loading classes not containing attributes (a BC issue) and the other is completely valid classes doing so which depend on an optional module. My suggestion would be to build a composer plugin which collects the use statements in all installed files and check this against the autoloader map. Building this using the tokenizer shouldn't be too hard. If it is, call in the heavy cavalry and use nikic/PHP-Parser. If there's interest I might write this.

#19 made me think of another interim approach: we know the fully qualified class name of the attribute, and it must appear somewhere in the plugin file, either as a use statement or directly in the attribute - so can we search the file for that string to be even more sure before we try reflection?

> and it must appear somewhere in the plugin file

I'm not 100% sure about that. You can do partial use a of a namespace, like this:

use Drupal\Core\Block\Attribute;

#[Attribute\Block(...

AFAIK this approach is very common in Doctrine ORM and while our namespace pattern makes this pretty weird, it's possible, even with an alias if you want to be extra funny:

use Drupal\Core\Block\Attribute as BecauseICan;

#[BecauseICan\Block(

I pushed another change to try/catch any throwable, that will cover interfaces and test classes.

I tried extending the DefaultPluginManager kernel test, but unsure how to do the error handling exactly. IMHO we really should log this somehow or it will be extremely confusing when your plugin isn't found. The parser is currently in Drupal\Component, I tried triggering a php warning, but that fails the test. We could do the error handling either in the Core subclass, but would need to inject a logger?

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

I think str_contains(file_get_contents($fileinfo->getPathname()), '#[') can be moved to catch clause in parseClass. That way most files will not be scanned, and this can be a check to decide if we want to log/throw an error in case parsing fails.

And, I could be wrong, but I think it is okay to throw the critical error in case the file contains an attribute, but the class isn't valid due to whatever. There are no attributes in contrib right now, and that way while someone will be implementing them we can let them know that there shouldn't be discoverable invalid plugins and that they should do something with it (perhaps move the plugin to the submodule with all needed dependencies declared)

There is also a possible problem with logging an error and returning ['id' => NULL, 'content' => NULL]. In case it failed during parse but an error is related to some base class and not the plugin class itself, the plugin will not be rediscovered (even after drush cr) unless something changes in the plugin class. Due to fileCache in getDefinitions. I guess this can happen during development and can cost someone some time to debug.

I think in an ideal world the Commerce payment plugin class would not be in the Plugin folder for discovery. It's not being discovered - it's being altered in so it does not have to be there.

#24 sounds a clever way of doing it.

As a matter of principle I don't think we should support code extending classes that possibly don't exist in auto-discovery locations. But we can trigger a deprecation about that at a later date.

plugin class would not be in the Plugin folder for discovery.

To date I've never heard the Plugin folder be called as for only discovery. The only reason I've heard not to put code in Plugins is for an minuscule performance boost for not parsing an additional file which I've always been told only occurs at a cache refresh.

TBH I find it disorganized to keep plugin related code outside the namespace for which its related to. If we want to get fancy we can start excluding *Base.php for a minuscule performance boost, but I would hate to see this change make code bases even less organized.

IMHO we really should log this somehow or it will be extremely confusing when your plugin isn't found.

Consider linking it to $config['system.logging']['error_level'] or similar? No need to generate a report for an issue that can be easily handled occurs, yet still provide a way for devs and site owners creating new modules to diagnose an issue.

Can.we emit E_USER_WARNING when this happens? We do that in some other low level non critical conditions, users can choose to ignore these but at least we are reporting something somewhere.

> Can.we emit E_USER_WARNING when this happens? We do that in some other low level non critical conditions, users can choose to ignore these but at least we are reporting something somewhere.

That's exactly what the MR now does. But as far as I see, there's no way to not have tests fail on that as it's automatically converted into an exception. We can expect that, but that still means the test is interrupted early and does not finish. See https://git.drupalcode.org/issue/drupal-3400458/-/jobs/342310.

I do agree with #26/#27, I think we should no longer support that, one problem is that annotations explicitly support it now and even have test for it, see #2796953: [regression] Plugins extending from classes of uninstalled modules lead to fatal error during discovery → . Because annotation parsing is based on just getting the file, we even manage to parse the definition and have the plugin kind of exist, implicitly assuming that the providing module uses the provider = someothermodule feature to filter out out again.

Which still works, as long as you don't extend/use a base class/interface/trait.

> There is also a possible problem with logging an error and returning ['id' => NULL, 'content' => NULL]. In case it failed during parse but an error is related to some base class and not the plugin class itself, the plugin will not be rediscovered (even after drush cr) unless something changes in the plugin class. Due to fileCache in getDefinitions. I guess this can happen during development and can cost someone some time to debug.

fileCache is specifically designed to get invalidated when a file changes, so I don't think that's a concern.

> I think str_contains(file_get_contents($fileinfo->getPathname()), '#[') can be moved to catch clause in parseClass. That way most files will not be scanned, and this can be a check to decide if we want to log/throw an error in case parsing fails.

I'm not sure if I understand this. The problem is that traits specifically are a parse error that we can _not_ catch. We can catch a missing base class and an interface, but not that, no idea why. So we need make sure that we don't even attempt to parse a class that has no attribute, or we cause immediate non-recoverable fatal errors on existing projects as they update to 10.2.

My proposal would be to silently catch those parse errors in the BC layer ( AttributeDiscoveryWithAnnotations) to fix the critical regression and then have a non-critical follow-up to see if we can have some kind of logging and how to handle it with phpunit. If nothing else, we could at least move it to a separate plugin namespace and test, so we only trigger it in that specific test.

Adding some comments from slack regarding non plugins in discovery namespaces

A common use case is swapping the class of an existing plugin. Eg you want system menu block to behave differently so you use the alter hook and change the discovered class. Where do you put that class? It can go anywhere but for discoverability I suspect many people would keep the Plugin/Block namespace




Or an abstract base class like MyModuleBlockBase

FWIW I think it is fine to deprecate support for that and advise folks to move them to the parent namespace (Plugin) perhaps?

So I think the recent changes to the MR allow us to make an important change to \Drupal\Core\Plugin\Discovery\AttributeDiscoveryWithAnnotations::parseClass(). Which I also think we should make before the release.

I think if both an attribute and an annotation are present we should use the attribute first. I guess we can handle that in a follow-up.

FWIW I think it is fine to deprecate support for that and advise folks to move them to the parent namespace (Plugin) perhaps

:/ as someone who constantly decorates plugins, I don't like this.

:/ as someone who constantly decorates plugins, I don't like this.

I don't like it either, most of my projects have at least half a dozen

@mglaman can you give some examples where you are decorating a plugin and the code has dependencies that might bot be there. I think the more examples we have, the better we'll be able to work out what's the best path forward.

@mglaman: I'm not sure if it's entirely clear, but note that this only a problem if your overridden classes extend from base classes or use interfaces/traits of modules that are *not installed*.

This is somewhat common in contrib, like the case here with commerce providing optional integration with layout_builder, but I would assume it's far less common in custom code, either you have a module installed that you integrate with or you don't. *Maybe* there's some leftover code that you didn't remove yet for stuff that was uninstalled (although there's still the scenario when you need to uninstall a no longer needed dependency).

Having any other classes that don't have attributes/ annotations in the plugin namespace is just a (very) minor performance overhead but will not fail. I think there might be a value in having a standard/recommendation to put them in a corresponding but different namespace, like PluginOverride\foo_module\Bar, but that would just be that, a recommendation, and I see advantages and disadvantages to that.

Nice idea to extend StaticReflectionParser - a few nits but this looks good and the additional test coverage in all areas is welcome.

I added Drupal Commerce to 10.2.x and reproduced the error from the issue summary after installing Commerce Product:

Error: Class "Drupal\layout_builder\Plugin\Block\FieldBlock" not found in include() (line 21 of modules/contrib/commerce/modules/product/src/Plugin/Block/VariationFieldBlock.php). 

After applying the MR and rebuilding caches, the error is gone.

Thanks for the review @longwave - addressed the points.

Very impressive test coverage.

I've worked on this as well, but I don't think there's anything left of my actual code changes.

DefaultsPluginManager now does some vfsStream trickery to ignore files with problems for the pure Attribute discovery. I've added a reference and some notes to 📌 [meta] Convert all core plugin types to attribute discovery Active , which is the overall meta issue to finish things around attribute discovery on whether or not we want to keep support for·that or not. If not then we'll remove those broken cases once we drop annotation support entirely and we'll need document that in the documetnation and change records.

Re-reviewed patch and no further nits, this is critical for 10.2.0-beta1 to avoid breaking contrib. Let's continue improvements in followup issues under the parent meta.

Committed and pushed 99487df88e to 11.x and 315d371b48 to 10.2.x. Thanks!

There should be a documented best pattern out of this.

And yes, I was forgetting most of my overrides are not in contrib.

I don't see any solution from this issue about how Commerce is going to this issue by providing conditional functionality when a new module is installed.

Added a note to 📌 Document attribute based discovery in the handbook Active as we have yet to complete documentation for plugin attributes.

Thank you code fairies in the night

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

Berdir → closed merge request !5336