Replace path-based entity links with route names

What's the change to the hal normalizer base test class about?

Hmm, can't we just revert those changes to WebTestBase and add the router schema to the DUTB tests? I think that should work, let's try.

Interdiff includes reverting changes in those tests from the last patch and the changes needed.

EDIT:

+++ b/core/modules/hal/lib/Drupal/hal/Tests/NormalizerTestBase.php
@@ -58,6 +58,10 @@
+    $this->installSchema('system', array('variable', 'url_alias', 'router')); << Just 'router'.

+++ b/core/modules/rdf/lib/Drupal/rdf/Tests/Field/FieldRdfaTestBase.php
@@ -43,7 +43,16 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function setUp() {
+    parent::setUp();
+
+    $this->installSchema('system', array('router'));
+  }

For ease, this is all that I added to those tests after the code reverts.

Great patch!

+++ b/core/modules/content_translation/content_translation.module
@@ -91,6 +91,7 @@ function content_translation_entity_info_alter(array &$entity_info) {
       if (!empty($info['links']['canonical'])) {
         // Provide default links for the translation paths.
+        // @todo NOPE NOPE NOPE NOPE NOPE.
         $info['links'] += array(
           'drupal:content-translation-overview' => $info['links']['canonical'] . '/translations',

Sounds like this would need to be fixed in this patch too? I believe the content translation module has derivative routes based on entity routes, so this can be placed here these. The routes are 'content_translation.translation_overview_' . $entity_type.

It is true they are created based on this path provided here, but I think the path automation can just as well be moved to the route subscriber, those needing to alter that can alter the routes. AFAIS that should be very rare. That means this can go away from the info hook.

Could we do something like this? Genuine question.

The uri() method counterpart is now at #2134871: Entities should provide route and route parameter methods (instead of uri()?).

1. index 0171dd5..604bf5d 100644
   --- a/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
   
   --- a/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
   +++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
   
   +++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
   @@ -267,9 +267,7 @@ class EntityType extends Plugin {
   
   @@ -267,9 +267,7 @@ class EntityType extends Plugin {
       *
   

   The documentation of EntityType.$links has to be changed as well.

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Entity/CustomBlock.php
   @@ -38,8 +38,8 @@
   - *     "canonical" = "/block/{custom_block}",
   - *     "edit-form" = "/block/{custom_block}"
   + *     "canonical" = "custom_block.edit",
   + *     "edit-form" = "custom_block.edit"
   

   <3

3. +++ b/core/modules/content_translation/content_translation.module
   @@ -162,7 +156,7 @@ function content_translation_menu() {
   -      $keys = array_flip(array('theme_callback', 'theme_arguments', 'access_callback', 'access_arguments', 'load_arguments'));
   +      $keys = array_flip(array('theme_callback', 'theme_arguments', 'load_arguments'));
   

   Wait: we do remove access_callback but keep it above? I would like to understand that.

4. +++ b/core/modules/content_translation/content_translation.services.yml
   @@ -5,9 +5,9 @@ services:
   -      - { name: event_subscriber }
   +      - { name: event_subscriber, priority: -100 }
   

   What is the reason to make an event subscriber have a different priority? The priority of it will be handled in the actual getSubscribers method anyway.

5. +++ b/core/modules/content_translation/content_translation.services.yml
   @@ -20,3 +20,7 @@ services:
   +    arguments: ['@plugin.manager.entity']
   

   ... this should be @entity.manager

6. +++ b/core/modules/content_translation/lib/Drupal/content_translation/ContentTranslationManager.php
   @@ -0,0 +1,54 @@
   +
   +class ContentTranslationManager implements ContentTranslationManagerInterface {
   

   What is the responsibility of this class?

7. +++ b/core/modules/content_translation/lib/Drupal/content_translation/ContentTranslationManager.php
   @@ -0,0 +1,54 @@
   +  public function isSupported($entity_type, $info = NULL) {
   

   I wonder myself about the usecase to specify the $info manually. I haven't looked core though, yet.

8. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Plugin/Derivative/ContentTranslationLocalTasks.php
   @@ -91,20 +76,15 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
   -        if ($routes = $this->routeProvider->getRoutesByPattern($path)->all()) {
   

   I really really love this line!

9. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUriTest.php
   @@ -7,10 +7,19 @@
   -class EntityUriTest extends EntityUnitTestBase {
   +class EntityUriTest extends WebTestBase {
   

   What is the reason for this change? Doesn't this causes a big slow down for the test?

Yeah sorry, I think that may be able to go. Not sure? I originally added it so this ran after any other subscribers.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
@@ -18,124 +20,144 @@
+      // First try to get the route from the current collection.

Nitpick: I think this comment should state this is basically to cover routes added in the dynamic_routes set.

Otherwise, I think this is looookin' good.

Yeah, similar to why you had to add the fix to try and find the route from the current collection I guess. If its last we should always be able to find any dynamic route.

The priority is not controlled on the services level.

+++ b/core/modules/content_translation/lib/Drupal/content_translation/Routing/ContentTranslationRouteSubscriber.php
@@ -161,4 +162,13 @@ protected function routes(RouteCollection $collection) {
+    $events[RoutingEvents::ALTER] = array('onAlterRoutes', -100);

We are using routes(), so onDynamicRoutes and not alter ?

You wanted to say: good will, but just horrible execution.

Yeah, right idea. Wrong implementation there :-) same goes for the priority too!

This is looking good now.

Committed 7b8e204 and pushed to 8.x. Thanks!

For more PHPUnit coverage of this change in the entity API, see #2134857: PHPUnit test the entity base classes.

This is potentially quite problematic. We *need* the canonical relationship there. Also, we added those link patterns originally for REST module, so that we could *generate* the routes off of the links. That is, the whole point was the other way around from what this issue just did. See #2019123: Use the same canonical URI paths as for HTML routes There's been on and off discussion of generating the HTML route off of the defined URI, too.

Really, I think this issue is a regression. :-(

We discussed this issue at length in today's WSCCI meeting. The resolution:

* We need a new patch on this issue to roll back the switch from URI templates to routes in the entity annotations. Entity annotations must be URIs.
* We cannot just revert this patch as is, as it also fixed various translation issues.
* Once that's done, we will move the HTML route generation to be dynamic, just like the non-HTML routes. This has some implementation details we did not fully finish fleshing out, but that's a topic for a new issue.

So the action item here is: We need a patch to switch back to URI template routes, but not break the translation work here. Volunteers?

Because this is an API change, and we have 2 other issues blocked hard until this is done, I am bumping the priority.

We seriously need to stop renaming and repurposing the same issues...

Yes please close this issue and open a new one, it's incredibly confusing restarting issues, messes up git blame and all kinds of other problems.

Here is one: #2150747: Switch back to URI templates in entity annotations

We don't need a change notice here if we're about to undo it, do we?

I agree, no change notice needed here since we should roll this back in #2150747: Switch back to URI templates in entity annotations.