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.

• #2350939: Implement a generic revision UI.
• #3043321: Use generic access API for node and media revision UI.
• Unlikely on #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider.

I believe #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider could be done in parallel and wouldn't be a hard blocker because #2350939: Implement a generic revision UI provides a separate route provided dedicated to providing revision routes.

Yes, but both this issue and #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider will be deperecating some route defined in node.routing.yml which mean both issues are blocked on #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name.

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 #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider depends on #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name 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: NodeRouteProvider should extend DefaultHtmlRouteProvider. Not ready for CI testing.

Here's a reroll of #7 on top of #2350939: Implement a generic revision UI (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: [PP-1] Node module revision permission IDs are too generic.

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

So to summarise, this issue is blocked on:

• #2350939: Implement a generic revision UI
• #3045358: [PP-1] Node module revision permission IDs are too generic
• #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name

But it would be nice to also have #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider done first, not making it a hard blocker though, as the current implementation in #2350939: Implement a generic revision UI uses a separate route provider.

Rerolled #9 on top of #202 of #2350939: Implement a generic revision UI

The latest patch in #2350939: Implement a generic revision UI 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.

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?

Can we test what this does to the diff module?

Since #2350939: Implement a generic revision UI is fixed, this can be made active again I think.

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

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

the second is more about polishing #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider

I agree with @dpi let's not add a scope creep of deprecating the routes we can do that in #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider. Let's keep it a follow up task to #2350939: Implement a generic revision UI.

#3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name 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 #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider on it.

permission providers also separate issue #2809177: Introduce entity permission providers

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 #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider 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 #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name is done (so not just the revision routes). We could either postpone that follow-up issue on #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider, 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.

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

Added #3384725: Add pagination to VersionHistoryController for #48

Blocked on #3374607: Revision UI reverts all language translations when only one language is reverted

I'd say this would also be blocked on #3384725: Add pagination to VersionHistoryController

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

Now I think this is only blocked by #2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider

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

#2723579: NodeRouteProvider should extend DefaultHtmlRouteProvider got in, so this is no longer blocked.

I created a new branch & merge request for Drupal 11.x. Basically just a reroll with few changes to legacy routes.

On issue summary:
- Added a remaining task for Diff module. Probably we don't want to remove the routes, but deprecate them, otherwise current Diff module users would face a fatal error on node/{nid}revisions route, due to https://git.drupalcode.org/project/diff/-/blob/2.x/src/Form/RevisionOver...
- Marked the "Use the administration theme when editing..." checkbox refactoring as Out-of-scope, since this issue is already rather hard to complete.
- Keeping or removing the yellow background for first row, would need a decision. Since there's no strong argument for keeping it, I'd suggest we remove it (currently on MR).
- Added screenshots with yellow background removal.

Fixed PHPCS. Now there are a few tests failing. One is in NodeRevisionsTest.php. It concerns a missing 'revert' button so seems related to the issue. Though looking at the screenshots in the IS does not seem to be any removal of 'revert' buttons going on.

Removed.

@oily because I get the emails. If you are going to post the test-only run it should also be with a code review please. Just running the test job and posting the results will not result in credit if that's the end goal and I've got a number of emails of just the posting. Again if you see me do 9/10 times I won't get credit either so just FYI

@oily we don't need the test-only job here, it's only useful for bug reports as it demonstrates the bug existed in HEAD and is fixed in the MR. Also, pasting its output only pollutes the issue and leads to more scrolling. Better to link to the job output if it's relevant.

Re: #72 @smustgrave You told me that before and I heard you. As far as credit goes, you can see at #70 that I have already fixed PHPCS and as a result the pipeline has moved forward to the tests and lo ahd behold! several tests are failing. So they need to be fixed. Maybe not a huge amount of progress but progress, nevertheless? Or maybe not? How do I know? If I could wave a magic wand and become an expert contributor overnight with 500 credits not 66 credits, maybe i could contribute more to this issue or do what I do at a faster rate. I have waited 48 hours for @sokru to PHPCS his own work. (another rule I was told about?). I see that test-only was a wrong move,now. But I was trying to do a bit more that just fix PHPCS. I was hoping to fix at least one of the broken tests, too. But the test-only test set me off on the wrong path. Thanks for putting me right on that!

The last failure is in NodeRevisionsUiTest::testNodeRevisionsTabWithDefaultRevision due to a behaviour change between the 2 revision overview controllers.

In NodeController::revisionOverview we have this:

// We treat also the latest translation-affecting revision as current
// revision, if it was the default revision, as its values for the
// current language will be the same of the current default revision in
// this case.
$is_current_revision = $revision->isDefaultRevision() || (!$current_revision_displayed && $revision->wasDefaultRevision());

Whereas in VersionHistoryController::buildRow we just check
$revision->isDefaultRevision()

We need to decide whether we want the behaviour change for Node, or for every other entity type using RevisionHtmlRouteProvider

testRevisionTranslationRevert is also failing which uses another node specific revision route node.revision_revert_translation_confirm.