@godotislate

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.

On a standard profile install, hasTranslationChanges() detects changes on the entity without content translation enabled.

I have a suggestion for a simpler change that might work.

MR also needs tests.

The changes all seem fine, but how do we know there aren't other ones that are missing or incorrect?

Created a new draft MR 10119 for a PoC of the approach I mentioned in #28 📌 Support #[AsEventListener] attribute on classes and methods Active . It leverages a lot of what HookCollectorPass is already doing to also find module class with the AsEventListener attribute and registers them as event listener services. No entry in the module .services.yml is required. Note that discovery is limited to the EventSubscriber directory in a module. Also, these services are autowired, so any arguments in class constructors need to be able to be autowired or have the Autowire attribute applied to them.

Putting back to NR for feedback on the PoC MR. I can also move this issue back to RTBC if this new work is better done in a followup.

Rebased against latest 8.x-1.x.

Did some investigation on the bot failures: Looks like PHPStan is only running against files in the MR diff. It's configured to ignore *.api.php files, and if there are no files to scan, it's also configured to exit with an error, so the bot flags that as a failure. (For MR builds, PHPStan runs over all core files, so this situation doesn't come up.)

For the future, I think it makes sense to push this issue back to RTBC whenever the bot sets it to NW, unless there really is a merge conflict to resolve. Hopefully it gets merged soon.

Changes look good on all 6 MRs (11.x, 11.1.x, 11.0.x, 10.5.x, 10.4.x, 10.3.x).

Updated IS, created follow up to remove spaceless usage in core ( 📌 Remove use of deprecated "spaceless" filter in core templates Active ), and added a test that the spaceless filter still works as expected.

Copied commits from MR 9665 from 📌 Twig Filter "spaceless" is deprecated Active to MR here, so hopefully the appropriate credits will be captured.

godotislate → changed the visibility of the branch 3477375-spaceless to hidden.

godotislate → created an issue.

rtbc +1

Test failure is probably unrelated and needs a re-run. Fine to go to RTBC after test passes.

1 small nit and a question.

It occurs to me that since 📌 OOP hooks using attributes and event dispatcher Needs review introduced a container compiler pass that essentially iterates through module files to find classes or methods with the Drupal\Core\Hook\Attribute\Hook attribute, and turns those into event listener services, if we:

1. Made the Hook attribute a subclass of the Symfony\Component\EventDispatcher\Attribute\AsEventListener attribute class
2. Changed the compiler pass to look for the AsEventListener attribute,

we could have those general event listener services discovered in the same pass. And those event listeners wouldn't need to be declared in services.yml files.

This wouldn't work for core event subscribers outside of modules, but it might be worth looking into?

lgtm

Nits: I think the convention is document as {class name}::{method name} instead of {class name}->{method name}.

Back to RTBC.

Rebased. Put it back to RTBC since that was where it was before the bot detected the merge conflict.

Changes in MR and CR look good to me. I agree that what to do with RenderCallbackInterface or ElementInterface should be handled in a follow up.

Test failure looks to be unrelated, so I think it just needs to be re-run. Once it passes, I think it's good to be moved to RTBC.

lgtm

My original suggestion from 🐛 module_implements_alter is not supported by OOP hooks Active was incorrect. Change should be @return static.

Nits/questions about which CR https://www.drupal.org/node/3355686 → or https://www.drupal.org/node/3349470 → should be used in deprecation messages. Looks good otherwise.

Rebased and fixed the return type. Also, switched to using PHP 8 null-safe operator in Tables.php to reduce diff.

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

This was moved to Fixed in #19 🐛 module_implements_alter is not supported by OOP hooks Active , but somehow it's still showing as RTBC?

This looks fine, so RTBC.

I do have two small out of scope questions:

• I don't see any test coverage for the existing deny list functionality. If there is actually isn't any, was this an intentional choice as something to be done later?
• The docblock return type hint for HookCollectorPass::collectAllHookImplementations(): @return \Drupal\Core\Extension\HookCollectorPass has the wrong namespace. It probably should be @return $this. Is it worth worth its own issue?

Added a comment about how the parsing of the pairs might need to handle the case where there is a mismatch between number of classes/methods.

Also, has it been considered that instead of changing the string argument $class_and_method into two string arguments $class and $method, that $class_and_method changed to be an array? And then `$others` should be an array of arrays. With each array being validated as having two items.

So it'd look like this:

$method = 'formFilterFormatFormAlter';
    HookOrder::after($container, 'form_filter_format_form_alter',
      [Ckeditor5Hooks::class, $method],
      [EditorHooks::class, $method],
      [MediaHooks::class, $method]);

IMO, it makes it a little nicer DX to be able to keep track of the pairs.

Rebased.

lgtm

Couple small comments.

smustgrave → credited godotislate → .

lgtm

I did some manually testing. On latest 11.x, after following the reproduction steps in #4 🐛 Nested paragraphs with same field name are ordered wrong Needs work to create the paragraphs and fields, I created Article content with 3 Parent Paragraphs (Parent 1, Parent 2, and Parent 3), and each Parent Paragraph with 2 children (Parent 1 => Child 1, Child 2; Parent 2 => Child 3, Child4; Parent 3 => Child 5, Child 6). I then dragged Parent 3 to go above Parent 2 (See screenshot → ). After saving, Parent 2 incorrectly remained above Parent 3 ( See screenshot → ).

I then applied the changes from MR 9897. Re-edited the page and dragged Parent 3 above Parent 2 again ( see screenshot → ). After save, Parent 3 stays correctly above Parent 2 ( See screenshot → ).

I tried to reproduce the issue mentioned in #8 🐛 Nested paragraphs with same field name are ordered wrong Needs work with block content and nested paragraphs in Layout Builder, but I did not see the console errors.

The automated test that's needed is a functional test that does similar to steps to the manual test just described. But I'm not sure an automated test can be written using core alone, at least not easily. Given that and the fact that the fix is essentially a one-line diff, I wonder if it qualifies under 🌱 [policy, no patch] Better scoping for bug fix test coverage RTBC as something that can be committed without this test. Moving to RTBC for confirmation.

I think the problem is there might be a bug with the PHPStan config in the NR bot, so it will kick this issue back to NW on the next upstream change. But we'll see.

We figured out how to remove the \

Nice! I think the outstanding question is whether the multiline arrays going away is fine.

I think we'll need more opinion/consensus on which code style issues are fine to ignore for now. I'm personally fine with globally-namespaced functions (like \t()), but I think \NULL and \TRUE look really weird and unnatural. Not sure how to balance that with arrays not being multiline.

back to rtbc

Some change upstream must have affected the PHPStan check, because the bot failed on this:

----------------------------------------------------------------------------------------------------

Running PHPStan on changed files.


 [ERROR] No files found to analyse.



PHPStan: failed

----------------------------------------------------------------------------------------------------

Some comments on the MR, mostly nits.

lgtm

Should the effort to replace all usage of spaceless in core templates (MR 9665) and the effort to suppress the spaceless filter deprecation warning + add drupal_spaceless filter (MR 9735) be combined into one MR?

Also, should Drupal have its own deprecation message for spaceless usage, with a recommendation to change to drupal_spaceless?

Could this be reopended as it is still an issue?

This issue has been closed for almost a year and a half. The best way forward is likely to create a follow-up/child issue noting that the height calculation is incorrect for CKEditor instances not in viewport on page load.

Rebased and applied suggestions from #30 🐛 Form state storage changes in #process callbacks are not cached during form rebuild. Needs review . Leaving in Needs Review for subsystem maintainer review.

Re #37 🐛 hook_requirements() doesn't say that severity is optional, or what the default is RTBC :

Actually, I am mistaken. A change is needed here. The default value is REQUIREMENT_INFO, see

#665790-291: Redesign the status report page
https://git.drupalcode.org/project/drupal/-/blame/11.x/core/modules/syst...

The linked code still matches what was mentioned in #2 🐛 hook_requirements() doesn't say that severity is optional, or what the default is RTBC , and should probably be documented as such:

      $severity = $severities[REQUIREMENT_INFO];
      if (isset($requirement['severity'])) {
        $severity = $severities[(int) $requirement['severity']];
      }
      elseif (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'install') {
        $severity = $severities[REQUIREMENT_OK];
      }

Which sounds like undefined severities default to REQUIREMENT_OK while installing, and REQUIREMENT_INFO otherwise.

This should indeed be documented explicitly.

godotislate → changed the visibility of the branch 2311679-separate-mime-type-fileeye to hidden.

godotislate → changed the visibility of the branch 2311679-separate-mime-type-11.x to active.

godotislate → changed the visibility of the branch 2311679-separate-mime-type-11.x to hidden.

lgtm

Just noting here that ✨ Make email not required for a Drupal site account Active is a mostly opposite effort. As to a use case where emails would not be required on registration, see #77 ✨ Make email not required for a Drupal site account Active :

Also here via search, also building a site involving user accounts for people of all ages, including very young, for whom an email address is inappropriate. It'd be great if this wasn't necessarily a required property/base field.

Revisiting the idea of replacing the spaceless filter: It happens that if a Twig extension is added to the environment that has a filter with the same name as a filter in an extension already added to the environment, then the filter definition in the extension added last takes precedence. I've added a commit to MR 9735 that adds a SpacelessBCExtension class after Twig's CoreExtension is added to the Twig Environment. The spaceless filter is currently defined in SpacelessBCExtension without deprecation warnings, and all tests now pass. Could also change it to issue a different deprecation message to use drupal_spaceless instead by Drupal 12.

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

One small deprecation docblock issue with version numbers.

Added commit per #23 to ignore classes with '\\EventSubscriber\\' in the path.

The RegisterListenersPass already occurs within Drupal's Drupal\Core\DependencyInjection\Compiler\RegisterEventSubscribersPass

That pass doesn't handle the #[AsEventListener] attribute, though. That is what this MR introduces, and the code to do so is copied from here: https://github.com/symfony/symfony/blob/9d89e05279631430f9037844fc4cc2ea...

So even if or when ✨ Directory based automatic service creation Needs review gets in, the work here still needs to be done to support #[AsEventListener].

Also see: https://stackoverflow.com/a/78338989

Automatic discovery of services without entry in a services.yml needs something like ✨ Directory based automatic service creation Needs review . There's no discovery of services in this MR, and it's probably out of scope for this issue.

Since there are multiple handlers in MaintenanceModeSubscriber, either each method needs to have an attribute as done in the MR, or there needs to be multiple AsEventListener attributes on the class, each including the method property.

I tried adding an additional AutowirePass right before ModifyServiceDefinitionsPass in CoreServiceProvider:

$container->addCompilerPass(new AutowireRequiredMethodsPass());
$container->addCompilerPass(new AutowireRequiredPropertiesPass());
$container->addCompilerPass(new AutowirePass(false));
$container->addCompilerPass(new ModifyServiceDefinitionsPass());

This led to a circular reference exception when running AutowireTest, and I did not investigate further:

1) Drupal\KernelTests\Core\DependencyInjection\AutowireTest::testAutowire
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "http_kernel", path: "http_kernel -> http_middleware.ajax_page_state -> http_middleware.negotiation -> http_middleware.reverse_proxy -> http_middleware.content_length -> http_middleware.kernel_pre_handle -> http_middleware.session -> http_kernel".

/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:67
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:70
/var/www/html/vendor/symfony/dependency-injection/Compiler/CheckCircularReferencesPass.php:43
/var/www/html/vendor/symfony/dependency-injection/Compiler/Compiler.php:73
/var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php:752
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:1376
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:899
/var/www/html/core/lib/Drupal/Core/DrupalKernel.php:505
/var/www/html/core/tests/Drupal/KernelTests/KernelTestBase.php:374
/var/www/html/core/tests/Drupal/KernelTests/KernelTestBase.php:253

One vague idea I had is to deprecate altering services via module service providers. Service decoration can handle the bulk of use cases for altering services. And if/when 🐛 Service decorates non-existant service when module not installed Needs work gets in, that should cover a lot of use cases of conditionally altering services.

Altering container parameters based on config like in LanguageServiceProvider might be something that be handled by custom compiler pass and tokenizing BootstrapConfigFactory::get() to use in YAML.

But I'm sure there are projects that do more complicated logic than that, so that might not cover enough use cases. It's also possible that projects implement ServiceProvider register() with conditional logic to add service definitions based on whether a module is installed, in which case just removing alter() is insufficient.

Rebased against lastest 11.x and got tests green.

One issue that's come up, kind of similar to what's mentioned in #41 📌 Use autowiring for core modules and services Needs work : If you have a service definition that is autowired with no arguments in the definition, or if any of its service dependencies' definitions are are autowired with no arguments, then trying to instantiate that service in a module ServiceProvder will result in error. Take these two example from test module service providers:

Drupal\container_rebuild_test\ContainerRebuildTestServiceProvider

  public function alter(ContainerBuilder $container) {
    $count = $container->get('state')->get('container_rebuild_test.count', 0);
    $container->get('state')->set('container_rebuild_test.count', ++$count);
  }

Drupal\service_provider_test\ServiceProviderTestServiceProvider 

  public function alter(ContainerBuilder $container) {
    ...
    // Make sure a cached service can be also called in a service provider.
    // https://www.drupal.org/project/drupal/issues/2363351
    /** @var \Drupal\Core\Extension\ModuleHandlerInterface $module_handler */
    $module_handler = $container->get('module_handler');
   ...

While the definitions for both state nor module_handler still have arguments, if arguments for cache.backend.database are removed, then the $container->get() call fails with an error that 0 parameters are being passed into the constructor for DatabaseBackendFactory.

This occurs because ModifyServiceDefinitionsPass compiler pass where the module ServiceProviders register and alter service definitions runs before the compiler passes to handle autowiring. Not sure how to handle this.

godotislate → changed the visibility of the branch 3295751-use-autowiring-for to hidden.

godotislate → changed the visibility of the branch 3295751-autowiring-11.x to hidden.

godotislate → changed the visibility of the branch 3295751-autowiring-simple-cases to active.

godotislate → changed the visibility of the branch 3295751-autowiring-simple-cases to hidden.

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

Rebased again.

Rebased.

Applied some suggested changes.

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

Rebased, back to RTBC.