@godotislate

Made docblock update for nr bot code sniff and rebased.

Rebased for merge conflict.

Refactored this to try to make the functionality available to be used outside plugin discovery.

smustgrave → credited godotislate → .

Rebased and updated the MR:

• Applied my suggestions for the null checks
• Removed bundle dependencies when entity permission granularity is at the entity type level
• The line $this->assertEquals(['entity_test.entity_test_mul_bundle.test'], $permissions['translate test entity_test_mul_with_bundle']['dependencies']['config']); when testing bundle granularity with bundle that is not config seems to be testing the wrong thing, so that has been replaced with $this->assertEquals(['language.content_settings.entity_test_mul.entity_test_mul'], $permissions['translate entity_test_mul entity_test_mul']['dependencies']['config']);
• Added test to confirm that when bundle translation is disabled, the permission is removed from the role

Is "Needs work" correct as current status, or is this ready to be reviewed again?

Addressed latest MR comments.

@catch Yes, that was part of my thinking of whether there'd need to be something like this for Hook discovery, or as attribute discovery may expand to services or routes, such as 📌 Use PHP attributes for route discovery Needs review , ✨ Directory based automatic service creation Needs review , 📌 Support #[AsEventListener] attribute on classes and methods Active , and ✨ Support hooks (Hook attribute) in components Active .

@godotislate there is 1 thread I wasn't 100% should be resolved but your comment in #10 made think it maybe could? Could you close it.

It's not my MR, so I can't close it. But the question was resolved.

Changes look good. Hook is no longer an alter hook, and looks like MR comments were addressed.

IS updated.

Are we sure we want to do this. Why did Twig remove the filter?

Per https://github.com/twigphp/Twig/pull/4236

Deprecate the spaceless filter for the following reasons:

• The performance is bad (as the work is done at runtime via a regexp)
• Optimizing the size of an HTML doc server side is "almost never" a good idea (compression is better and enough)
• There are some edge cases where you want to keep some spaces (see https://github.com/twigphp/Twig/issues/3576
• Controlling whitespace is possible and fine-grained via the dedicated Twig modifiers on {{ }}

If someone find it useful, re-creating it is trivial (return trim(preg_replace('/>\s+<', $content ?? ''));),
but with so many caveats and not so many use cases, I think it does not belong to core.

The rationale for keeping it is that it likely will be very disruptive for a lot of contrib/custom projects, some of which previously had to refactor their templates when Twig deprecated and removed the spaceless tag.

Why do we have both drupal_spaceless and spaceless? Seems unnecessary too introduce drupal_spaceless

Added commit to remove drupal_spaceless.

Noting here that recent test-only runs for the MR on 🐛 Placing a block containing a form in navigation layout prevents layout builder from saving Active having been failing as expected, such as https://git.drupalcode.org/issue/drupal-3493671/-/pipelines/391171. I'm not sure what's different, so it might be an intermittent issue.

A couple suggestions on the MR for missing NULL/empty checks, one of which is causing the majority of test failures.

The update hook is also loading role entities without checking that the role entity type exists, but it's only an issue if the user module isn't installed, which seems like a very unusual edge case.

Updated the log message and restored $this->permissions = $valid_permissions; which had been accidentally deleted. Also added an assertion that the saved role does not have the non-existent permissions.

When making the change, you'd need to make sure the functionality is not affected. If it isn't, then the test needs to be updated as well. There are two test failures, so you'd also need to make sure that the necessary changes can be made and are applied for both.

Addressed MR comments, updated tests, and re-based.

Looks to me that IS was updated, so removing "Needs issue summary update".

This is ready for review.

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

@arunsahijpal It may be helpful for you to make sure the MR build passes before setting to Needs Review. The build for the latest changes still fails.

See https://git.drupalcode.org/project/drupal/-/merge_requests/10785. You can review the build job log https://git.drupalcode.org/issue/drupal-3497021/-/jobs/3926956 to see why it is failing.

Looks like everything was addressed. PHP Unit Build job needs a re-run and we're probably there.

Resolved merge conflict and rebased. As mentioned in #13, core has test coverage to make sure spaces and special characters in source names work (see https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php?ref_type=heads#L644), so I don't think it's necessary to duplicate here.

Moving to NR.

Unnecessary test removed and rebased MR.

Did a bit of docblock clean up. Test only fails correctly still, so this is ready.

Code archaeology seems to show this is cruft from this commit 19 years ago, and how confirm forms originally worked: https://git.drupalcode.org/project/drupal/-/blob/198ec98f756673da8c899bb0236a91808ed010ec/includes/theme.inc#L898

Not sure if any of these might help with the investigation:

https://www.drupal.org/node/3453468 →

🐛 browser_output is empty when testing with Drupal 11 Active
📌 Bootstrap HtmlOutputLogger from phpunit.xml RTBC
📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Fixed

In general, I'm unsure about the Class suffixes. I think I'd prefer that thy all use handler somehow, so for example AccessHandler, not AccessClass.

Changed all the handlers from something like AccessClass to AccessHandler. Given that...

I think it's definitely worth adding a RouteProvider attribute as well, that greatly simplifies the syntax: #[RouteProvider('html', NodeRouteProvider::class)]

Added this as RouteProviderHandler, since now all the other handlers have that suffix. I do not feel strongly about this, though.

I also added a EntityKey (which I guess could just be "Key") and a Label attribute, because they are commonly used enough. Adding Label also provides headway for #2813895: Combine entity type labels in an array → , or at least, a start to a way to provide new types of labels more easily. (There'd still need to be an API change on how to get those labels.)

Along those lines, here are some pros to splitting up the entity type attributes like this:

• It is more schema-agnostic for developers working on their own entity type definitions (maybe somewhat mitigated by devs using drush generate)
• Can set arbitrary properties on entity type definitions (though entity types already have $additional)
• Typehinting for property values otherwise within nested arrays
• Each attribute can provide more documentation about its property, though the classes are currently only lightly documented since this is POC

For cons:

• As mentioned in other comments, it's not necessarily more readable
• It's more typing, unless an IDE helps out with that via tab completion or similar
• Conversion to use them on existing entity types would be a large effort (though this maybe this is optional?)

lgtm

FWIW, making sure the form_build_id hidden input value does not get retained by the browser was enough to fix the issue for me on Firefox. I put up a draft MR 10706 with that change.

Per conversation with catch on Slack, the overall goal is to change everything in tests over to key value, except for tests specifically testing state, because writing to key value will be cheaper than writing to state.

The references changed in the MR were ones specific to LayoutBuilderBlocksTest and everything that shared those references. I think it's just a question of how to scope changing over all the state usage across all the tests.

@bkosborne What browser did you test in?

For me testing locally on latest 11.x dev:

1. drush si standard -y
2. drush en media_library
3. Add image media field to basic page
4. Create basic page, upload media image, save
5. Edit basic page
6. Remove selected media

On Windows, Chrome 131.0.6778.205, when I reload, the media selection is restored on the page. When I click the remove button again, the media item is correctly removed.

Also on Windows, Firefox 133.0.3, I can reproduce the issue mentioned in the IS. However, if I revisit the page (while on the page, click in location bar and hit enter) instead of reloading, then the remove button works as expected.

It seems to be related to Firefox keeping form data on reload: https://stackoverflow.com/questions/7377301/firefox-keeps-form-data-on-r.... It's an old issue on stackoverflow, but I can confirm that if I add the autocomplete="off" attribute to the node form, e.g. $form['#attributes']['autocomplete'] = 'off';, then reloading on Firefox works the same as Chrome.

Per discussion on Slack, we can go forward with the state to keyvalue swap for the relevant block tests here. There should be a follow up though to investigate further about what is the cause of the intermittent failures state failures, if only to establish that it's a test-only condition.

The other random test failure issues linked in the meta 🌱 [meta] Known intermittent, random, and environment-specific test failures Active should be looked at as well to see whether there are usages of state that can be replaced with key value instead.

godotislate → changed the visibility of the branch 3477586-2500x-LayoutBuilderBlocksTest-testBlockPlaceholder-as-is to hidden.

godotislate → changed the visibility of the branch 3477586-2500x-LayoutBuilderBlocksTest-testBlockPlaceholder-replaced to hidden.

Tried re-running the test-only job, and it failed as expected! https://git.drupalcode.org/issue/drupal-3493671/-/jobs/3825109.

I'm not sure what's differe, but unpostponing and putting back to NR.

Thanks for the review, @plopesc. I made the suggested change as well as a similar one I forgot.

Not sure what's going on with the test. I created 🐛 Functional Javascript test false postive and missing browser output Active , because I have observed some unexpected behavior going on with the browser-based tests at least, but I've made little headway in identifying the cause. Postponing this issue on the resolving the tests.

One thing I have noticed is that on test-only job, this message shows twice: HTML output directory sites/simpletest/browser_output is not a writable directory.

Example: https://git.drupalcode.org/issue/drupal-3493671/-/jobs/3819911

benjifisher → credited godotislate → .

Pushed up draft MR 10689 with just the state -> keyvalue changes mentioned in #10. Only can report that the the build did not fail.

Refactored so that an event subscriber isn't necessary. #pre_render and #post_render callbacks can handle it. Tests pass, but still not getting the test only build to fail as expected in CI, though.

The only BC safe way would be some func_get_args() trickery.

I think this might make sense, along with a deprecation triggered. Is there also a way to annotate in the interface that the method parameters will change?

I'm not keen on special-casing some handler types over others. I'm not sure why EntityTypeInterface does that anyway!

I think someone in the other issue mentioned there'd be plenty of bikeshedding over which attributes to create and what they should be named, so might as well start now :).

I'm not that opinionated about this, though I will say it's nice to align with the set*() methods in that it leverages API as opposed to making assumptions about the definition schema. (The HandlerClass attribute is special cased because setHandlerClass doesn't deal with nested hander definitions.) On the other hand, with adequate test coverage, the point is largely moot.

Separately, I don't love how long "EntityTypeProperty" is, but "Property" on its own is useless as a search string.

With the attributes it's just one massive block of text.

I don't know if it looks better this way, either. Though it does look better in an IDE (or at least PHPStorm) than it does in Gitlab UI. And the attributes that have multiple arguments can be made to be multiline if preferred.

Changing the signature to get() to have two arguments or adding a "safe" method to FieldableEntityInterface would both introduce a BC break. I think it'd make more sense to do the former, if for no other reason that it doesn't require an additional change to the Twig allow list. (Though I imagine most Twig templates use the magic getter instead of get).

#157 📌 Use cache collector for state Fixed from 📌 Use cache collector for state Fixed might be relevant here:

however we're seeing other test failures in runs which aren't consistent. I think the issue is that any test using state to track.. well.. state between requests, now needs to either switch to WaitTerminateTestTrait or to use the key value service directly without the state wrapper - because the cache will be updated in terminate.

Looking through comments on 📌 Use cache collector for state Fixed , I think one thing to explore would be to replace all the uses of state with keyvalue when testing block content, such as all the lines that look like

• \Drupal::state()->set('block_test.content', ...
• \Drupal::state()->get('block_test.content')

and see if random failures for LayoutBuilderBlocksTest go away.

These lines could also be swapped for keyvalue:

• \Drupal::state()->set('block_test.attributes', ...
• \Drupal::state()->get('block_test.attributes')

Since it was suggested that this maybe could go forward independently of ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active , I pushed up MR 10682. This is based on PoC work I originally started in the other issue, but now with additional attributes for setting entity type definition properties. Essentially, there is an attribute that corresponds to every set*() method in EntityTypeInterface, since that seemed the most convenient. It is possible to add other attributes regardless of whether there is a specific set method, because we can just leverage set(). But for now, started with these attributes:

• AccessClass (setAccessClass())
• Constraints (setConstraints())
• EntityTypeProperty (set())
• FormClass (setFormClass())
• HandlerClass (setHandlerClass())
• LinkTemplate (setLinkTemplate())
• ListBuilderClass (setListBuilderClass())
• StorageClass (setStorageClass())
• UriCallback (setUriCallback())
• ViewBuilderClass (setViewBuilderClass())

Also made converted the Node and Comment entity classes to use the new attributes.

I was able to locally reproduce the circular service definition error in #11 📌 Replace lazy service proxy generation with service closures Active , but only once (the first time I tried).

I basically did the same the steps from the failed Validatable config build:

1. Against latest 11.x, drush si standard -y
2. ls core/modules | grep -v sdc | xargs vendor/bin/drush pm:install --yes
   I ignored an error about not being able to install package manager
3. Switch to MR branch, rebased against latest 11.x
4. drush cr

And I saw the same error:

Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "cron", path: "cron -> automated_cron.subscriber". in /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php on line 147 #0 /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php(430): Drupal\Component\DependencyInjection\Container->get('cron', 1)
#1 /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php(237): Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array)
#2 /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php(177): Drupal\Component\DependencyInjection\Container->createService(Array, 'automated_cron....')
#3 /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php(454): Drupal\Component\DependencyInjection\Container->get('automated_cron....', 1)
#4 /builds/issue/drupal-3483996/vendor/symfony/event-dispatcher/EventDispatcher.php(243): Drupal\Component\DependencyInjection\Container->Drupal\Component\DependencyInjection\{closure}()
#5 /builds/issue/drupal-3483996/vendor/symfony/event-dispatcher/EventDispatcher.php(206): Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}(Object(Symfony\Component\HttpKernel\Event\TerminateEvent), 'kernel.terminat...', Object(Symfony\Component\EventDispatcher\EventDispatcher))
#6 /builds/issue/drupal-3483996/vendor/symfony/event-dispatcher/EventDispatcher.php(56): Symfony\Component\EventDispatcher\EventDispatcher->callListeners(Array, 'kernel.terminat...', Object(Symfony\Component\HttpKernel\Event\TerminateEvent))
#7 /builds/issue/drupal-3483996/vendor/symfony/http-kernel/HttpKernel.php(114): Symfony\Component\EventDispatcher\EventDispatcher->dispatch(Object(Symfony\Component\HttpKernel\Event\TerminateEvent), 'kernel.terminat...')
#8 /builds/issue/drupal-3483996/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(63): Symfony\Component\HttpKernel\HttpKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Render\HtmlResponse))
#9 /builds/issue/drupal-3483996/core/lib/Drupal/Core/DrupalKernel.php(683): Drupal\Core\StackMiddleware\StackedHttpKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Render\HtmlResponse))
#10 /builds/issue/drupal-3483996/vendor/drush/drush/src/Boot/DrupalBoot8.php(312): Drupal\Core\DrupalKernel->terminate(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Render\HtmlResponse))
#11 [internal function]: Drush\Boot\DrupalBoot8->terminate()
#12 {main}
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "cron", path: "cron -> automated_cron.subscriber". in Drupal\Component\DependencyInjection\Container->get() (line 147 of /builds/issue/drupal-3483996/core/lib/Drupal/Component/DependencyInjection/Container.php).

However, running drush cr again right after succeeded with no errors.

I also tried repeating these steps, with and without running XDebug, and the error did not occur again.

I actually created 📌 Provide FieldableEntityInterface get() functionality that returns NULL instead of throwing exception Active before finding this issue, so closed that one as a dupe. There I proposed adding a second parameter to get() or a new get-like method that returns NULL instead of throwing an exception, to account for anything relying on the exception being thrown. So either

/**
   * Gets a field item list.
   *
   * @param string $field_name
   *   The name of the field to get; e.g., 'title' or 'name'.
   *
   * @param bool $throw_exception
   *   Whether to throw an exception if field name is invalid or return NULL.
   *
   * @return \Drupal\Core\Field\FieldItemListInterface|null
   *   The field item list, containing the field items.
   *
   * @throws \InvalidArgumentException
   *   If an invalid field name is given.
   */
  public function get($field_name, bool $throw_exception = TRUE);

or

/**
   * Gets a field item list safely.
   *
   * @param string $field_name
   *   The name of the field to get; e.g., 'title' or 'name'.
   *
   * @return \Drupal\Core\Field\FieldItemListInterface|null
   *   The field item list, containing the field items, or NULL if invalid field name.
   */
  public function safeGet(string $field_name): ?FieldItemListInterface {
    try {
      return $this->get($field_name);
    }
    catch (\InvalidArgumentException) {
      return NULL;
    }
  }

Turns out this is actually a dupe of 📌 Remove exception when accessing a non-existing field with ContentEntityInterface::get() Active .

Another consideration that came to mind is that if there's a new method, it should be allowed to be used in Twig.

godotislate → created an issue.

For a workaround until someone identifies the bug in Composer Deploy or there's consensus on a fix otherwise, I put up MR 10669 with the ProjectCoreCompatibility constructor change mentioned in #15. The diff can be downloaded and applied locally as a patch.

I put up MR 10668 with a POC of having a variadic property in the plugin attribute constructor. The EntityType attribute already has an readonly additional property that's used differently, so I couldn't use additional as the property name in the base Plugin attribute, and for now it's just $other. Also updated the Block attribute and added the allow_in_navigation property to the appropriate navigation block plugin classes per 📌 Provide a way for other modules to flag block plugin implementations as 'navigation safe' Active to demo this.

This approach works as far as I can tell, but I have reservations: setting arbitrary properties in the attribute is not self-documenting, so it's not immediately obvious where these properties come from or how they're used. Also, there would need to be some effort to update all the attribute constructors, and account for attributes that return arrays from get().

Since there's still desire to allow the EntityType attribute to be split up into multiple attributes, discovery will need to be updated to handle multiple attributes. Whether those additional attributes are used to set defined properties, in the EntityType case, or additional properties needed by other modules, such as in the Navigation case, do not seem to be a significantly different implementations.

The TypeError exception is being thrown at this line:
if (!isset($core_releases[$this->existingCoreVersion])) {

$this->existingCoreVersion is set in the class constructor like so: $this->existingCoreVersion = $core_data['existing_version'];.

AFAICT, the only time in core that the 'existing_version' property can be set to TranslatableMarkup is in core/modules/update/update.compare.inc:

if (isset($info['version'])) {
  <snip ...>
}
else {
  // No version info available at all.
  $install_type = 'unknown';
  $info['version'] = t('Unknown');
  $info['major'] = -1;
}

// Finally, save the results we care about into the $projects array.
$projects[$key]['existing_version'] = $info['version'];

I'm not sure how to get in a state where version info for Drupal core itself would be missing, but it's possible that the composer_deploy module touches some of that metadata, so there might be something in there that causes the version to get unset.

Nevertheless, the code in ProjectCoreCompatibility can be hardened to detect whether the existing version is not a string, using logic similar to what Package Manager does:

if ($existing_version instanceof TranslatableMarkup && $existing_version->getUntranslatedString() === 'Unknown') {

So in ProjectCoreCompatibility, it could look something like:

  public function __construct(array $core_data, array $core_releases, array $supported_branches) {
    if (isset($core_data['existing_version'])) {
      $this->existingCoreVersion = $core_data['existing_version'] instanceof TranslatableMarkup ? $core_data['existing_version']->getUntranslatedString() ? $core_data['existing_version'];
      $this->possibleCoreUpdateVersions = $this->getPossibleCoreUpdateVersions($core_releases, $supported_branches);
    }
  }

Alternatively, probably should consider enforcing version to be a string (making it translatable doesn't make much sense IMHO), so
changing $info['version'] = t('Unknown'); to $info['version'] = 'Unknown'; might be a good place to start. That code dates back a long way (at least to 2007?), though, so not sure if there'd be any side effects from doing that.

Discussed this issue with one of the plugin subsystem maintainers over Slack. He is not completely against having supplemental attributes, but he thinks it might be addressed more easily by allowing arbitrary properties to be set on Plugin attributes: $this->additional[$key] = $value. (Splitting the EntityType attributes into separate attributes is still valid and would not be tied to this.)

I know Drupal core does have a version of this with one plugin type: \Drupal\Core\Layout\Attribute\Layout, which looks like this:

/**
   * Any additional properties and values.
   *
   * @var array
   *
   * @see \Drupal\Core\Layout\LayoutDefinition::$additional
   */
  public readonly array $additional;

  /**
   * Constructs a Layout attribute.
   *
   * @param string $id
   *   The plugin ID.
       <snip ...>
   * @param class-string|null $deriver
   *   (optional) The deriver class.
   * @param mixed $additional
   *   (optional) Additional properties passed in that can be used by a deriver.
   */
  public function __construct(
    public readonly string $id,
    <snip ...>
    public readonly ?string $deriver = NULL,
    ...$additional,
  ) {
    // Layout definitions support arbitrary properties being passed in, which
    // are stored in the 'additional' property in LayoutDefinition. The variadic
    // 'additional' parameter here saves arbitrary parameters passed into the
    // 'additional' property in this attribute class. The 'additional' property
    // gets passed to the LayoutDefinition constructor in ::get().
    // @see \Drupal\Core\Layout\LayoutDefinition::$additional
    // @see \Drupal\Core\Layout\LayoutDefinition::get()
    $this->additional = $additional;
  }

We could possibly add $additional to Drupal\Component\Plugin\Attribute\Plugin, and merge the contents of $this->additional to AttributeBase::get() in Plugin::get(). This would involved updating all the plugin type attribute constructors as well.

+1 to going forward with the hook here and doing additional work in a follow-up. Though if we're going forward with the hook only, the IS and CR need updates.

Added comments on the MR.

Looks like there's an existing issue about KTB log testing: ✨ Allow kernel tests to fail or expect logged errors Needs work .

Made the change so that the test class itself is the logger.

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 🌱 Use \Drupal consistently in tests Needs work 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.