Redirects for translation set migration path in Drupal 6 and 7

This was addressed for D6 in #2827644: Fix path alias migration of translated nodes [D6] - it looks like all that's needed for D7 is to make the corresponding changes to d7_url_alias.yml and add tests. We can either do this as a follow-up to #2669964: Migrate Drupal 7 core node translations to Drupal 8, or incorporate it into that patch.

IMHO do it as a followup, we don't do any other reference updates, eg. where comments were posted to nodes, or where menu items are pointing to the node, etc. All of those are also broken. Aliases are not anywhere more special than the rest of the data referencing the node that is no longer there.

Also we do not have a redirection solution in core that I know, so we could only do the aliasing updates.

In that case this would reach back to Drupal 6's data too.

I believe #2827644: Fix path alias migration of translated nodes [D6] is using migrate's ID map tracking, but for this we would need to somehow have a more permanent tracking data store(?)

Updated issue summary given #2850984: Fix path alias migration of translated nodes [D7] so we can focus on the redirect problem here.

I'd like to work on this. Why is it postponed?

It was originally postponed on #2669964: Migrate Drupal 7 core node translations to Drupal 8. I think it may still be postponed on #2850984: Fix path alias migration of translated nodes [D7] unless there is no overlap in where they change things.

I don't see any overlap with #2850984: Fix path alias migration of translated nodes [D7], so I put it back to Active.

I have a working proof of concept, but I'm wondering where to put the code, right now it's in a custom module.

The code consists only of two event subscribers, one for the migrate.post_row_save event (to populate a key/value collection) and the other for the kernel.exception event (to redirect 404s based on the key/value collection).

I can think of these three possible locations:

• As a hidden sub-module of the node module (although I don't see any of those in core)
• Directly in the node module (If adding two services only used in on migrated multilingual sites is not a problem)
• In a contrib module

What would be the best approach here?

@maxocub: I think @catch meant a separate module in core that the migration enables but otherwise is hidden.

@Gábor Hojtsy: OK, thanks, I'll upload a patch when my tests are fixed.

Here's a first draft.

I have a hard time trying to test the redirection, if anyone have a better idea, please tell me.

Trying to fix the failing tests.

Can we just use redirect.module instead? I really don't like the feel of this direction...

@heddn: is that in core? :)

I'm suggesting we add a sub-module to redirect.module that does just what we need. But a lot of the plumbing and test cases are pretty solid in that module and I'd be worried about conflicts between the two modules. And a UI and the UX of a similar but different module in core.

Assigning to myself for review.

1. +++ b/core/modules/migrate_drupal/migrate_drupal.services.yml
   --- /dev/null
   +++ b/core/modules/migrate_drupal/src/EventSubscriber/NodeTranslationSubscriber.php
   
   +++ b/core/modules/migrate_drupal/src/EventSubscriber/NodeTranslationSubscriber.php
   @@ -0,0 +1,95 @@
   +  public function onPostRowSave(MigratePostRowSaveEvent $event) {
   +    if (substr($event->getMigration()->getPluginId(), 2, 17) == '_node_translation') {
   ...
   +  public function onPostImport(MigrateImportEvent $event) {
   +    if (substr($event->getMigration()->getPluginId(), 2, 17) == '_node_translation') {
   

   Hm, should node module handle this? I don't know how these subscribers are implemented elsewhere.

2. +++ b/core/modules/node/tests/src/Kernel/Migrate/d6/MigrateNodeTest.php
   @@ -30,6 +30,8 @@ protected function setUp() {
   +      'd6_language_types',
   +      'd6_language_negotiation_settings',
   
   +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php
   @@ -46,6 +46,8 @@ protected function setUp() {
   +      'd7_language_types',
   +      'd7_language_negotiation_settings',
   
   +++ b/core/modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php
   @@ -31,6 +31,8 @@ protected function setUp() {
   +      'd6_language_types',
   +      'd6_language_negotiation_settings',
   

   This seems unrelated?

3. +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php
   --- /dev/null
   +++ b/core/modules/node_translation_redirect/node_translation_redirect.info.yml
   
   +++ b/core/modules/node_translation_redirect/node_translation_redirect.info.yml
   +++ b/core/modules/node_translation_redirect/node_translation_redirect.info.yml
   @@ -0,0 +1,7 @@
   
   @@ -0,0 +1,7 @@
   +name: Node translation redirect
   

   Should this be a sub-module under migrate_drupal?

4. +++ b/core/modules/node_translation_redirect/src/EventSubscriber/ExceptionSubscriber.php
   @@ -0,0 +1,86 @@
   +/**
   + * TODO
   + */
   +class ExceptionSubscriber implements EventSubscriberInterface {
   

   Should this be same as the module description? Don't think we need to be very creative here :)

5. +++ b/core/modules/node_translation_redirect/src/EventSubscriber/ExceptionSubscriber.php
   @@ -0,0 +1,86 @@
   +    $path = $event->getRequest()->getPathInfo();
   +    if ($status_code == 404 && preg_match('/^\/node\/(\d+)$/', $path, $matches)) {
   

   This is always the resolved internal URL? I think it would be useful to highlight that we already migrate path aliases too.

6. +++ b/core/modules/node_translation_redirect/src/EventSubscriber/ExceptionSubscriber.php
   @@ -0,0 +1,86 @@
   +      $collection = $this->keyValue->get('node_translation_redirect');
   

   Hm, how does this work if a site migrates data from different sources? Eg. a Drupal 6 and a Drupal 7 site? That may not be a 80% use case, but sounds like this would break there, given the different source node ids possibly overlapping.

Looks like there are questions to be answered... Setting to NW

I'm working on this in Baltimore.

1. I don't know, the only other non-migrate module implementing those subscribers is content_translation in ContentTranslationUpdatesManager. I'm really not sure where it's best to put those, maybe @catch could weight in?
2. This is required for the tests to pass because the language module implements hook_modules_installed(), which is call when we install the node_translation_redirect module after we migrate translated nodes. And than hook updates language types:
   

   $negotiator = \Drupal::service('language_negotiator');
   $configurable = \Drupal::config('language.types')->get('configurable');
   $negotiator->updateConfiguration($configurable);
   

3. I don't know, in #15 and #16 we kind of assumed that @catch meant a separate module. Also, there's no sub-module in core.
4. Done.
5. Added a comment.
6. I don't think there's an acceptable way to diferentiate two identical ids comming from to Drupal sources. Should we just document the risks of migrating more than one Multilingual Drupal site? We could also throw an exception if someone try to migrate a second Drupal site and if one ID is already present in the key/value collection.

Fixing coding standards and two tests.

Fixing the remaining failing tests.

#27.1 I think this should be move into node module. We've tried pushing that type of stuff out to the module itself as much as possible.
#27.3 We split out sub-modules in contrib to bundle in the same package for drush, but in core, we don't need that. I don't think we need to switch from that and start creating sub-modules.
#27.6 I don't think this is a possibility that can be addressed. Unless I misunderstand your comment.

Moving back to NW for item #1.

Moved the subscriber from migrate_drupal to the node module

We need to check if the MigrateEvents class exists before subscribing to migrate events now that the subscriber is in node.
Hope this fixes all the failing tests...

Minor coding standards correction: use short array syntax, [].

+++ b/core/modules/node/src/EventSubscriber/NodeTranslationRedirectSubscriber.php
@@ -0,0 +1,99 @@
+    if (substr($event->getMigration()->getPluginId(), 2, 17) == '_node_translation') {
...
+    if (substr($event->getMigration()->getPluginId(), 2, 17) == '_node_translation') {

Haven't done a full code review, but a casual look gives me some concern here - I'm not thrilled with depending on magic ID names. Could we determine this instead from the source plugin having ['translations' => TRUE]?

If not, at the very least s/2, 17/-17/ would enable this to work with migrate_upgrade-generated migrations of the form 'upgrade_d6_node_translation'.

@mikeryan: You're right, that was not very strong or flexible.
Now I check if we are migrating node entities by looking at the destination plugin and if they are translatable by looking at the translations configuration.
Although, instead of looking at the 'translations' key in the source plugin, as you suggested, I look at the 'translations' key in the destination plugin, since its already loaded. I don't think it matters, but I might be wrong.

@maxocub - this needs to be done for onPostImport() as well.

@c.nish2k3: Oups, forgot about that one. To avoid duplicating the code there, I added a helper method.

Also, finally, I look at the 'translations' key on the source configuration instead of the destination, to avoid running that code if we are migrating sources other than Drupal. Let's see if it makes the 'MigrateExternalTranslatedTest' pass.

Parent meta is critical. Moving that priority down here. And closing out parent as this was the last active task.

#2746527: [META] Handle data related to Drupal 6 and 7 node translations with different IDs has various points that did not yet have subissues.

It has been discussed during the weekly migrate hangout that we should use a dedicated table to store the redirection map instead of the key/value store, since it could become bloated on a big site. Back to NW.

Not just the bloat. We discussed that you may be *done* with your migrations, but want to keep your site up including the redirection feature, which is useful way past when migrate was needed on the site.

Here's a new version using a mapping table instead of the key/value store.

Assigning to myself for review.

I cannot see anything wrong in the code. I'm not sure I like the yucky feeling I get with the architecture implemented here, but I don't have any better ideas. So let's move along to RTBC.

I kind of prefer the key value collection too.
But since the node_translation_redirect module is essentially a single event subscriber, wouldn't it be better to just move it to the node module instead of adding a new module to Core?

Inventing a tag to mark this as pertaining to migrate_drupal and therefore blocking its stable release.

Blocker for #2905736: Mark Migrate Drupal as stable

First a re-roll.

re #57 why a new module: I think the idea for a new module is that the K/V lookup is somewhat expensive and to turn it on/off, it is easier to do this with a new module. It is only needed for nodes that were migrated from an i18n source.

+++ b/core/modules/node_translation_redirect/tests/src/Kernel/Migrate/d6/NodeTranslationRedirectTest.php
@@ -0,0 +1,68 @@
+    $response = $kernel->handle($request);

+++ b/core/modules/node_translation_redirect/tests/src/Kernel/Migrate/d7/NodeTranslationRedirectTest.php
@@ -0,0 +1,70 @@
+    $response = $kernel->handle($request);

Nit: unused variable.

I've fixed the nit in the attached patch and marked this RTBC. I don't think removing an un-used variable makes me disqualified.

Should we be worried about bloat of K/V if someone has a TON of translated nodes on their site?

1. +++ b/core/modules/node/node.services.yml
   @@ -50,3 +50,8 @@ services:
   +  node.node_translation_redirect_subscriber:
   +    class: Drupal\node\EventSubscriber\NodeTranslationRedirectSubscriber
   +    arguments: ["@keyvalue", "@module_handler", "@module_installer"]
   +    tags:
   +      - { name: event_subscriber }
   
   +++ b/core/modules/node/src/EventSubscriber/NodeTranslationRedirectSubscriber.php
   @@ -0,0 +1,116 @@
   +    if (class_exists('\Drupal\migrate\Event\MigrateEvents')) {
   

   I was concerned about having this in the global container/event dispatcher for all sites - and seeing the way the getSubscribedEvents method is implemented confirms my concerns.

   We need to add a NodeServiceProvider class here that implements ServiceModifierInterface and only adds the event subscriber service if migrate module is enabled.

   For more info see https://www.previousnext.com.au/blog/overriding-services-drupal-8-advanc... and for a concrete example of conditionally adding a service if a module is enabled - see https://github.com/drupal-media/file_entity/blob/8.x-2.x/src/FileEntityS...

2. +++ b/core/modules/node/src/EventSubscriber/NodeTranslationRedirectSubscriber.php
   @@ -0,0 +1,116 @@
   +    return !empty($source_configuration['translations']) && $destination_configuration['plugin'] == 'entity:node';
   

   nit: we can use === here

3. +++ b/core/modules/node_translation_redirect/node_translation_redirect.info.yml
   @@ -0,0 +1,7 @@
   +name: Node translation redirect
   

   I'm also not convinced that a new module is the answer here. Will dig about with the service provider notion and see if we can conditionally add this in node too.

4. +++ b/core/modules/node_translation_redirect/src/EventSubscriber/ExceptionSubscriber.php
   @@ -0,0 +1,89 @@
   +    if ($status_code == 404 && preg_match('/^\/node\/(\d+)$/', $path, $matches)) {
   

   We can't match this on path - because paths can be modified. We need to match it on route name - entity.node.canonical

5. +++ b/core/modules/node_translation_redirect/src/EventSubscriber/ExceptionSubscriber.php
   @@ -0,0 +1,89 @@
   +      $old_nid = isset($matches[1]) ? $matches[1] : NULL;
   

   Similarly, once we use the route match service, we can use the raw parameter here instead of the regex matches

6. +++ b/core/modules/node_translation_redirect/src/EventSubscriber/ExceptionSubscriber.php
   @@ -0,0 +1,89 @@
   +        $response->send();
   

   I think we should be doing $event->setResponse($response) here. Sending the response immediately prevents further execution.

7. +++ b/core/modules/node_translation_redirect/tests/src/Kernel/Migrate/d6/NodeTranslationRedirectTest.php
   @@ -0,0 +1,68 @@
   +    $this->expectOutputString('<!DOCTYPE html>
   +<html>
   +    <head>
   +        <meta charset="UTF-8" />
   +        <meta http-equiv="refresh" content="1;url=/fr/node/10" />
   +
   +        <title>Redirecting to /fr/node/10</title>
   +    </head>
   +    <body>
   +        Redirecting to <a href="/fr/node/10">/fr/node/10</a>.
   +    </body>
   +</html>');
   +    $kernel->handle($request);
   
   +++ b/core/modules/node_translation_redirect/tests/src/Kernel/Migrate/d7/NodeTranslationRedirectTest.php
   @@ -0,0 +1,70 @@
   +    $this->expectOutputString('<!DOCTYPE html>
   +<html>
   +    <head>
   +        <meta charset="UTF-8" />
   +        <meta http-equiv="refresh" content="1;url=/is/node/2" />
   +
   +        <title>Redirecting to /is/node/2</title>
   +    </head>
   +    <body>
   +        Redirecting to <a href="/is/node/2">/is/node/2</a>.
   +    </body>
   +</html>');
   +    $kernel->handle($request);
   

   Once we change the code above to not use $response->send() we can then go $response = $kernel->handle() and then assert that the status code is 301 and the location is as expected - instead of asserting the raw output.

Would anyone object to people needing this functionality having to add a container parameter to enable the redirect, instead of the hidden module?

so in sites/default/services.yml they need to add

parameters:
  migrate.node_translation_redirect: 1

And then we use that in the service provider to enable the subscriber?

@larowlan: trick question: can migrate add that container parameter?

If Drupal can write to sites/default/services.yml then yes it can.

I think we do that in tests...will confirm

\Drupal\Core\Test\FunctionalTestSetupTrait::setContainerParameter is what I'm referring to.

But it also relies on having this in settings.php

$settings['container_yamls'][] = $app_root . '/' . $site_path . '/services.yml';

Although I note that is the default in default.settings.php.

Can migrate check those two things first?

i.e. that services.yml exists and is writable and that Settings::get('container_yamls', []) includes the services.yml file?

I will also ask around in case there is a better way

64.1: where we not trying to do the redirect in another module outside of migrate and using k/v instead of the migrate mapping takes so we could disable migrate module?

Also, I'd like a confirmation on my concern with k/v bloat

For a start, I fixed #64.2, #64.6 & #64.7.

About #64.4 & #64.5, I am not able to retrieve the route name with RouteMatch in the ExceptionSubscriber, it's always returning NULL, anybody have a clue why? This is what I'm doing:

  public function onException(GetResponseForExceptionEvent $event) {
    $route_name = RouteMatch::createFromRequest($event->getRequest())->getRouteName();
    // $route_name is always NULL.
    // ...

If the URL we are trying to reach does not exists, isn't it normal that we don't have a route name for it?
If so, how else can we check that the requested path is a node path, other than $request->getPathInfo();?

About #64. & #64.5:
After a few shots at this, I don't think there's a way to use the route match service to replace the preg_match. If, for example, '/node/2' was a translation on D7 and so does not exists anymore on D8, then there's no way to match this path to a route. We will always end up with a NullRouteMacth. So for now, I don't see any way around a regex, unless someone has a better idea?

One thing though, the current regex is not OK, since it's not catching paths like '/fr/node/2'.

Tagging this for some in person brainstorming in Vienna.

What would be nice is a list of issues that /need/ to be resolved for migrate drupal stability. Or at least as much as possible, a full list of what is remaining.

WIP, am I on a good track?

Still a WIP. Got rid of the new module in favor of two new node services:

1. NodeTranslationMigrateSubscriber, which is only added to the container if migrate is enabled and is responsible of saving the mapping between old translated nids to new ones during a multilingual migration.
2. NodeTranslationExceptionSubscriber, which is only added to the container if a container parameter is set (still needs work) and is responsible of redirecting old translated nids to new ones.

I now have to work on setting that container parameter after a multilingual migration.

This should fix the tests.

+++ b/core/modules/node/src/NodeServiceProvider.php
@@ -0,0 +1,38 @@
+    // Register the node.node_translation_exception service to the container if
+    // the node.node_translation_redirect container parameter is set
+    if (TRUE) {

This is the part that needs work an discussion.

What was suggested between #65 and #68 is that we add a container parameter to services.yml if it exists, is writable and is included in settings.php.

Those conditions might not be met in the majority of cases, and in those case the user might not be familiar or have no access to the creation of a services.yml file.

Should we look for another way or is that an acceptable solution?

This is tough, the writing to a new or existing services file during the migration. It won't work for example on platform.sh, because the disk will be read-only. Instead, we can enable/disable a module. Which gets us back to where we started.

Would a simple $container->setParameter() in the first if statement of NodeServiceProvider work? That seems too easy and I'm not familiar with container parameters.

If this works, it means we are 'activating' the redirections only if we have the migrate & language modules enabled, but them we can disable the migrate module and still have our redirections.

Also, re #83:
I added the early return if it's not a NotFoundHttpException.
One thing though, I don't understand if you are saying that the added parameters/methods to ParamNotConvertedException are not a good idea and we need to find another solution, or if it's bad but we have to live with it?

Re #85:
No that won't work, because if we uninstall migrate then the container gets rebuilt and the parameter is lost. Bummer.

Can we:

• unconditionally add the event subscriber service to node
• add a state flag in migrate that says 'i18n nodes were migrated'
• in the event subscriber return early if that flag isn't set - possibly in the getSubscribedEvents method

I have the read the issue, installed the patch and had a brief look at the code. Everything looks good, so far. I mean that all the comments have been addressed but one.

What remains to do is to get an answer to maxocub question in #82.

Discussed with phenaproxima, and he would like to have input from another framework manager on this.

I would like input from another framework manager, but if we can't get it:

in the event subscriber return early if that flag isn't set - possibly in the getSubscribedEvents method

This looks like a great suggestion to me.

I really like this patch a lot. I feel like it's a pretty elegant solution to this persistent problem, and I can't wait to see it land.

1. +++ b/core/lib/Drupal/Core/ParamConverter/ParamNotConvertedException.php
   @@ -7,4 +7,45 @@
   +  public function __construct(string $message = "", int $code = 0, \Throwable $previous = NULL, $routeName = "", $parameters = []) {
   

   I could be wrong, but I think \Throwable is not a thing in PHP 5, so $previous needs to be \Exception.

   Also, $routeName should be $route_name, $parameters should be type hinted, and if $routeName and $parameters are required, we should make them required constructor parameters :) PHP lets constructors have any signature you want, even if it's incompatible with the parent constructor.

   Additionally, we need a full doc block (not @inheritdoc), since we're defining new parameters here.

2. +++ b/core/lib/Drupal/Core/ParamConverter/ParamNotConvertedException.php
   @@ -7,4 +7,45 @@
   +  /**
   +   * Get the route name.
   +   *
   +   * @return string
   +   */
   +  public function getRouteName() {
   

   @return needs documentation, even if it's just "The route name" (lame, I know).

3. +++ b/core/lib/Drupal/Core/ParamConverter/ParamNotConvertedException.php
   @@ -7,4 +7,45 @@
   +  /**
   +   * Get the route parameters.
   +   *
   +   * @return array
   +   */
   +  public function getParameters() {
   

   @return needs documentation. It's probably worth mentioning if these are the raw parameters, or the so-far converted ones.

4. +++ b/core/modules/node/src/EventSubscriber/NodeTranslationExceptionSubscriber.php
   @@ -0,0 +1,98 @@
   +   * @param \Drupal\Core\KeyValueStore\KeyValueFactoryInterface $keyValue
   +   *   The key value factory.
   +   * @param \Drupal\Core\Language\LanguageManagerInterface $languageManager
   +   *   The language manager.
   +   * @param \Drupal\Core\Routing\UrlGeneratorInterface $urlGenerator
   +   *   The URL generator.
   

   Nit: These variable names should be using $snake_case, not $camelCase.

5. +++ b/core/modules/node/src/EventSubscriber/NodeTranslationMigrateSubscriber.php
   @@ -0,0 +1,62 @@
   +   * @param \Drupal\Core\KeyValueStore\KeyValueFactoryInterface $keyValue
   +   *   The key value factory.
   

   Nit: Should be $key_value.

6. +++ b/core/modules/node/src/EventSubscriber/NodeTranslationMigrateSubscriber.php
   @@ -0,0 +1,62 @@
   +   * Maps the old nid to the new one in the key value collection.
   

   This is a bit vague, can it be expanded to explain why it's storing this mapping? A paragraph or two would be useful. A change like this is best documented extensively in the code.

7. +++ b/core/modules/node/src/NodeServiceProvider.php
   @@ -0,0 +1,39 @@
   + * Register the node_translation_redirect service if migrate is enabled.
   

   Nit: This doc comment isn't really accurate, and it repeats the comment in register(), which is more useful because the code is right there. So I think this can just say something generic, like "Registers services in the container."

8. +++ b/core/modules/node/src/NodeServiceProvider.php
   @@ -0,0 +1,39 @@
   +      $container->register('node.node_translation_migrate', 'Drupal\node\EventSubscriber\NodeTranslationMigrateSubscriber')
   

   Nit: Can this use the ::class syntax instead?

9. +++ b/core/modules/node/src/NodeServiceProvider.php
   @@ -0,0 +1,39 @@
   +      $container->setParameter('node.node_translation_redirect', TRUE);
   

   In discussion, we agreed to ditch the container parameter in favor of setting a state or config key. Given the circumstances, a container parameter may be too volatile for what we're trying to accomplish.

10. +++ b/core/modules/node/src/NodeServiceProvider.php
    @@ -0,0 +1,39 @@
    +    if ($container->hasParameter('node.node_translation_redirect') && $container->getParameter('node.node_translation_redirect')) {
    

    As per the last comment, we don't need the if statement anymore.

11. +++ b/core/modules/node/src/NodeServiceProvider.php
    @@ -0,0 +1,39 @@
    +      $container->register('node.node_translation_exception', 'Drupal\node\EventSubscriber\NodeTranslationExceptionSubscriber')
    

    Can we use ::class here?

12. +++ b/core/modules/node/tests/src/Kernel/Migrate/d6/NodeTranslationRedirectTest.php
    @@ -0,0 +1,58 @@
    + * @group node_translation_redirect
    

    Not a valid group anymore :) This should probably have:

    @group migrate_drupal
    @group node
    

13. +++ b/core/modules/node/tests/src/Kernel/Migrate/d7/NodeTranslationRedirectTest.php
    @@ -0,0 +1,60 @@
    + * @group node_translation_redirect
    

    Need to adjust this too. ;)

I'll take a look at addressing @phenaproxima's review.

Sorry Jo, I'm already on it at the migrate sprint in vienna, I just didn't thought of assigning it to myself

No problem, @maxocub. Go for it!

I just wish I was with y'all in Vienna.

@Jo Fitzgerald: Wish you were here to meet you!

Re #90:

1. You're right, \Throwable is new in PHP7. I don't think we can make routeName and parameters required because we would need to change all the throw new ParamNotConvertedException calls everywhere and I think this will break BC.
2. Done.
3. Done.
4. Done.
5. Done.
6. I added the documentation in the class doc block instead.
7. Done.
8. Done.
9. Done.
10. Done.
11. Done.
12. Done.
13. Done.

Removed "string" & "int" type hints since they won't work on PHP5.

Oh, I like it.

On the assumption that Drupal CI will pass it, I think this is RTBC. Thanks, @maxocub!

Unassigning.

Following discussions with @phenaproxima and @catch, here are some conclusions to questions we had to improve this patch:

Could we merge the two subscriber into one?

• Based on #64.1, I'd say it is better as it is since we don't have to check if the migrate event classes exists.

What about checking for the state in the getSubscribedEvents()?

• It doesn't work, even with \Drupal::state(). This is because that the container is not initialized at that point.

Thanks for the review @catch, I'm on it.

1. I moved the fetching from state after the route name check, and changed the variable names.
2. I removed the call to $collection->has(), $collection->get() already returns NULL if it's not set.
3. For the same reasons as why it can't be in the getSubscribedEvents() call, the container is not yet initialized.

#103 looks lovely to me.

And +1 to @maxocub's reply to #101.3 -- we can't use container services in a service provider, because the container is in an indeterminate state (i.e., still in the middle of being built). LanguageServiceProvider, it seems to me, realized this and had to hack around it.

On it.

Here's it is.

Looks like @catch is all over this one.