Problem/Motivation

The routes for viewing a node revision should render via the API just as regular entity canonical routes

Proposed resolution

I think the route that we need to support is this one:

entity.node.revision:
  path: '/node/{node}/revisions/{node_revision}/view'
  defaults:
    _controller: '\Drupal\node\Controller\NodeController::revisionShow'
    _title_callback: '\Drupal\node\Controller\NodeController::revisionPageTitle'
  requirements:
    _access_node_revision: 'view'
    node: \d+
CommentFileSizeAuthor
#15 3227653-15-node-revision-ce.patch5.04 KBaludescher
#9 3227653-9-node-revision-ce.patch4.76 KBaludescher
#6 3227653-6-node-revision-ce.patch3.85 KBaludescher
#3 3227653-3-node-revision-ce.patch4.98 KBaludescher
#2 3227653-2-node-revision-ce.patch5.03 KBaludescher

Comments

fago created an issue. See original summary.

aludescher’s picture

aludescher commented
StatusFileSize
new 3227653-2-node-revision-ce.patch5.03 KB
aludescher’s picture

aludescher commented
StatusFileSize
new 3227653-3-node-revision-ce.patch4.98 KB
fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Active » Needs review
fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

thx, here a review

  • please set issues with patch to needs-review, so it's clear they are waiting for review
  • it seems weird that the ::entityRevision() controller has to do magic to load parameters from the route. that's not how the routing system is usually working, we should be able to take entity_revision route parameter from the route, no? core also works without that custom loading code: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

+ // The 'node' parameter is only a node id string for revision routes.
+ $node = $route_match->getParameter('node');

Weird, not nice to do all those special casing here for "node" - does this mean our metatag support would not work for otehr entity routes? I guess we should file a bug ticket and improve that in a separate ticket if so.

aludescher’s picture

aludescher commented
StatusFileSize
new 3227653-6-node-revision-ce.patch3.85 KB

> it seems weird that the ::entityRevision() controller has to do magic to load parameters

Good point, I updated the controller.

> Weird, not nice to do all those special casing here for "node" - does this mean our metatag support would not work for other entity routes?

Yes, it seems so. Maybe it shouldn't even be addressed here but this change is needed to make node revisions work.

We could also add `entity.media.revision: path: '/media/{media}/revisions/{media_revision}/view'` and/or a new route for all entity types.

aludescher’s picture

aludescher commented
Status: Needs work » Needs review
mostepaniukvm’s picture

mostepaniukvm
he/him
commented
Version: 8.x-1.x-dev » 2.x-dev
Status: Needs review » Needs work

Hi Anton,
Changes look good. It works for entity.node.revision route but I think we also need that for entity.node.latest_version route.

I don't really like to have node-specific implementation but let's finish it like that.
I created a follow-up issue so we can solve this problem there: #3304320: Re-implement node specific code to work with any entity types

aludescher’s picture

aludescher commented
StatusFileSize
new 3227653-9-node-revision-ce.patch4.76 KB

Thanks Vova,
I added entity.node.latest_version.
Note: permissions 'view latest version', 'view any unpublished content' are still required for testing CE output.

aludescher’s picture

aludescher commented
Status: Needs work » Needs review
mostepaniukvm’s picture

mostepaniukvm
he/him
commented

Patch looks good, I will do manual testing soon

fago’s picture

fago
Primary language German
Location Vienna
commented

not sure about this change, what does that do?

@@ -375,6 +380,7 @@ class CustomElementsRenderer {
foreach ($metatag_values['#attributes'] as $value) {
if (in_array($value, $blacklisted_metatags[$metatag_values['#tag']])) {
$blacklisted = TRUE;
+ break;

also those hunks seem to interfere with #3312042: Alternate-link meta tag is missing when using content translation, maybe let's merge the branch from there into this and re-roll the patch?

mostepaniukvm’s picture

mostepaniukvm
he/him
commented
Status: Needs review » Needs work

As changes in #3312042: Alternate-link meta tag is missing when using content translation already merged we definitely need to re-roll the patch.
Meanwhile I tested and get api response fro both revision and latest revision routes. Paths: /api/node/*/latest, /api/node/*/revisions/*/view
What is unexpected that in both cases I got all the meta values but expected empty array.
> // The 'node' parameter is only a node id string for revision routes.
Are you sure that this is really true, or maybe it was changed in resent version of drupal?
In general it's not hurt a lot but if it's not really needed I wouldn't add that change.

fago’s picture

fago
Primary language German
Location Vienna
commented

> > // The 'node' parameter is only a node id string for revision routes.
not sure either, it depends on the route slug names used I suppose, if it uses "node" it's there and works.

I think we should follow traditional drupal behaviour here and only add all the metatags if drupal already does it like that, else we should not.

aludescher’s picture

aludescher commented
StatusFileSize
new 3227653-15-node-revision-ce.patch5.04 KB

Re-rolled without the (now) unnecessary 'node' parameter part.

Also changed return type of CustomElementsMetatagsGeneratorTrait::setCeMetagasGenerator() from static to self in order to fix the following blocking error:

Uncaught ParseError: syntax error, unexpected 'static' (T_STATIC) in .../lupus_ce_renderer/src/CustomElementsMetatagsGeneratorTrait.php:25

`static` only works in PHP >= 8, see https://php.watch/versions/8.0/static-return-type
Drupal 9 requires PHP >=7.3.0 which should be supported unless we change the module to require PHP >= 8.

aludescher’s picture

aludescher commented
Status: Needs work » Needs review
mostepaniukvm’s picture

mostepaniukvm
he/him
commented
Status: Needs review » Reviewed & tested by the community

It looks good now. Tested locally works correctly. I think we can merge it now.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Reviewed & tested by the community » Fixed

great, merged then.

  • fago committed bf91803 on 2.x 
    Issue #3227653 by aludescher, fago, mostepaniukvm: Add support for...

Status: Fixed » Closed (fixed)

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