Remove ContentPreprocess and associated code

I think we should only do this if we update the Node entity annotation to include the 'latest-revision' link template. This is currently added to entity types in \Drupal\content_moderation\EntityTypeInfo::addModerationToEntityType.

ok, I mean adding the whole shebang to core / node.

Seems a bit odd to add a check for a route in node.module which only exists if content moderation is enabled.

Looks good to me but node_is_page() test needs adding or updating, and maybe deprecating in favour of a service if that makes it more testable?

This has some overlap with #3005029: Make node_is_page() work on revisions and previews as well. That's also tagged as needing tests, but from what I can tell, none of the procedural code in node.module has any dedicated tests.

This function would be very easy to unit test, but I don't know if there's a standard for testing global functions via unit tests. Failing that, it would need to be a kernel test (a functional test seems like overkill for such a simple method.)

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-roll patch need to 11.x.

Rerolled for 11.x.

This will need test coverage. Why is the one test being removed?

The test that was removed was specifically testing the CM preprocessor which was removed so the test was no longer needed. Instead I've repurposed it to test the node_is_page function.

I was in 2 minds about doing this or #3515218: [Regression] Restore functionality of node_is_page, generalised to content entity types first but this might be easier to get in first.

Alternatively if we make it a service I guess CM could decorate it to add the condition to its own route 🤔

Can we update the issue summary to reflect the new scope and findings please.

Hiding patches

@smustgrave what exactly is missing from the IS? It explains exactly what we're doing?

left some comments, still marking as the changes to the test didn't break.

I don't think we should hardcode this. How about we add something to route so we have an API here that other modules could use.

Something like
if ($route_match->getRouteName() == 'entity.node.canonical' || $route_match->getRouteObject()->getOption('node.is_full_page_view')) {

And then in \Drupal\content_moderation\Entity\Routing\EntityModerationRouteProvider::getLatestVersionRoute() do ->setOption($entity_type_id . '.is_full_page_view', TRUE) on the route it returns. This way any entity that has similar needs to node can do things in the same way.

We should also update the docs in template_preprocess_node() as the following is not correct...

  // The 'page' variable is set to TRUE in two occasions:
  // - The view mode is 'full' and we are on the 'node.view' route.
  // - The node is in preview and view mode is either 'full' or 'default'.

We're actually trying to deprecate this variable entirely so I guess this should be postponed #3458589: Deprecate $variables['page'] for node.html.twig and node_is_page()

I do like the idea in #35 though!

Confirming we can get rid of all of this code once #3458589: Deprecate $variables['page'] for node.html.twig and node_is_page() is done, however CM doesn't seem to have any functional test coverage of what this was doing (i.e hiding the duplicate h2 heading on the latest version page).

I think in this issue we should add that test coverage. I've opened an MR to remove the associated code and will add the test coverage which will fail until 3458589 is merged and this is rebased.

This looks good, but as I commented as in the parent issue, we can only do this in D13 when the page variable actually gets removed, otherwise templates relying on the BC behavior would be broken with content moderation?

Should we split this up? Get the test coverage in now and then the removal later? We could do one postponed page variable cleanup issue and write down all the parts that can go then, because there's a lot and we might miss some bits otherwise (node itself + views/content moderation special cases + tests)

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

initially was going to repurpose this issue but I think it's cleaner if we open a separate one for the test coverage

#3567137: Add test coverage for duplicate node titles on content_moderation latest version page

Beautiful. Note that the functional test coverage that we added in #3567137: Add test coverage for duplicate node titles on content_moderation latest version page remains and ensures that the result is unchanged.

Committed 8f36150 and pushed to main. Thanks!
Committed 46deaa3 and pushed to 11.x. Thanks!