Deprecate $variables['page'] for node.html.twig and node_is_page()

Please see https://www.drupal.org/node/3334622 and #3270148: Provide a mechanism for deprecation of variables used in twig templates for how to deprecate Twig variables.

I realised this wasn't documented in the deprecation policy so also opened #3459205: Document how to deprecate Twig variables to fix that.

Hi

It looks much better. The patch is applied cleanly and I even searched in the Drupal code base for $variables[‘page]. I did not find it. So for me, it looks good.

RTBC ++

There are failing tests due to get deprecation messages from this deprecation.

The variable is still in use and it looks like it needs to be set to TRUE or FALSE. Can this really be deprecated or do I just not know something?

Thanks for updating IS @catch

The 'page' template variable in node templates duplicates 'view_mode' = full.

But 'page' is set to a boolean, how can that be the same as 'view_mode' = full?

Seems to cause valid test failures in tests.

There are two failing tests. In both of those the page variable is 'false' and the view_mode is 'full'. So, the $page boolean is not the same thing as the full view mode as stated in #15.

The tests are

• core/modules/taxonomy/tests/src/Functional/Views/TermTranslationViewsTest.php
• core/modules/node/tests/src/Functional/NodeTitleTest.php

Right, so the MR is completely wrong. Is this is about changing a confusing name or a need to introduce more variables?

The key 'page' is documented for node and clearly states that is is not the same as full mode. And taxonomy also uses 'page' is a similar, though simpler way. It uses $variables['page'] = $variables['view_mode'] == 'full' && taxonomy_term_is_page($term);. Would a change be needed there as well?

I tried to debugged why : core/modules/taxonomy/tests/src/Functional/Views/TermTranslationViewsTest.php its faling and that is because the method : taxonomy_term_is_page($term) is checking for the route \Drupal::routeMatch()->getRawParameter('taxonomy_term') and the test is runing on a view that has a contextual filter argument as : arg_0 .

So , the above check $variables['page'] = $variables['view_mode'] == 'full' && taxonomy_term_is_page($term); will always be false , since we are relying on view_mode so this change is confusing.

@deepakkm and @vakulrai, did you read comment, #18, which starts with, "the MR is completely wrong"? The work to do here is to decide on the resolution, which is shown in the issue summary as 'unknown'.

Adding related issue as I think replacing this variable with the view mode check would fix that issue as well? Right now we only set page to true if we're on the canonical route (or if we're in preview and also viewing the full/default view mode).

Also, in regards to #21, to add to what @catch said and make it explicit: The MR is definitely not "completely wrong". The MR is exactly where we want to go (as an intermediate step at least). It *is* a behavior change in some cases and that's probably OK, so if we have tests for those edge cases, the most likely path forward is to either to adjust those tests or even remove them if they no longer test something meaningful.

We should also do some manual tests and document the differences and then decide if there's something we want to do about. Specifically around term pages, with and without views and node previews and latest version.

Rebased, removed taxonomy changes.

My proposal is to extend the scope of this issue to "Deprecate the *concept* of the node is own page check". Including the node_is_page() function. What we want to say is that within rendering, we no longer want to make a distinction between node being on its own page and the full view mode. If you use that, you get the same output as on the node detail page.

I did test node preview, that's fine because template_node_process() specifically checks that.

However, I had a look at NodeTitleTest and it does show an interesting problem. It's actually testing comment module, specifically the comment reply route that shows the entity your commenting on, hardcoded to the full view mode. A behavior that I find quite confusing on drupal.org as it can be quite long. And with this, the node title no longer shows on that page at all, but at the same time, you won't see the other comments.

The challenge is that you can't really know whether or not a view mode exists, whether or not it shows the title.

As a quickfix, I'd suggest that we convert the current static "Add a comment" title to a callback that does something like "Add a comment to %entity_title". As a follow-up, I think it might make sense to make this behavior configurable per comment type either a view mode or disable to skip entirely?

Also, I did find another case where this makes a difference and IMHO it's a clear improvement. The revision page, which currently on HEAD shows the title twice and now only once:

HEAD

With this MR

#31 is a massive improvement! Also solves #3431659: Duplicate title on node revision page

Closed #3515218: [Regression] Restore functionality of node_is_page, generalised to content entity types as a dupe of this

Cloned locally to check the test failure (NodeTitleTest::testNodeTitle) as explained in #29 we lose a little bit of context on the comment reply form, however the HTML makes a lot more sense.

This is the output on HEAD:

You can see the h1 (Add a new comment) is below the h2 (the node title) which is anti-a11y.

This is the output on this branch:

We still get the node title in the breadcrumbs but not in an h2. There is still an h2 on the page but I have no idea what is populating it (it's not the user name or node title)

EDIT: The h2 is the label of the breadcrumb block that's randomised in BlockCreationTrait::placeBlock

That's a very good example why I really don't like random strings in tests. Little to zero benefits and it just makes debugging and understanding things harder :)

Thanks for manually testing, didn't get to that yet. So that example would become "Add new comment to @title".

That's a very good example why I really don't like random strings in tests

Yes I thought of you when I was debugging this lol.

Should this be NW for the comment title callback? Or are we doing that in a followup?

I added the title callback.

Updated the IS with the UX and API changes, also updated the test to remove the pesky random title.

From my pov this is RTBC but I think I've been too involved to do it myself.

For the comment screenshot, you picked a version that is a reply to another comment, that hasn't changed as much and it doesn't show the difference to the node title (it shows either the comment you reply to or the node, not both)

From my pov this is RTBC but I think I've been too involved to do it myself.

FWIW, I think there are two aspects to this.

One is whether or not you personally feel OK with doing that, which I perfectly understand, that's entirely up to you.

The other part is the official rules around this. @catch recently relaxed/clarified that a bit: https://www.drupal.org/node/3156237/revisions/view/13965063/13985848. So, having worked on this is actually a good thing as you're very familiar with the changes and you can still RTBC as long as it wasn't *only* you.

Well, we will be able to remove that once we actually remove the flag, yes. Actually all of it then, because the other half is already deprecated.

Setting to needs work because I think we should have a BC test here, I'm not sure if the deprecation also works for flags that are checked in a condition and we want to make sure the flag is set correctly. And I think this should also have an issue in upgrade_status/phpstan-drupal or so with a dedicated check. There are going to be a ton of node templates out there that will suddenly start to display duplicated titles when we no longer set this and they will not see the deprecations as that's only in tests (#3024296: Add option to log deprecation errors would help though)

Added test coverage

looks good to me

Left a comment on missing DI, otherwise +1 to RTBC from me, the deprecation test looks good and verifies that an if condition with a variable triggers the deprecation message, great.

MR had failures and wasn't rebasing cleanly so I've squashed and rebased against 11.x

> Instead of checking for $page, check if we're on the full view mode.

Knowing you're on a full page for an entity is useful. Could we add something generic for all entity types for that?

being a bit late here, but $page still may have a point distincting whether to print the title for a given view mode. This is now gone.

Especially this affects a case where you might not want to create a custom (sub) theme just to change that behaviour (for example using the admin theme), previously you could do so by using preprocess hook to change $page. This does not have effect anymore and changing the view mode in preprocess is probably no good idea ...

It seems a workaround for this case might be to enable the visibility configuration for the title field, like so:

/**
 * Implements hook_entity_base_field_info_alter().
 */
function mymodule_entity_base_field_info_alter(&$fields, \Drupal\Core\Entity\EntityTypeInterface $entity_type) {
  if ($entity_type->id() === 'node' && isset($fields['title'])) {
    $fields['title']
      ->setDisplayConfigurable('view', TRUE)
      ->setDisplayOptions('view', [
        'region' => 'hidden',
      ]);
  }
}

(trying to unset label in preprocess did not work out)

It seems that with #2353867 it is planned to have this configurable yourself without hooking, but this still has not landed.

With configurable title field you can decide yourself, whether title is printed in given view mode, not relying on core's current definition saying title is printed in every view mode except full.