Updated: Comment #31

Problem/Motivation

The ContentTranslationControllerInterface::get*Path() methods, which provide the paths to be used when building the UI for a particular entity type, should not be translation specific. AAMOF this is a common need and we should be exploiting a generic API instead of providing a one-off solution.

Proposed resolution

Rely on the Entity links API introduced in #1970360: Entities should define URI templates and standard links to generate entity URLs in a generic fashion. Remove the code implementing the current Content-Translation-specific API, deprecate what cannot be removed yet.

Remaining tasks

User interface changes

None

API changes

  • Removed the menu_view_path, menu_edit_path and menu_path_wildcard entity keys.
  • Deprecated the menu_base_path entity key (removal needs to wait for the conversion to the new routing system to be complete).
  • Added the links entity key definition to the custom block entity type.
  • Removed the getBasePath(), getEditPath() and getViewPath() methods from the ContentTranslationControllerInterface.

Background

Originally in #1188388-19: Entity translation UI in core
plach

The get*Path() methods, which provide the paths to be used when building the UI for a particular entity type, should belong to a UI controller or whatever comes up from #1783964: Allow entity types to provide menu items and #1781372: Add an API for listing (configuration) entities where the same issues we have here are being addressed. AAMOF those methods are not specific to translation and atm we need to inject menu router info in the entity info to be able to generate the translation UI in a generic fashion.

And responded in support in #1188388-81: Entity translation UI in core
Gabor

There was a lot of discussion on the entity listing patch about paths known to the controllers, and the need for getURI if only admin paths are known, etc. I think these are fine to discuss in the mentioned related issues and we can keep refining this without the need for any major rework, making this patch free to use its current ways.
CommentFileSizeAuthor
#82 ct-paths-1810350-82.patch47.05 KBplach
#82 ct-paths-1810350-82.interdiff.do_not_test.patch950 bytesplach
#79 ct-paths-1810350-79.patch47.71 KBplach
#79 ct-paths-1810350-79.interdiff.do_not_test.patch3.28 KBplach
#76 ct-paths-1810350-76.patch46.57 KBplach
#76 ct-paths-1810350-76.interdiff.txt1.73 KBplach
#73 ct-paths-1810350-73.patch45.57 KBplach
#73 ct-paths-1810350-73.interdiff.txt20.54 KBplach
#67 ct-paths-1810350-67.interdiff.txt6.48 KBkfritsche
#67 ct-paths-1810350-67.patch43.16 KBkfritsche
#66 ct-paths-1810350-66.patch43.14 KBboran
#66 interdiff-1810350-66.txt1.36 KBboran
#64 interdiff-1810350-61-63.txt1.36 KBboran
#61 ct-paths-1810350-57-61-interdiff.txt1.29 KBkfritsche
#61 ct-paths-1810350-61.patch43.14 KBkfritsche
#60 ct-paths-1810350-57-59-interdiff.txt1.58 KBkfritsche
#60 ct-paths-1810350-59.patch43.43 KBkfritsche
#57 ct-paths-1810350-57.patch41.85 KBplach
#56 ct-paths-1810350-55.patch41.92 KBplach
#50 ct-paths-1810350-50.patch.interdiff.do_not_test.patch1016 bytesplach
#50 ct-paths-1810350-50.patch.patch41.92 KBplach
#45 ct-paths-1810350-45.patch.interdiff.do_not_test.patch10.27 KBplach
#45 ct-paths-1810350-45.patch.patch42.18 KBplach
#42 ct-paths-1810350-41.patch39.34 KBplach
#40 ct-paths-1810350-40.patch38.57 KBplach
#39 ct-paths-1810350-39.patch39.74 KBplach
#37 ct-paths-1810350-37.interdiff.do_not_test.patch2.04 KBplach
#37 ct-paths-1810350-37.patch39.74 KBplach
#35 ct-paths-1810350-35.interdiff.do_not_test.patch2.49 KBplach
#35 ct-paths-1810350-35.patch39.53 KBplach
#33 ct-paths-1810350-33.interdiff.do_not_test.patch672 bytesplach
#33 ct-paths-1810350-33.patch39.47 KBplach
#28 ct-paths-1810350-28.interdiff.do_not_test.patch790 bytesplach
#28 ct-paths-1810350-28.patch39.43 KBplach
#23 ct-paths-1810350-23.patch39.42 KBplach
#22 ct-paths-1810350-22.patch39.43 KBplach
#19 ct-paths-1810350-19.interdiff.do_not_test.patch4.48 KBplach
#19 ct-paths-1810350-19.patch39.34 KBplach
#17 ct-paths-1810350-17.patch35.89 KBkfritsche
#17 interdiff-15-17.txt1.18 KBkfritsche
#15 ct-paths-1810350-15.patch36.42 KBkfritsche
#15 interdiff-13-15.txt3.19 KBkfritsche
#13 ct-paths-1810350-13.patch33.58 KBkfritsche
#13 interdiff-8-13.txt14.56 KBkfritsche
#11 ct-paths-1810350-7-rerolled-do-not-test.patch21.78 KBkfritsche
#10 ct-paths-1810350-7-rerolled-do-not-test.patch21.78 KBkfritsche
#8 ct-paths-1810350-7.patch21.78 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Title: get*Path() in should not be translation specific [Follow-up to Entity Translation UI in core] » EntityTranslationControllerInterface::get*Path() in should not be translation specific
Component: language system » translation_entity.module
Issue tags: +API clean-up
plach’s picture

Title: EntityTranslationControllerInterface::get*Path() in should not be translation specific » EntityTranslationControllerInterface::get*Path() should not be translation specific
Gábor Hojtsy’s picture

Component: translation_entity.module » content_translation.module
Pancho’s picture

Title: EntityTranslationControllerInterface::get*Path() should not be translation specific » ContentTranslationControllerInterface::get*Path() should not be translation specific
Pancho’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Assigned: Unassigned » plach

I will provide an inital patch for this.

plach’s picture

plach’s picture

Issue tags: +sprint
plach’s picture

Assigned: plach » Unassigned
Status: Active » Needs review
FileSize
21.78 KB

Here is an initial patch. The menu_base_path is retained for now as we need full conversion to the new routing system to remove it.

Status: Needs review » Needs work

The last submitted patch, ct-paths-1810350-7.patch, failed testing.

kfritsche’s picture

Assigned: Unassigned » kfritsche
FileSize
21.78 KB

Trying to get some fails down here now.
Attached rerolled patch.

kfritsche’s picture

--duplicate--

plach’s picture

Status: Needs work » Needs review

Thanks!

kfritsche’s picture

Assigned: kfritsche » Unassigned
FileSize
14.56 KB
33.58 KB

Okay took me a while but this patch should replaced all the rest of getBasePath, getEditPath, getViewPath. Was used in some tests and some other functions.
Also the custom block didn't used it at all.
Further added links to the annotations in the EntityTest Modules so that this is working correctly.

All test which failed should go green now. Hopefully nothing new breaks ;)

Status: Needs review » Needs work

The last submitted patch, ct-paths-1810350-13.patch, failed testing.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
3.19 KB
36.42 KB

Fixed the access problem in comments.
Added a new Test Entity for the URI so this doesn't conflicts with the translation things.

Now it should go green.

plach’s picture

Status: Needs review » Needs work

Awesome work really! We are just missing these minor things below and then we are good :)

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTranslationUITest.php
@@ -103,11 +103,11 @@ protected function assertPublishedStatus() {
+    $uri = $entity->uri('canonical');

Just $entity->uri() should be enough.

+++ b/core/modules/system/tests/modules/entity_test/entity_test.info.yml
@@ -7,4 +7,4 @@ core: 8.x
+hidden: false

Debug leftover :)

kfritsche’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
35.89 KB

Rerolled because of #1754246: Languages should be configuration entities got in.
Changed these two minor things mentioned by plach in #16.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Cool!

RTBC if green :)

plach’s picture

Removed some more obsolete docs, and marked what cannot be removed yet as deprecated. Functionally code is the same.

Berdir’s picture

Doc improvements look good to me, so keeping this at RTBC is fine.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

plach’s picture

FileSize
39.43 KB

Chasing head.

plach’s picture

FileSize
39.42 KB

Again

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, ct-paths-1810350-23.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

#23: ct-paths-1810350-23.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +translatable fields, +API clean-up, +D8MI, +sprint, +language-content, +RTBC July 1

The last submitted patch, ct-paths-1810350-23.patch, failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
39.43 KB
790 bytes

It was just a copy/paste problem: the new no-URI test entity-type had the wrong label. Setting back to RTBC as this was definitely minor.

Gábor Hojtsy’s picture

Issue tags: +API change

Make sure proposed API changes have the API change tag.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

Quoting from the current issue summary...

Proposed resolution

Specific solution needs to be proposed.

... seems like we have a solution... let's get an update :)

alexpott’s picture

Issue summary: View changes

EntityTranslationControllerInterface was renamed to ContentTranslationControllerInterface

plach’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Updated the issue summary, sorry for missing that. I hope it's ok to set this back to RTBC, anyone feel free to change the status if not.

plach’s picture

Issue summary: View changes

Updated issue summary

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.phpundefined
@@ -34,6 +34,10 @@
+ *     "canonical" = "block/{id}",
+ *     "edit-form" = "block/{id}/edit"

The edit-form is actually block/{id} too ... see #1983682: Convert applicable custom_block_menu() entries to use _entity_form in routing.yml

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
39.47 KB
672 bytes

...and fixed :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/content_translation/content_translation.pages.incundefined
@@ -23,9 +23,11 @@ function content_translation_overview(EntityInterface $entity) {
+  $base_path = $rel['translation-overview']['path'];

I think $base_path is not a great variable name because the entity annotations still have a base path key. Perhaps $translation_path is better?

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Plugin/Core/Entity/EntityTestNoUri.phpundefined
@@ -2,42 +2,39 @@
+ * Definition of Drupal\entity_test\Plugin\Core\Entity\EntityTest.

Should be EntityTestNoUri

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Plugin/Core/Entity/EntityTestNoUri.phpundefined
@@ -2,42 +2,39 @@
+ */class EntityTestNoUri extends EntityTest {

Needs a new line after */

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
39.53 KB
2.49 KB

Addressed #34.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/content_translation/content_translation.pages.incundefined
@@ -23,9 +23,11 @@ function content_translation_overview(EntityInterface $entity) {
+  $translation_path = $rel['translation-overview']['path'];

@@ -51,13 +53,13 @@ function content_translation_overview(EntityInterface $entity) {
+      $add_path = $translation_path . '/add/' . $original . '/' . $langcode;
+      $translate_path = $translation_path . '/edit/' . $langcode;
+      $delete_path = $translation_path . '/delete/' . $langcode;

Oh no! I've made it worse :( sorry plach... if we need this variable perhaps $base_translation_path?

However reviewing the code...

+++ b/core/modules/content_translation/content_translation.moduleundefined
@@ -80,16 +80,18 @@ function content_translation_entity_info_alter(array &$entity_info) {
+    // Provide default links for the translation paths.
+    $canonical = $info['links']['canonical'];
+    $info['links'] += array(
+      'translation-overview' => $canonical . '/translations',
+    );

+++ b/core/modules/content_translation/content_translation.pages.incundefined
@@ -23,9 +23,11 @@ function content_translation_overview(EntityInterface $entity) {
+  $rel = array();
+  foreach (array('canonical', 'edit-form', 'translation-overview') as $name) {
+    $rel[$name] = $entity->uri($name);
+  }
+  $translation_path = $rel['translation-overview']['path'];

@@ -51,13 +53,13 @@ function content_translation_overview(EntityInterface $entity) {
+      if ($translation_path) {

The if ($translation_path) { looks completely unnecessary as it is not possible for en entity not to have a translation-overview link. So I think this code can be simplified quite a bit...

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
39.74 KB
2.04 KB

This should look better now :)

webchick’s picture

Assigned: Unassigned » alexpott

Seems like Alex has been all over this one. :)

plach’s picture

FileSize
39.74 KB

Rerolled

plach’s picture

FileSize
38.57 KB

Rerolled again

Status: Reviewed & tested by the community » Needs work

The last submitted patch, ct-paths-1810350-40.patch, failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
39.34 KB

Better reroll

plach’s picture

Priority: Normal » Major

Promoting to major as this is heavily touching the Content Translation module API: the more we wait for this to be committed the higher is the likelihood that some module out here will break.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -222,35 +230,17 @@ class EntityType extends Plugin {
+   * @deprecated Define the "canonical" URI for the entity through "links"
+   *   property.
    */
-  public $menu_path_wildcard;
+  public $menu_base_path;

+++ b/core/modules/content_translation/content_translation.moduleundefined
@@ -234,7 +232,7 @@ function content_translation_menu_alter(array &$items) {
-        $edit_path = $info['menu_edit_path'];
+        $edit_path = $info['menu_base_path'] . '/edit';

We shouldn't be deprecating stuff and still using it in core. So we either need to open a followup to remove of do it here. I'm favour of the latter as it gets this closer to done.

plach’s picture

Status: Needs work » Needs review
FileSize
42.18 KB
10.27 KB

Ok, removed the menu_base_path key altogether. The current solution is a bit unreliable but it just has to work until we are done with the new routing system conversion.

Status: Needs review » Needs work

The last submitted patch, ct-paths-1810350-45.patch.patch, failed testing.

Gábor Hojtsy’s picture

Not passing :(

plach’s picture

Status: Needs work » Needs review

#45: ct-paths-1810350-45.patch.patch queued for re-testing.

plach’s picture

@Gabor:

Passing :)

plach’s picture

As discussed with @alexpott in IRC, I added a couple of @todos linking the follow-up issues which will allow to get rid of the current entity link/router path futzing, namely #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit and #1987882: Convert content_translation routes to a new style controller.

plach’s picture

Title: ContentTranslationControllerInterface::get*Path() should not be translation specific » Remove menu_* entity keys and replace them with entity links

More accurate title.

plach’s picture

I no longer can RTBC this one. Anyone available for a final review? Gabor?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks like all changes requested by @alexpott have indeed been taken care of well.

catch’s picture

This looks a bit like the sort of change I'd expect to see in #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items. I don't know if it complements or conflicts with that one though. Note in that issue there was discussion of using routes rather than paths.

pwolanin’s picture

@catch, yeah, the way these link are defined is still very problematic. Ideally we'd define all the entity routes based on these patterns, rather than duplicating them in the annotation and the route

Honestly, the pattern alone isn't something we have good code in place to handle, so I'd like to see something like the entity methods returning a route name and parameters instead of a path, possibly based on the content type (e.g. html vs JSON). This patch is probably a step in the right direction, but it will need follow-ups.

w.r.t. #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items another option there might be to make them plugins with automatic per-entity-type derivatives? Just haven't had time to get traction on that issue - I'm afraid it's likely to be a late API change.

plach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
41.92 KB

@catch:

I don't know if it complements or conflicts with that one though.

Mmh, I think that one would provide an even cleaner way to address this issue, AAMOF what we are doing here is defining a 'translation-overview' link, which makes much more sense as an operation rather than a relationship link. I am going to ask feedback in the other issue, hence moving back to NR until I get an answer.

Rerolling meanwhile.

plach’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
41.85 KB

Rerolled.

I am setting this back to RTBC as #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items is not moving atm. As pointed out by peter this may need a follow-up but at least we would be removing a deprecated API that has stayed in D8 core long enough.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, ct-paths-1810350-57.patch, failed testing.

kfritsche’s picture

I'm trying to get this green again.

kfritsche’s picture

Assigned: alexpott » Unassigned
Status: Needs work » Needs review
FileSize
43.43 KB
1.58 KB

In the ContentTranslationRouterSubscriber was still a use of menu_path_wildcard and menu_base_path.
This were replaced and the tests should work now.
Also manually tested if the translation of a node and a custom block is working.

@Plach: Feel free to review it again and set it to RTCB, if you are fine with my change. (see interdiff)

kfritsche’s picture

And an update. There was still something I tested in #59, why the delete translation tests will fail.

Here the update for it.

plach’s picture

The change looks the right thing to do, but it's very unfortunate we have to do all this path futzing. That clearly indicates there's still something wrong with our APIs. I really hope we can exploit #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items to solve this but I'm not actually sure.

I will try to have someone with more knowledge than me on the route/menu stuff to have a look to the patch to get some suggestions.

boran’s picture

Had a read through the mods and found no logical mistakes - the changes are consistent throughout the affected files.

As regards doc, a nit pick would be to keep the present tense (we dont need to converts to past tense because something is deprecated) and follow the comment guidelines more closely (https://drupal.org/node/1354).

boran’s picture

FileSize
1.36 KB

Used this as an excuse to learn to create an interdiff :-)

YesCT’s picture

@boran :) Let's work on this again tomorrow.

The interdiff is backwards

in comments name spaces should start with \ (an example in https://drupal.org/node/1354#file, and the standard in https://drupal.org/node/1353118 )

and....
we need the whole patch too. :)

boran’s picture

@YesCT:
- name space comment corrected.
- updated interdiff and patch

kfritsche’s picture

Did a last review of the patch and manual testing. Only some nit picking left, which I fixed.

Added a comment to the str_replace in the ConfigTranslationRouteController to explain why are we replacing the entity_type name with just {entity}.

Also noticed that in the entity_test and custom block entity the canonical link has no trailing slash, but all other canonical links in node, comment, term and user have a canonical link. Changed this too for consistency.

+++ b/core/modules/content_translation/content_translation.module
@@ -80,31 +80,27 @@ function content_translation_entity_info_alter(array &$entity_info) {
+      $info['links']['canonical'] = "/$entity_type/{$entity_type}";

Added curly brackets around the brackets. With only one curly brackets PHP thinks this is the Complex (curly) syntax and just adds $entity_type again, without the curly brackets around.

Manual testing:
* Clean install in german with patch
* Enabled content translation
* Added english as second language
* Enabling translation for article
* Created a article
* Translation overview: node/1/translations
* Added translation: node/1/translations/add/de/en
* Edited translation: node/1/translations/edit/en
* Deleted translation: node/1/translations/delete/en

All urls are reachable and are working.

As I worked on this issue before I'm not sure if I should RTBC it.

vasi1186’s picture

The code looks good to me, found nothing that should stop this to rtbc, from the code style point of view and implementation. But probably needs a review also from some people that were more involved in this issue before :)

plach’s picture

I'd still like to get some expert's feedback on the latest changes. I'll try to get @fubhy have a look to this.

Gábor Hojtsy’s picture

I asked fubhy (again) on IRC to post his feedback here.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

67: ct-paths-1810350-67.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 67: ct-paths-1810350-67.patch, failed testing.

plach’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
20.54 KB
45.57 KB

Rerolled, performed some additional clean-up and added a proper namespace to the new CT relationships. Hopefully we should good to go now for reals.

plach’s picture

+++ b/core/modules/content_translation/content_translation.module
@@ -497,9 +492,9 @@ function content_translation_set_config($entity_type, $bundle, $setting, $value)
+  $entity_info = entity_get_info($entity_type);

To do: this can go away now.

Status: Needs review » Needs work

The last submitted patch, 73: ct-paths-1810350-73.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
46.57 KB

Fixed local tasks test.

plach’s picture

Given that #2040379: Define contextual entity operations through annotation; remove the context property from hook_menu items was closed in favor of a solution dealing with contextual links and not operations, I think the way forward here is the current one. We should be ready for commit, reviews for the latest changes are welcome.

Gábor Hojtsy’s picture

Only two things stood out to me:

  1. +++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
    @@ -161,9 +161,17 @@ class EntityType extends Plugin {
    +   *
    +   * @deprecated The entire concept of field translation handler has been
    +   *   removed. This will no longer be used. Use the "translatable" property
    +   *   instead.
        */
    

    Should this not be removed, or would that be a different issue? It is not related to this issue scope BTW, so while I see the urge to document it, seeing it like it is is puzzling :D

  2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Entity/CustomBlock.php
    @@ -37,8 +37,10 @@
    - *   menu_base_path = "block/%custom_block",
    - *   menu_edit_path = "block/%custom_block",
    + *   links = {
    + *     "canonical" = "/block/{custom_block}",
    + *     "edit-form" = "/block/{custom_block}"
    + *   },
    
    @@ -177,19 +179,6 @@ protected function init() {
    -  public function uri() {
    -    return array(
    -      'path' => 'block/' . $this->id(),
    -      'options' => array(
    -        'entity_type' => $this->entityType,
    -        'entity' => $this,
    -      )
    -    );
    -  }
    -
    -  /**
    -   * {@inheritdoc}
    -   */
    

    HUGE DX WIN!

  3. +++ b/core/modules/content_translation/content_translation.module
    @@ -287,6 +284,24 @@ function content_translation_local_tasks_alter(&$local_tasks) {
    + * Convert an entity canonical link to a router path.
    + *
    + * @param string $link
    + *   The entity link to be converted.
    + *
    + * @return string
    + *   The resulting router path. For instance "/node/{node}" is turned into
    + *   "node/%node".
    + *
    + * @todo Remove this and use the actual link values when all the Content
    + *   Translation code is adapted to the new routing system.
    + */
    +function _content_translation_link_to_router_path($entity_type, $link) {
    +  $path = preg_replace('|{([^}]+)}|', '%$1', trim($link, '/'));
    +  return str_replace('%id', '%' . $entity_type, $path);
    +}
    +
    

    So why do we need the node/%node old type path? This type of path is archaic at best in the new system. I don't see anything left in this patch that would handle these style paths?!

plach’s picture

FileSize
3.28 KB
47.71 KB

Should this not be removed, or would that be a different issue?

We have #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation to complete the clean up.

It is not related to this issue scope BTW, so while I see the urge to document it, seeing it like it is is puzzling

Yeah, I know, but this was written just before the "code freeze" of the first of july. Do you think we should just remove it in the issue above?

So why do we need the node/%node old type path? This type of path is archaic at best in the new system. I don't see anything left in this patch that would handle these style paths?!

We still have content_translation_menu() and content_translation_menu_alter() with the related @todos. I removed the call in content_translation_entity_info_alter() though, plus another clean-up.

Status: Needs review » Needs work

The last submitted patch, 79: ct-paths-1810350-79.patch, failed testing.

Gábor Hojtsy’s picture

Yeah I think the first hunk should be delegated to #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation, not in the scope for this.

As for the old style paths, I see the hook_menu() relation now and conversion to route subscriber should fix that. There are already issues for that, and this is just a stop-gap. So I think that is fine then.

plach’s picture

Status: Needs work » Needs review
FileSize
950 bytes
47.05 KB

Ok, rerolled and removed that hunk

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to me now. Unifies DX nicely, no special properties only used by content translation. Fabulous!

catch’s picture

Title: Remove menu_* entity keys and replace them with entity links » Change notice: Remove menu_* entity keys and replace them with entity links
Status: Reviewed & tested by the community » Active

This is great. Committed/pushed to 8.x, thanks!

Will need a change notice update I think.

Gábor Hojtsy’s picture

I've been trying to find an existing change notice to update, but not sure which one would be sufficient... :/

plach’s picture

Gábor Hojtsy’s picture

Title: Change notice: Remove menu_* entity keys and replace them with entity links » Remove menu_* entity keys and replace them with entity links
Status: Active » Fixed
Issue tags: -sprint

I don't think that is relevant, it does not talk about any annotation stuff :) Also I found https://drupal.org/node/1827470 but no mention of content translation annotations there either.

Created this new change notice: https://drupal.org/node/2133913

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.