@duadua

I really like the idea of copying Symfony 6.4 and adding an always there FQCN route alias. That would mean getting #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name done. Definitely think it is worth a follow-up. I wonder if we should forgo the automatically generated names like 'MethodRouteLocale' and instead where no name is provide automatically name using the FQCN that Symfony would create an alias for?

What feels very sensible for me. Looking at the current way its generated, a FQCN name would be no way longer compared to the current autogeneration.
Looking at this it just let me realize that auto-generation has the advantage of avoiding accidental re-definitions of the same route name.

CREATE TABLE IF NOT EXISTS "router" (
"name" VARCHAR(255) NOT NULL DEFAULT '',
"path" VARCHAR(255) NOT NULL DEFAULT '',
"pattern_outline" VARCHAR(255) NOT NULL DEFAULT '',
"fit" INTEGER NOT NULL DEFAULT 0,
"route" BLOB DEFAULT NULL,
"number_parts" INTEGER NOT NULL DEFAULT 0,
"alias" VARCHAR(255) NOT NULL DEFAULT '',
 PRIMARY KEY ("name")
);
CREATE INDEX "router_pattern_outline_parts" ON "router" ("pattern_outline", "number_parts");
CREATE INDEX "router_alias" ON "router" ("alias");

Looking at the schema for the router, I see there is a limit of length for the route name. Looking at everything, 255 seems plenty of space though for:

• Drupal
• Module name
• "Controller"
• Controller name
• Method name

   // Not using chunks because not many aliases expected.
+      $insert = $this->connection->insert($this->tableName)->fields([
+        'name',
+        'route',
+        'alias',
+      ]);

I'm coming here from https://www.drupal.org/project/drupal/issues/3311365 📌 Use PHP attributes for route discovery Needs review . It seems like this assumption might not hold necessarily?

+/**
+ * Rebuilds the router with alias support.
+ */
+function system_post_update_rebuild_matcher_dumper_with_alias_support(): void {
+  \Drupal::database()->schema()->dropTable('router');
+  \Drupal::service('router.builder')->rebuild();
+}

Is this a case where it would be better to use ->setRebuildNeeded(). Looking at what post update is doing, it does flush all caches afterwards, which would rebuild the router if needed.

@duadua I think I got a bit change-record-keen :)

For a while I was trying to figure out whether there is a logical space in which the slightly different title might exist.
I tried to delete the particular change record, but there are some additional permissions required. Someone will probably see the updated title → .

Good question. I think I'd expect these to be declared in a modules services file - maybe an event subscriber. Yeah - this change introduces RoutingEvents::STATIC so these callbacks could be changed to an event subscriber listening to that event and then add their routes in.

Added https://www.drupal.org/project/drupal/issues/3401172 📌 Figure out how to handle generating dynamic Active

duadua → created an issue.

@alexpott
You created two change records:
- https://www.drupal.org/node/3324758 →
- https://www.drupal.org/node/3314769 →

Neither of them had any information in it, so I started filling out some information on the first.
Is there a reason for us to keep the second one around?

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

Personally I think the two existing change records (one for plugins, one for plugin types/managers) are enough, and we can update them as we convert more plugin types - perhaps we need a table to show which types were converted in which core versions.

Great idea! I expanded the existing table in https://www.drupal.org/node/3395575 → with a new column.

This is some interesting research about the php parsing process!

Question: Do we need a change record if there is already one for the original ticket?

So you are saying for now it would be better to:

- open up a follow up to accept an iterable
- accept only an array right now?

I’m excited to see what you come up with on the factory/initialisation side of things.

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

• Two change records provided: one for plugins and one for plugin types (As suggested by longwave)
• Update the issue summary

Looking at this now.

It seems like @donquixote and @joachim are bringing out some valid points:

- Naming
- Developer experience
- Genericness

Generally I agree that the experience is not great. It feels like having some form of factory for that seems a good idea.
A few places for that are:

- Entity storage class (adding even more to those classes seems hard)
- Entity repository (feels like a nice place) (createEntityIterable(iterable $ids): EntityIterable)
- Tie it to an entity query, like a method ->executeAndLoadIterable()

My general gut feeling is that entity repository is a good place. It balances flexibility with discoverability . We could then put in something on the entity query as a follow up on top of that.

duadua → created an issue.

Testing this as Drupalcon today. I'm trying to understand the reasoning behind this change, and talked with lauriii about this. We added one more sentence to the change record to help existing sites/themes to make a decision.
I clicked through some example pages:

- /admin
- /admin/content (On this one 'Content' appears multiple times, but that seems correct, given that admin/content/overview is the default tab.
- /admin/modules
- ...

When the original router patch was committed, there wasn't even thought given to the relationship between a router item, its access, and a menu link (see #1793520-7: Add access control mechanism for new router system downwards), and it took several people years, blocking Drupal 8's release, to repair some (but not all) of the damage done to the menu system. #2407505: [meta] Finalize the menu links (and other user-entered paths) system and sub-issues has some of the pain, and you can see that wasn't even opened until two years after the original router commit landed. So short version: none, but also worse than none.

It is an interesting aspect that this moves the responsibility from the developer towards the site builder/theme, the layer that actually knows what sensible. From my experience this makes the most sense.
While reading the code I had two minor nitpicks

@dpi

To be honest I am confused whether to create MRs against 10.1.x or 11.x at this point.

@dpi
That's a great recommendation. I'll do that and experiment a bit more first, before posting a MR into this issue.

Narrowing the scope of this, perhaps this can be a discussion issue and then we open child issues when we have determined tags that could be autoconfigured. I opened #3367455: Use service autoconfiguration for all event subscribers for the remaining event subscribers.

That sounds like a really good idea!

Cache contexts

Look at cache contexts there are actually two interfaces involved:

1. \Drupal\Core\Cache\Context\CacheContextInterface
2. \Drupal\Core\Cache\Context\CacheContextsPass

Interesting enough the services then require to start with cache_context., see \Drupal\Core\Cache\Context\CacheContextsPass,
but I don't see a reason why this would block autoconfiguration.

Cache bins (potentially)

Looking at an example:

  cache.page:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin }
    factory: ['@cache_factory', 'get']
    arguments: [page]

Given the additional arguments, it feels like this could be its own issue to discuss it.

Page cache policies

An example of a page cache policy looks like:

    basic_auth.page_cache_request_policy.disallow_basic_auth_requests:
    class: Drupal\basic_auth\PageCache\DisallowBasicAuthRequests
    public: false
    tags:
      - { name: page_cache_request_policy }

This looks like a perfect candidate for autoconfiguration.

Theme negotiators

An example looks like this:

    theme.negotiator.ajax_base_page:
    class: Drupal\Core\Theme\AjaxBasePageNegotiator
    arguments: ['@csrf_token', '@config.factory', '@request_stack']
    tags:
      - { name: theme_negotiator, priority: 1000 }

Theme negotiator seem to care all about priorities. This seems to unqualify them for autoconfiguration. We could introduce an additional attribute.

plugin_manager_cache_clear

This has its own autoconfigure logic anyway already in \Drupal\Core\Plugin\PluginManagerPass::process

Param converters

Looking at an example:

    access_check.entity:
    class: Drupal\Core\Entity\EntityAccessCheck
    tags:
      - { name: access_check, applies_to: _entity_access }

The additional parameter is required. This seems to unqualify them for autoconfiguration. We could introduce an additional attribute.

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

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

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

Potential other tags to enable this for:

• Cache contexts
• Cache bins (potentially)
• Page cache policies
• Theme negotiators
• plugin_manager_cache_clear (which has its own autoconfigure logic anyway already in \Drupal\Core\Plugin\PluginManagerPass::process
• Param converters

One interesting question would be: How can the documentation updates be handled. It seems like it would be best to update the documentation once the story is more coherent.

MR has been rebased.

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

Woah how come potx already finds those strings?

Yeah I was very much surprised to find that out as well. It speaks for PHP attributes that they are mostly just PHP itself

Also we should have a text case with context too.

Is this test case enough from your point of view?

And format plural indeed as per #3.

I posted a comment on the parent ticket 📌 Use PHP attributes instead of doctrine annotations Fixed due to my lack of understanding what the plan is for that.

@Gábor Hojtsy
The job for "PHP 5.6 & MySQL 5.5, D7 PHPLint Failed" is failing at the moment, given it introduces PHP 8.1 only language features into the test file.
Do you consider this as a problem? We could probably split up the test file into a PHP 8.1 only version.
Let me know what you think

After reviewing the comments mentioned above, it appears that the general consensus is to directly use the new PluralTranslation approach 📌 Use PHP attributes instead of doctrine annotations Fixed . However, there is a concern regarding the current placement of this object within the Drupal\Core\Annotation namespace, which does not seem like a progressive step.

Moving forward, there are several possible options to consider:

• Create a new object with the same properties within the Drupal\Core\StringTranslation\Attribute\PluralTranslation namespace: This option involves developing a fresh object that mirrors the existing properties but resides in the appropriate Drupal\Core\StringTranslation\Attribute\PluralTranslation namespace.
• Accept the fact that the object is in the wrong namespace: This option may involve some compromises or implications related to code organization and maintainability.
• Defer the concern to a later point and open a follow-up ticket: Since there are no usages of the object outside of entity annotations in the core

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

I'm also playing around with the idea to support using environment variables during runtime, see the --runtime branch.
This is clearly a bit more involved, and I think belongs really into its own issue in the first place.

What an amazing win in terms of developer experience.

• Rebased the branch
• Addressed a comment by @longwave
• Simplifed a tiny bit of code in AttributeBridgeDecorator

I was looking at the todo

::parseClass<code>:
<blockquote>
      // @todo Handle deprecating definitions discover via annotations.
</blockquote>
Given that Drupal 8/9 did some magic with the loading of annotations, it feels like it will not be enough to put <code>@deprecated

onto existing Annotation classes.

A small suggestion:

• Add @deprecated to the existing annotation classes, as this doesn't hurt
• Add a property onto the old annotation classes to point to the new attributes
• Let AttributeDiscoveryWithAnnotations::parseClass trigger a deprecation on runtime

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

@bircher
I added some basic test coverage for resolving the env variable at compile time.

duadua → created an issue.

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