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

Created on 2 July 2024, 8 months ago

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

11.0 🔥

Component
Node system  →

Last updated 1 day ago

No maintainer
Created by

🇬🇧United Kingdom catch

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • Status changed to Needs work 8 months ago
  • 🇬🇧United Kingdom catch
  • First commit to issue fork.
  • Pipeline finished with Failed
    8 months ago
    Total: 181s
    #215949
  • 🇬🇧United Kingdom longwave UK

    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.

  • First commit to issue fork.
  • Merge request !9037Issue #3458589 "Deprecate page variable" → (Open) created by ankitv18
  • 🇮🇳India ankitv18

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

  • Pipeline finished with Failed
    7 months ago
    Total: 205s
    #241426
  • Pipeline finished with Failed
    7 months ago
    Total: 165s
    #241441
  • Pipeline finished with Failed
    7 months ago
    Total: 569s
    #241444
  • Pipeline finished with Failed
    7 months ago
    Total: 503s
    #241523
  • 🇮🇳India Sahana _N

    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 ++

  • Status changed to Needs review 5 months ago
  • 🇳🇿New Zealand quietone

    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?

  • 🇬🇧United Kingdom catch

    Updated the issue summary.

  • Status changed to Needs work 5 months ago
  • 🇺🇸United States smustgrave

    Thanks for updating IS @catch

  • Pipeline finished with Failed
    2 months ago
    Total: 557s
    #375691
  • Status changed to Needs review about 2 months ago
  • 🇳🇿New Zealand quietone

    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?

  • 🇺🇸United States smustgrave

    Seems to cause valid test failures in tests.

  • 🇬🇧United Kingdom catch

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

  • Pipeline finished with Failed
    15 days ago
    Total: 1568s
    #419164
  • 🇳🇿New Zealand quietone

    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
  • 🇬🇧United Kingdom catch

    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.

  • 🇳🇿New Zealand quietone

    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?

  • First commit to issue fork.
  • Pipeline finished with Failed
    14 days ago
    Total: 409s
    #419593
  • 🇮🇳India vakulrai

    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.

  • 🇳🇿New Zealand quietone

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

  • 🇬🇧United Kingdom catch

    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.

Production build 0.71.5 2024