Deprecate $variables['page'] for node.html.twig

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

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

ankitv18 → changed the visibility of the branch 3458589-deprecate-variablespage-for to hidden.

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?

Updated the issue summary.

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.

@quietone it's duplicating in the sense that the $page boolean means the same thing as the full view mode. There is some additional logic in setting it, but that logic should not exist at all, just makes it confusing.

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

So, the $page boolean is not the same thing as the full view mode as stated in #15.

Yes it's not identical 100% of the time, it's identical about 95-99% of the time but that's what makes it even more confusing.

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 , so some additionla logic i needed to that covers the scenario of view.

@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'.

The history here is that Drupal started of with only hard-coded 'teaser' and 'page' template variables.

At some point, I think in Drupal 7, configurable view modes were introduced. However, the template variables were never updated nor clarified.

We didn't remove 'teaser' until 📌 Deprecate $variables['teaser'] Fixed even though it was 100% a duplicate of the view mode.

'page' is slightly more complicated because the logic for 'page' vs 'full' was never brought fully into sync with each other, so there are subtle differences.

However, I think we should be deprecating $variables['page'] and adjusting any necessary logic to rely on view mode = 'full' - e.g. the separation between 'page' and 'full' is because this has just never been cleaned up properly in 15 years, it is not because there was a well thought out discussion about keeping them similar but subtly different concepts.

If someone had a use case for a further view mode, that has the same configuration as 'full', but avoids any logic around the full mode specifically, that can be achieved by configuring 'full' the same as 'default' and then adding an additional view mode that is not overridden.

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).

@acbramley yes it would fix that, would just be able to rely on the full view mode.

Updated the issue summary here. I think that the MR actually looks mostly fine and could probably be revived - the issue summary was a bit confusing, but the MR matches what I think we should be trying to do here.

Nearly all the checks for $view_mode != full in the MR could eventually be removed by ✨ [META] Expose Title and other base fields in Manage Display Active which is also a remnant of 20 year old code that was never updated to use view/display modes properly yet.

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.

For taxonomy_term_is_page(), I think we should open another issue to deprecate things there - the pattern is the same but it would affect a different set of templates. Also probably makes sense to do that only once this one lands (or is at least RTBC) so that the approach can be copied. Tagging needs followup for that.

Went ahead and opened it 📌 Deprecate $variables['page'] in taxonomy term templates Postponed

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?

Deprecating node_is_page() here and also using a title callback for the comment reply route both sound good to me.

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 🐛 Duplicate title on node revision page Active

Closed 📌 Deprecate node_is_page and move into a service Active 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)

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 an still RTBC as long as it wasn't *only* you.

We can also remove half of views_preprocess_node() here - just noticed via reviewing/committing the other issue.

https://git.drupalcode.org/project/drupal/-/commit/fc8732f24ea193a618b27...

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 ( 📌 Add option to log deprecation errors to prepare for Drupal 11 Needs work would help though)

Well, we will be able to remove that once we actually remove the flag, ye

Oh good point, getting over excited...

Added test coverage