[PP1] Switch Node revision UI to generic UI

Component should probably be "node system" in this case, since this issue will primarily impact the node module.

This is at least postpone by 2 if not 3.

• ✨ Implement a generic revision UI Fixed .
• #3043321: Use generic access API for node and media revision UI → .
• Unlikely on 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work .

I believe 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work could be done in parallel and wouldn't be a hard blocker because ✨ Implement a generic revision UI Fixed provides a separate route provided dedicated to providing revision routes.

Yes, but both this issue and 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work will be deperecating some route defined in node.routing.yml which mean both issues are blocked on 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work .

Yep that makes sense. So would that be [PP-3] now or still [PP-2], since yeah technically this issue is postponed on 3 issues, but as you said 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work depends on 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work anyway.

Here is a WIP patch based on #2350939-134: Implement a generic revision UI → , #2730631-140: Upcast node and node_revision parameters of node revision routes → , #3043321-56: Use generic access API for node and media revision UI → and #2723579-94: [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider → . Not ready for CI testing.

Padded summary.

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.1.0-alpha1 → will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule → and the Allowed changes during the Drupal 9 release cycle → .

Here's a reroll of #7 on top of ✨ Implement a generic revision UI Fixed (patch 134)

#3043321: Use generic access API for node and media revision UI → was committed, so this is not longer blocked on that issue; However it should probably be blocked on #3045358: Node module revision permission IDs are too generic → .

This issue also needs to be resolved first 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work , so PP-3.

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

So to summarise, this issue is blocked on:

• ✨ Implement a generic revision UI Fixed
• #3045358: Node module revision permission IDs are too generic →
• 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work

But it would be nice to also have 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work done first, not making it a hard blocker though, as the current implementation in ✨ Implement a generic revision UI Fixed uses a separate route provider.

Rerolled #9 on top of #202 ✨ Implement a generic revision UI Fixed of ✨ Implement a generic revision UI Fixed

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

The latest patch in ✨ Implement a generic revision UI Fixed adds this:

+/**
+ * Implements hook_local_tasks_alter().
+ */
+function node_local_tasks_alter(&$local_tasks) {
+  // Removes 'Revisions' local task added by deriver. Local task
+  // 'entity.node.version_history' will be replaced by
+  // 'entity.version_history:node.version_history' after
+  // https://www.drupal.org/project/drupal/issues/3153559.
+  unset($local_tasks['entity.version_history:node.version_history']);
+}

We should probably do something about this. I imagine the cleanest way would be to deprecate the entity.node.version_history local task and at the same time deprecate this hook implantation so that they are both removed.

MR + patch on top of #2350939-268: Implement a generic revision UI → #268

Is there a rule where to put the RouteProvider class? Core seems to have a mess. Would be nice to unify where these files should go, and use the correct directory in this patch.

content_moderation\src\Entity\Routing\EntityModerationRouteProvider.php
media\src\Routing\MediaRouteProvider.php
node\src\Entity\NodeRouteProvider.php
taxonomy\src\Entity\Routing\VocabularyRouteProvider.php
user\src\Entity\UserRouteProvider.php

should it be src\Entity\ or src\Routing\ or src\Entity\Routing\ directory?

Adding info from #2350939-261: Implement a generic revision UI → + #2350939-277: Implement a generic revision UI → re usability review of main entity revision UI issue, especially comparing existing node functionality vs work-in-progress MR. More detail/context is provided these linked comments.

When node to generic UI, we should revisit whether its necessary to retain the yellow background. It was introduced way back in #42072: Rework revisions overview screen → , there doesnt seem to be much foundation/rationale for keeping it exactly as is. So we can decide whether to retain/rework/remove, and whether to move the styling to the theme, especially Claro.

We should also port the "Use admin theme" checkbox provided by node to entity generic, in system.module or similar. Specifically, move config schema, migrate existing node config, to system. Then update \Drupal\Core\Entity\Routing\RevisionHtmlRouteProvider::getVersionHistoryRoute to pull from this config.

Rebased onto and updated style to match ✨ Implement a generic revision UI Fixed

Can we test what this does to the diff module?

Since ✨ Implement a generic revision UI Fixed is fixed, this can be made active again I think.

It needs a way to deprecate routes, so next blocker is 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work

@andypost

My understanding with this is we don't actually need to deprecate routes, we can update the new NodeRevisionRouteProvider in the MR to use the same route ID's as found in existing node.routing.yml.

entity.node.revision and entity.node.version_history are fine as RevisionHtmlRouteProvider will generate the same IDs.

However RevisionHtmlRouteProvider has entity.$entityTypeId.revision_revert_form and entity.$entityTypeId.revision_delete_form for the revert and delete routes. Whereas the existing node routes are node.revision_revert_confirm and node.revision_delete_confirm.

We can override \Drupal\Core\Entity\Routing\RevisionHtmlRouteProvider::getRoutes with the legacy ID's, and if we really want can deprecate them. But its not really a requirement as far as I can tell.

Or is there something else I'm missing?

@dpi thank you! it makes sense but moreover except of routing it needs permission fix so at least one blocker #3045358: Node module revision permission IDs are too generic →

the second is more about polishing 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work

I agree with @dpi let's not add a scope creep of deprecating the routes we can do that in 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work . Let's keep it a follow up task to ✨ Implement a generic revision UI Fixed .

📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work should make deprecating routes possible.

Long-term it would be good if the node module followed the accepted pattern for route names, we could open a follow-up issue to this one and block 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work on it.

Though we aren't blocked on 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work as revision provider is a totally different class.

What are we actually postponed on?

permission providers also separate issue 📌 Introduce entity permission providers Needs work

Since it's not clear what exactly is blocking this and what might be done in parallel, going to tentatively unpostpone - then if we do hit a hard blocker we can always re-postpone again.

Though we aren't blocked on 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work as revision provider is a totally different class.

Agreed, I was thinking that in this issue we could just keep the route names the same as they are right now, then open a follow-up issue to change all the node route names which don't conform to the standard once 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work is done (so not just the revision routes). We could either postpone that follow-up issue on 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work , or the other way round, or maybe even do them in parallel.

Latest commit should fix some of the revert failures (although very well may break other things). The local task failures are beyond me, those unit tests are voodoo magic.

In the MR you say that entity.node.version_history is replaced but seems to not be getting picked up?

@smustgrave it is being picked up, the local task unit tests are failing for other reasons. Manually testing the patch shows the revisions route.

Was not meant to change back to NR - @VladimirAus are you going to be working on this?

Latest commit should fix the local task tests. The theme handler is needed for the other local task deriver.

@acbramley I'm testing it...

Awesome! Thanks :)

Pushed along some more failures, generally speaking the Node revision tests are a bit of a mess and there seems to be a lot of duplication across different test classes. I think this is going to take someone that knows the ins and outs of translations and the rest of it to really figure out how we are going to roll this out so it's not so disruptive. The functionality is changing quite a bit.

I've also added a pager to the generic revision UI controller as we were missing that and some of the Node based tests depended on it, I think it's a good idea to add one in any case.

I've also added a pager to the generic revision UI controller as we were missing that and some of the Node based tests depended on it

I wonder if it would be better to do that in a separate issue, along with any changes to the generic implementation to support Node moving to it, then this issue is well scoped to only the changes needed in the Node module itself.

Fixed a few but still can't figure out what's wrong.

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Lets get a screenshot comparison of the patch-to-date versus core as-is.

Do we need to deprecate \Drupal\node\Controller\NodeController::revisionOverview in some way?

🐛 Revision UI reverts all language translations when only one language is reverted Active should be fixed before switching to new revision UI to maintain existing behavior for reverting revisions of node translations.

Added ✨ Add pagination to VersionHistoryController Active for #48

Blocked on 🐛 Revision UI reverts all language translations when only one language is reverted Active

I'd say this would also be blocked on ✨ Add pagination to VersionHistoryController Active

Re #52 I have marked that postponed, I can't reproduce the issue.

Now I think this is only blocked by 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work

Adding what this is postponed on to the remaining tasks per Remining tasks → .