Installing Content Translation module breaks Rest resources

Here's a test only patch to prove the bug. I opted to simply installing Content Translation in the respective rest resource base classes instead of adding dedicated tests as there really is no translation-related functionality that we are testing here. It's simply about the fact that the resources work as before even with Content Translation enabled. But I guess this is arguable and could be changed, as well.

Didn't check why the language cache context is bubbled in the resources, but it doesn't seem problematic to me.

And here's the fix. Stole the comment in the YAML file from the Views UI implementation in case anyone is wondering.

The last submitted patch, 2: 3368071-2.patch, failed testing. View results →

@SenthilMohith thank you for the interest but shouldn't assign tickets to yourself unless a maintainer per https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett... →

queuing for 11.x tests.

Tests all green.

Also ran a test locally without the fix file and the testGet() failed with

Drupal\Component\Plugin\Exception\PluginNotFoundException : The "drupal:content-translation-overview" plugin does not exist. Valid plugin IDs for Drupal\Core\Http\LinkRelationTypeManager are: add-form, add-page, delete-form, delete-multiple-form, revision, revision-revert-form, revision-delete-form, create, enable, disable, edit-permissions-form, overview-form, reset-form, cancel-form, flush-form, duplicate-form, about, alternate, appendix, archives, author, blocked-by, bookmark, canonical, chapter, collection, contents, copyright, create-form, current, customize-form, derivedfrom, describedby, describes, disclosure, dns-prefetch, duplicate, edit, edit-form, edit-media, enclosure, first, glossary, help, hosts, hub, icon, index, item, last, latest-version, license, lrdd, memento, monitor, monitor-group, next, next-archive, nofollow, noreferrer, original, payment, pingback, preconnect, predecessor-version, prefetch, preload, prerender, prev, preview, previous, prev-archive, privacy-policy, profile, related, replies, search, section, self, set-default, service, start, stylesheet, subsection, successor-version, tag, terms-of-service, timegate, timemap, type, up, version-history, via, webmention, working-copy, working-copy-of, entity-permissions-form

Think this is good.

The last submitted patch, 3: 3368071-3.patch, failed testing. View results →

Random failure, back to RTBC

+++ b/core/modules/content_translation/content_translation.link_relation_types.yml
@@ -0,0 +1,14 @@
+drupal:content-translation-overview:
...
+drupal:content-translation-add:
...
+drupal:content-translation-edit:
...
+drupal:content-translation-delete:

Why are these prefixed with drupal:? No other Drupal-specific link relations have this prefix.

True. Moving to NW to update links.

First of all, not sure that's true. canonical, edit-form, etc. are all standard IANA link relation types. The idea was to use the drupal: prefix for all custom ones. We may not have followed through on that correctly, not sure off the top of my head.

But regardless, this is not establishing the content translation link relation types, they already exist and are being used. They are just missing their declaration. And Rest module happens to ve particularly strict about that.

So moving back to needs review as I'm not sure what else could be done here.

Any response @longwave or @smustgrave? Per the above I don't think the criticism of the patch was warranted. I also took another look at the issue summary and I think the situation is explained sufficiently there, as well. Not going to re-RTBC myself because people are very sensitive with that, but maybe either of you can confirm and do so? Would be much appreciated.

Going to put in committer bucket to get more eyes on it.

Not sure I could make the call.

Great, thanks!

I'm triaging RTBC issues → . I read the IS and the comments.

Thanks for answering longwave's query. Due to the adding of the prefix, this should have subsystem maintainer review first (I checked with xjm)

Oh I see:

        if (!$entity_type->hasLinkTemplate('drupal:content-translation-overview')) {
          $translations_path = $entity_type->getLinkTemplate('canonical') . '/translations';
          $entity_type->setLinkTemplate('drupal:content-translation-overview', $translations_path);
          $entity_type->setLinkTemplate('drupal:content-translation-add', $translations_path . '/add/{source}/{target}');
          $entity_type->setLinkTemplate('drupal:content-translation-edit', $translations_path . '/edit/{language}');
          $entity_type->setLinkTemplate('drupal:content-translation-delete', $translations_path . '/delete/{language}');
        }

So we already declare these link templates - this code has existed since 8.0. But we didn't define the metadata for them, which REST module requires. Changing these now would break backward compatibility with code that already uses them, which is out of scope of this issue.

Re #16: Again, this is not adding a prefix. It's just formally declaring, i.e. in a sense documenting, something that is already used. Subsystem maintainer review never hurts, so I have no issue with adding that tag, but wanted to point that out.

Re #17: Exactly. I tried to explain this in the issue summary, but maybe I didn't do a super good job. But that is exactly the point.

The last submitted patch, 3: 3368071-3.patch, failed testing. View results →

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Yup, back to RTBC then. Just pushed the patch to an MR, so the RTBC from #14 still applies.

OK @tstoeckler's explanation means we don't need subsystem maintainer review, just a misunderstanding about the links already existing (which I also would have done probably).

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch → closed merge request !8726

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