@godotislate

I wonder if there's a way to prevent or discourage people from implementing hook_entity_duplicate_create() as a procedural hook. (Not just this hook, but any new hook introduced by core at least.)

Or maybe something like this should/could be done with events instead of hooks?

Tweaked the service definition YML a bit and rebased. Not sure whether the log recorder should have more of an API with an interface defining methods, instead of expecting to work directly with state, but leaving as is for now.

There's still a missing space typo.

Rebased and refactored a little bit so that stuff that has to do with "providers" or modules is moved to the core AttributeClassDiscovery class instead of component.

There will be a merge conflict with 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active depending on which gets in first. Ideally #3490322 goes in first, but it is not a hard blocker.
Either way, it will be good to get both in to help with 📌 Convert MigrateSource plugin discovery to attributes Active and finally finish off moving core plugin types to attributes.

Splitting the header into separate lines with the same header name is going to be difficult, given that the Symfony Response object's headers object doesn't allow for multiple headers with the same key.

Also, if the goal here is only to fix headers for the debug cache tags, having the -1, -2 suffixes probably works just fine. If this issue also needs to address headers generally that are too long (such as the ones used by Purge), the current MR does not address that, and the question of whether and how to split headers into multiple lines with the same name is relevant.

I think the first thing to try is to have all the plugins provided by this module implement Drupal\views\Plugin\DependentWithRemovalPluginInterface and its onDependencyRemoval() method. It might be convenient to use the Drupal\Core\Plugin\PluginDependencyTrait as well. One example to look at is the Drupal\views\Plugin\views\field\EntityField plugin.

Basically you want onDependencyRemoval() to return TRUE when the BEF module is uninstalled, so that the plugin is removed and hopefully prevents the whole config entity from being deleted.

Not sure whether this by itself is sufficient, since the views handlers/plugins ecosystem is complex, but it's probably a good place to start.

Added a test to the MR per #6/#7. The tricky part was that the entity storage exception actually is caught by the module installer and logged. Then the workspace field ends up getting created correctly later anyway. Had to create a logger that would re-throw the exception.

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

Created separate issue about strange behavior with the browser based tests not running as expected: 🐛 Functional Javascript test false postive and missing browser output Active

godotislate → created an issue.

LGTM. Nice work, @kim.pepper. Thanks for sticking with this.

Tracked down that the :is selectors are coming from the postcss-nesting plugin: https://github.com/csstools/postcss-plugins/tree/main/plugins/postcss-nesting.

With the update, the default edition for the plugin becomes 2024-02 which is documented with:

usage of :is() pseudo-class is no longer optional

Setting the edition to '2021' does prevent the :is selectors from being added and can be done with this change to core/scripts/css/compile.js:

diff --git a/core/scripts/css/compile.js b/core/scripts/css/compile.js
index 5c9426747e8..3064bbbae38 100644
--- a/core/scripts/css/compile.js
+++ b/core/scripts/css/compile.js
@@ -33,6 +33,7 @@ module.exports = (filePath, callback) => {
           'has-pseudo-class': false,
           'image-set-function': false,
           'prefers-color-scheme-query': false,
+          'nesting-rules': { edition: '2021'},
         }
       }),
       postcssPixelsToRem({

I don't know enough about the CSS nesting standard though to say whether that should be done.

Apologies, seeing a couple new things with fresh eyes that I missed last pass:

• One more use of in_array and array_search can be reduced to array_search once
• Discovered in 📌 Memory leak in DrupalKernel when installing modules Active that #2066993: Use \Drupal consistently in tests → should be done, so can get ahead of it here with using \Drupal::service() instead of $this->container->get in test classes

A couple comments.

RTBC + 1. The the assert in the latest commit confirms there user, route, and query params cache contexts are not part of the cache key.

godotislate → changed the visibility of the branch 3493671-test-only to hidden.

Added a class to the layout builder element and updated the CSS selector to hide the border.

Not sure what's going on with the test. I think there might be some difference between my local env and Gitlab CI for browser-based tests. I'll look into that later when I have to time to revisit.

While testing it manually, noticed that a new blue rectangle appears on the Navigation Blocks page.

Oh, interesting. With the LB element moved outside the form, I assume it's picked up some sort of border CSS rule. Will investigate.

Put up MR 10539, which is ready for review. I'm not sure what's going on with the add tests, since the test-only job passes when it shouldn't. So I also pushed up a Test-only MR, which was failing in the wrong place. Meanwhile the test was consistently failing/passing as expected locally.

godotislate → created an issue.

Sounds good. Are you planning to write that test in this MR or do it in a separate issue?

Looks like all my comments were addressed. One test failed, but it seems a random know failure that's unrelated, and the test likely just needs re-running.

One remaining question is whether there needs to be test coverage that the navigation is being cached correctly?

A couple comments on the MR.

I think that introducing (or re-introducing?) 5 different attributes here to control hook order makes the scope of the issue quite large, at least the testing aspect of it.

But before that, I think it'd be helpful to define what the expectation is of how ordering should (or will?) work. I understand the goal is to replace hook_module_implements_alter in a way that works with both procedural and OOP hooks, but I think a full example with the variations of hook implementations and how exactly they can be ordered in a business sense is something missing from the IS.

Given that we can't know what existing service providers might want to do, I'm not sure how we can do this without breaking backward compatibility.

Yeah, aside from checking whether a module's installed, or some state value or config value, it occurs to me that people could be doing things like a custom environment detection service and altering a defined service based on what environment it is.

Core services probably need to remain manually wired, because determining which services would never or almost never be instantiated in a module's ServiceProvider is arbitrary guesswork, other than the event subscriber example, and even then, you never know.

Removing the blocker tag because it looks like ✨ Allow defining injected services with PHP attributes Postponed was closed.

I investigated #48-50 some more. Basically, there's a reason that the AutowirePass happens after a bunch of other passes in the "optimization" phase, never mind a few more in the "before optimization" phase, in Symfony\Component\DependencyInjection\Compiler\PassConfig: a lot has to be in place first in order for the service constructor arguments to be autowired correctly. Since Drupal's ModifyServiceDefinitionsPass has to run near the very beginning, there's no way to autowire services before that without essentially running the bulk of the other passes. I think it'd work out to running almost all the passes, then running ModifyServiceDefinitionsPass, and finally running a lot of the same passes again. And even then, I'm not sure that's possible without a side effect or error.

Technically this doesn't just affect core services. If $container->get('my_service') is run in any module's ServiceProvider class, if my_service is defined as autowired with no arguments, the service will fail to load. This is probably mitigated, because the most common use case is getting the module handler and adding or altering a service based on whether another module is installed. So it's less likely that many other services, except for maybe state or config.factory, would be instantiated in module ServiceProviders.

Anyway, I'm not sure how to move forward with autowiring core services, because the use of ->get() means that the whole dependency chain of services for the service that is being instantiated can not be autowired.

Here's example output of the error during a test run:

1) Drupal\Tests\system\Functional\UpdateSystem\RebuildScriptTest::testRebuild
ArgumentCountError: Too few arguments to function Drupal\Core\Cache\DatabaseBackendFactory::__construct(), 0 passed and exactly 5 expected

/var/www/html/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php:42
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:1190
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:626
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:571
/var/www/html/core/lib/Drupal/Core/Cache/CacheFactory.php:110
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:1171
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:626
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:1308
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:1260
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:1160
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:626
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:571
/var/www/html/core/modules/system/tests/modules/container_rebuild_test/src/ContainerRebuildTestServiceProvider.php:16
/var/www/html/core/lib/Drupal/Core/DependencyInjection/Compiler/ModifyServiceDefinitionsPass.php:30
/var/www/html/vendor/symfony/dependency-injection/Compiler/Compiler.php:73
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:814
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:1380
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:899
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:821
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:722
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:320
/var/www/html/core/lib/Drupal/Core/Extension/ModuleInstaller.php:229
/var/www/html/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php:83
/var/www/html/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:500
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:555
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:367

Rebased and addressed comments on the MR. I also moved all the "providers" logic to the core AttributeClassDiscovery class, since that's where it should happen.

I'm continuing with the hybrid approach (option 2) for now. Hopefully can get this pushed forward and help get 📌 Convert MigrateSource plugin discovery to attributes Active unblocked. It should be fairly easy to remove the *_exists() checks and rely solely on the Dependencies attribute if requested to go with option 1. I think option 3 of going through all the class hierarchies is too slow.

Along those lines, if nickic/php-parser is adopted here, and since it looks like BC support for plugin discovery by Annotations will continue for a while, plugin classes will be essentially parsed three times during discovery: once via the Doctrine-based StaticReflectionParser, then with nikic PhpParser, and then with Reflection. I think a reasonable follow up would be to replace or refactor StaticReflectionParser with PhpParser, and then somehow pass the parsed result into parseClass(), so that would remove one parsing pass. An eventual end goal could be parsing plugin Attributes with PhpParser as well, but as mentioned before, there is a lot of complexity in some of the Attribute classes.

Only gave a cursory investigation into https://github.com/Microsoft/tolerant-php-parser, which documents itself as "early-stage":

This is an early-stage PHP parser designed, from the beginning, for IDE usage scenarios (see Design Goals for more details). There is still a ton of work to be done, so at this point, this repo mostly serves as an experiment and the start of a conversation.

good to see the constant expression evaluator here but without the helpers described in "Unsupported expressions and evaluator fallback" under https://github.com/nikic/PHP-Parser/blob/master/doc/component/Constant_e... it is quite limited and I think these limitations need to be documented on the Dependencies::__construct $modules argument. Especially the lack of constants.

Added this documentation for the modules parameter in the Dependencies constructor.

Added Performance tag and related to 🌱 Reduce memory/cpu/io cost of attribute discovery Active .

I think all comments were addressed. RTBC +1

@liam morland: 📌 Remove use of deprecated "spaceless" filter in core templates Active was split from this issue per request in #74 📌 Twig Filter "spaceless" is deprecated Active .

I think you missed core/modules/menu_link_content/src/Plugin/migrate/source/d7/MenuLinkTranslation.php, so NW for that.

Thanks for catching that. Rebased the MR and added the change.

Can we really make the existing trait a wrapper for the new trait in the migrate_drupal module and not add any deprecation notice?

Not sure what the appropriate deprecation message, given #29 🐛 Move content_translation I18nQueryTrait to migrate module Active and whatever is going to happen to migrate_drupal, but I think I agree with the idea that the deprecation message should be set when migrate drupal moves out of core.

Can we turn these STR into an automated test? I think so. I am adding the tag for tests.

Unfortunately, this is more difficult than you might think, because all classes are autoloaded when tests run. So the trait would never be missing, unless it just doesn't exist at all.

I did run the steps to reproduce with the Entity Import module. I had to

• Use the lenient composer plugin
• Edit entity_import.info.yml
• Add an inherited method return type.

I was able to install entity_import against 11.x and this MR's branch. Confirmed that the fatal error was reproduced on 11.x and gone after the trait was moved.

Moving back to Needs Review. Leaving at needs tests in case there is another idea of how to test this proposed.

And/or use statements could easy be parsed for non-existent modules.

Migrate source discovery by annotations does this.

Adding a correction that migrate source discovery does not quite do this. It does the recursion, but it checks the namespaces of only parent and ancestor classes and does not look at namespaces of traits or interfaces.

Both MRs lgtm.

For the core services in scope to be autowired here, only a few were excepted from the conversion test and their definitions kept their arguments. Tests are green this way. 48 - 50 calls out the exception, and it's really a question of do we keep the exception in place and punt resolving 48 - 50 to one of the follow ups where we deal with more complicated autowiring conversions, or do we deal with it now. Also, a solution to how to deal with 48 - 50 does not currently exist.

Why do we have to move the four source plugins? Isn't it enough to move the trait to migrate_drupal?

Investigated this and found that an exception is thrown for missing DrupalSqlBase which prevents the fatal error, if only the trait is moved. That work is in MR 10426 for 🐛 Move content_translation I18nQueryTrait to migrate module Active and is in Needs Revew.

lgtm

One small comment that isn't critical, but would be nice. Otherwise RTBC.

godotislate → changed the visibility of the branch 3258581-to-migrate-drupal to hidden.

Problem/Motivation

Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait is also used in multiple migrate source plugins of other modules (e.g. Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation). This may make sense as translations usually require to have content_translation enabled. However it prevents e.g. the type checking of such plugins without instantiating them, as done in https://www.drupal.org/project/entity_import → , Drupal\entity_import\EntityImportSourceManager::getDefinitions().

Steps to reproduce

• Install Drupal standard installation profile in English, make sure content_translation is not enabled
• Install migrate_drupal module
• Install https://www.drupal.org/project/entity_import →
• Login in and go to /admin/config/system/entity-importer/add (Content > Importers > entity importer creation page)

Instead of displaying a form to add an entity importer, the following fatal error occurs:

Trait 'Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait' not found in /var/www/html/web/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php on line 22

Proposed resolution

I18nQueryTrait is used for source plugins that require migrate_drupal. So, move it to migrate_drupal. This is on MR 10426

User interface changes

None.

API changes

None.

Data model changes

None.

I don't feel strongly about this, but I thought I'd mention it: what about using container parameters instead of the sibling file approach?

So for my_module, in my_module.services.yml:

parameters:
  my_module.hooks_converted: true

In HookCollectorPass, the $container object would need to be passed down to the relevant method, but the logic would look something like
$skip_procedural = $container->hasParameter(''$module.hooks_converted");

There are two open MRs. Can you clarify which one is for review and hide the other one?

Updated the IS. Removed references to MR 8070, because solutions to hitting fatal errors during plugin discovery because of missing traits is being explored in 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active .

Added 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active as related issue - trying to handle missing trait dependency for all plugin discovery. It might make sense to postpone this issue on that one?

Also added ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active because it would provided a way to address the source_module property.

Surfacing a comment I made in 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active about missing dependencies that I think warrants mention here outside the specific context of plugin discovery:

That being said, I think the scope of the missing dependencies issue will expand as attribute-based discovery expands to routes ( 📌 Use PHP attributes for route discovery Needs review ), and services ( ✨ Directory based automatic service creation Needs review , 📌 Support #[AsEventListener] attribute on classes and methods Active , ✨ Support hooks (Hook attribute) in components Active ), etc. It might actually make sense to provide some sort of missing-dependency-safe wrapper for Reflection to address all these use cases.

Whole hog. Make DrupalKernel::$classLoader public readonly, use $kernel->classLoader->findFile() to find any classes/traits/interfaces being depended on, read 'em, parse 'em and recurse with a static cache.
...
And/or use statements could easy be parsed for non-existent modules.

Migrate source discovery by annotations does this. Trying to provide BC for this annotated discovery specifically for migrate source, on top of adding attribute discovery, results in some complex code. And recreating this functionality, again only for migrate source, makes the code even more complex. Handling recursive dependencies in the attribute discovery base class would eliminate the specific complexity for migrate source. In fact, an issue about moving this functionality from migrate source to core was created, even before attribute-based discovery was a thing, but it's been unresolved for years: https://www.drupal.org/project/drupal/issues/2786355 → .

That being said, I think the scope of the missing dependencies issue will expand as attribute-based discovery expands to routes ( 📌 Use PHP attributes for route discovery Needs review ), and services ( ✨ Directory based automatic service creation Needs review , 📌 Support #[AsEventListener] attribute on classes and methods Active , ✨ Support hooks (Hook attribute) in components Active ), etc. It might actually make sense to provide some sort of missing-dependency-safe wrapper for Reflection to address all these use cases. Recursively iterating through all the files of all the classes in the hierarchies might be really slow, so that could eliminate it as an option.

Thanks for the review, @ghost of drupal past. I'll address the MR comments and update it sometime tomorrow or later, but here's a couple quick replies to #6 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active

I think these limitations need to be documented on the Dependencies::__construct $modules argument. Especially the lack of constants.

This is a fair point, and I will add the documentation.

A depends on B depends on C where C doesn't exist , this code parses A and runs class_exists(B) which autoloads and happily throws a Fatal error: Uncaught Error: Class "C" not found.

Yeah, I tried to acknowledge that in this comment, but it's probably a little opaque:

  // There is a risk that a class or trait uses a missing trait somewhere
  // in its hierarchy and will cause a fatal error.

Let me give some background first. My initial idea was only to introduce the Dependencies attribute, and leave it to plugin developers to purposely designate which modules the plugin depends on, in order to avoid the fatal error on missing traits. While it is an additional DX hurdle, my thought was that this amount of friction is relatively small. It also really only applies to classes using missing traits, since we are already catching the exception for missing interfaces and classes.

While I was working with nikic/php-parser on this, I realized that the interfaces, classes, and trait dependencies were parsed and available, so I thought it was worth identifying those specifically as dependencies as well. Running the _exists() functions does run the risk of some trait in the class hierarchy being missing and causing the fatal. The chance of this happening is lower than a missing trait being used directly by the plugin class, and even in case there is a fatal error, that's when a plugin developer would use the Dependencies attribute.

There is a gap if a missing dependency in the hierarchy comes from some PHP library that is not a module, but I think this is a fringe use case.

Created 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active to explore using nikic/php-parser to help with plugin discovery per #12 📌 Investigate possibilities to parse attributes without reflection Active and put up an MR.
Hat tip to @ghost of drupal past for the library suggestion and gist.

MR is up with tests.

The Dependencies attribute stuff may not be strictly needed, but I think it can help make plugin discovery more robust. If it ends up staying in, there will need to be a CR documenting its use.

godotislate → created an issue.

Which means:
- That compiler pass is already in place, iterating over all services, and no nightmare happened.

It only iterates with reflection if attributes are registered for autoconfiguration via $container->registerAttributeForAutoconfiguration. Right now there aren't any in Drupal core, so this reflection iteration does not occur.

One thing to consider besides performance issues, a problem that's come up in 📌 Convert MigrateSource plugin discovery to attributes Active and 🐛 Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency Needs review is that attributes can't be read via Reflection on classes that implement missing interfaces, extend missing classes, or use missing traits, in the case that the missing dependency comes from an uninstalled module. While Reflection will throw an exception for a missing interface or class, PHP will fatal on a missing trait. The exception thrown during plugin discovery was handled in 🐛 Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency Needs review , but a solution for handling missing traits gracefully is still outstanding.

Using something like nikic/php-parser to retrieve attribute values without might be a viable way forward to retrieve at least some attribute values in the case of missing dependencies.

The $container->registerAttributeForAutoconfiguration is directly copied from Symfony Framework bundle: https://github.com/symfony/symfony/blob/e128d76b3dfd5147fe1263a807e8ce83..., but it effectively just does the reflection looping that is the performance issue in question.

it's just a performance/memory nightmare as you'd need to reflect every class and method defining a service to see whether they have a #Hook attribute

Yeah, forgot to mention that the MRs for 📌 Support #[AsEventListener] attribute on classes and methods Active need to be rebased, but I was holding off on that until 📌 Add a filecache-backed attribute parser Active goes in.

@rodrigoaguilera My understanding is that having multiple implementations of the same hook in a module makes re-ordering hooks via hook_module_implements_alter() convoluted, so there will be only 1 hook per module allowed until the whole procedural hook BC layer is removed in the future.

As for this issue: I think the need can be addressed by something similar to WIP MRs in 📌 Support #[AsEventListener] attribute on classes and methods Active . If we do the following, then registered services that have the #[Hook] and #[AsEventListener] attributes should get registered correctly as event listeners.

• Make the Hook attribute a subclass of AsEventListener
• Add something like this:
  

      $container->registerAttributeForAutoconfiguration(AsEventListener::class, static function (
        ChildDefinition $definition,
        AsEventListener $attribute,
        \ReflectionClass|\ReflectionMethod $reflector,
      ) {
        $tagAttributes = get_object_vars($attribute);
        if ($reflector instanceof \ReflectionMethod) {
          if (isset($tagAttributes['method'])) {
            throw new LogicException(sprintf('AsEventListener attribute cannot declare a method on "%s::%s()".', $reflector->class, $reflector->name));
          }
          $tagAttributes['method'] = $reflector->getName();
        }
        $definition->addTag('kernel.event_listener', $tagAttributes);
      });
  

There is the risk that someone registers a Hook service in their .services.yml, and the class for the service is in the Hook namespace, that the listener would get added twice and fire twice per event, but it can be documented that services in the Hook namespace should not be added to services.yml, and services outside that namespace should.

TY to @ghost of drupal past for the review. Replied to comments on the MR. I resolved the open threads, but please feel free to unresolve if they were not addressed satisfactorily.

MR is up with tests.

This RTBC +1 from me. There are outstanding comments from @longwave that @berdir replied to, and I agree those can be pushed to a follow up, but I'll defer as to whether those have been satisfactorily addressed to him.

Couple comments on the MR.

I don't think 🐛 Can only intentionally re-render an entity with references 20 times Needs work is a duplicate, but the two issues are closely related. I did notice that _referringItem would get overwritten while working on 🐛 Can only intentionally re-render an entity with references 20 times Needs work , and cloning the entity so that _referringItem is correct might actually help address recursion detection. Though I think in the (edge?) case where you are previewing new entities, cloning might make recursion detection more difficult, but I don't think that's a blocker here.

Note that this would need a core minimum of 10.2.

godotislate → created an issue.

Tests are green, all comments resolved, back to RTBC.

There are a couple unresolved comments on the MR from @catch.

lgtm

lgtm

How about this? In theme.inc, add

// {Add comment documenting why this is here}.
const THEME_INC_INCLUDED = TRUE;

Then do if (!defined('THEME_INC_INCLUDED')) {.

Some comments.

A couple small comments.

Made a revision to the MR. Instead of using provider, added moduleDependencies to the PluginProperty class, so that multiple modules can be set as requirements for the property to be added to the plugin definition.

godotislate → changed the visibility of the branch 3443882-allow-supplemental-plugin-attributes to hidden.

I've closed PoC MR 10179. Taking learnings from that MR and reducing scope to remove most of the entity type definition subclasses, I've opened MR 10218. I created a follow up for the entity type property attributes: 📌 Make it possible to define entity types with multiple smaller attributes Active

I also removed checking whether existing properties can be overridden. If there is a use case for making sure properties aren't overridden, that functionality can be reinstated, but it complicates logic when setting values with nested keys.

I've also added additional tests and updated the IS. I believe this is ready for review.

godotislate → created an issue.

godotislate → changed the visibility of the branch 3443882-plugin-option-attribute to hidden.

Gave it some thought, and refactored my PoC MR 10179.

My new approach to deal with the file cache: It won't be allowed to use attribute classes from modules. Only the base extender attribute (which I've bikeshedded myself again to be name PluginProperty) and some subclasses that all live in \Drupal\Core can modify plugin definitions. In order to annotate a plugin with PluginProperty attributes that such apply only when a module is installed, the provider property of the PluginProperty attribute must be set to name of the module dependency.

Attribute class discovery is changed so that the definitions parsed from Plugin attributes and altered by core provider PluginProperty attributes are saved in the file cache. PluginProperty attributes that have a provider other than core are saved in the "third_party_properties" in the array returned by parseClass and saved to file cache. The third party attributes are applied after retrieval from file cache if their module provider is installed.

I included examples of PluginProperty subclasses and applied them to Node, which looks like this:

#[StorageHandler(storageClass: NodeStorage::class)]
#[Handler(type: 'storage_schema', value: NodeStorageSchema::class)]
#[Handler(type: 'view_builder', value: NodeViewBuilder::class, allowOverride: TRUE)]
#[Handler(type: 'access', value: NodeAccessControlHandler::class, allowOverride: TRUE)]
#[Handler(type: 'views_data', value: NodeViewsData::class)]
#[FormClass(operation: 'default', formClass: NodeForm::class)]
#[FormClass(operation: 'delete', formClass: NodeDeleteForm::class)]
#[FormClass(operation: 'edit', formClass: NodeForm::class)]
#[FormClass(operation: 'delete-multiple-confirm', formClass: DeleteMultiple::class)]
#[Handler(type: 'route_provider', value: ['html' => NodeRouteProvider::class])]
#[Handler(type: 'list_builder', value: NodeListBuilder::class)]
#[Handler(type: 'translation', value: NodeTranslationHandler::class)]
#[Property(
  key: 'field_ui_base_route',
  value: 'entity.node_type.edit_form',
  provider: 'field_ui',
)]

MR is still a PoC, so there's some clean up, documentation, and more tests to be included.

longwave → credited godotislate → .

Draft MR 10179 → opened per #26 ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active . I think the Nightwatch test failure has to do with field UI as mentioned, but I haven't confirmed.

I used attribute classes for Handler/StorageHandler/FormHandler for entity types, but I forgot that those plugin definitions aren't arrays, so they aren't a good example for how the base class works with array definitions, but I think it still shows how the attributes would work overall.

I think it's worth investigating how much overlap there is in a use case like splitting up the entity type attributes and the use case of having other modules provide supplemental attributes.

A few thoughts:

• If we're providing "optional" properties, maybe the base attribute class name can be "PluginOption" instead of "PluginExtender." But this is a bikesheddy nit
• ::get() called on a Plugin attribute does not necessarily return an array; it can also return an object. See CKEditor5Plugin for an example. In which case, we can't assume that the $content variable in AttributeClassDiscovery::parseClass() is an array that new properties can just be appended to. I think we can solve this with a method on the base option/extender class like addToDefinition(array|object &$definition). The base method can be gated to only work an array definition, leaving child classes that work with plugin attributes that return objects to override that on their own
• I think it's OK to allow the option attribute to overwrite an existing property value, even if it's not deprecated? We can provide guardrails if needed by adding a property on the option attribute like "allowOverride", defaulted to FALSE
• The option attribute should probably specify which plugin attribute classes it can work with

I'm pushing a PoC MR here that hopefully demonstrates what I'm talking about a little better.

One additional concern that is not yet accounted for:
Plugin definitions discovery is backed the FileCache stored in APCu. The FileCache is only invalidated by plugin class file modified time, so Drupal cache clears do not affect its contents. That means that installing or uninstalling a module will not invalidate the FileCache. In the example of Field UI, if entity type definitions are discovered with Field UI uninstalled, then that entity type definition is stored in FileCache without the field UI base route. Even after installing Field UI, FileCache data is not invalidated, so the field UI base route is not added to the entity type definition.

I'm not sure how to get around this without needing to invalidate the FileCache on every module install. This is also a very difficult thing to capture in automated tests, because all classes are autoloaded during tests regardless of whether their modules are installed.

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

Rebased baseline still looks good, leaving in RTBC.

Rebase in baseline looks fine, and tests are green.

Moving to RTBC per #77 📌 Twig Filter "spaceless" is deprecated Active .

@catch Follow up created: 📌 Confirm which ConfigEntityType attribute contructor arguments are needed Active .

godotislate → created an issue.

Upstream issue fixed. Rebased and tests are green again.

@catch, @larowlan can you clarify the follow up requests?

I'd love to see constants or enums for these (in a follow up), magic strings lead to typos

Is to use constants/enums for common form keys such "default" and "delete"?

I think I found some additional ConfigEntityType constructor arguments we don't need. We might want a follow-up to try to rationalise these anyway?

Is this to discuss restoring some properties that were removed?

Looks like after these changes on 11.x, PHPStan is failing the build: https://git.drupalcode.org/project/drupal/-/jobs/3330912.

------ ---------------------------------------------------------------------------- 
  Line   core/modules/language/tests/src/Traits/LanguageTestTrait.php (in            
         context of class Drupal\FunctionalTests\Entity\EntityUuidIdTest)            
 ------ ---------------------------------------------------------------------------- 
  65     Method                                                                      
         Drupal\FunctionalTests\Entity\EntityUuidIdTest::disableBundleTranslation()  
         has no return type specified.                                               
 ------ ---------------------------------------------------------------------------- 

Removed some unneed arguments from ConfigEntityType attribute constructor and rebased.

I believe all previous threads were addressed, but I can't resolve them because I didn't open the MR.

Will take a look at the new threads for the config entity attributes a bit later.

OK, agreed that if any more are found, they can be fixed in another issue. RTBC +1.