Problem/Motivation

Views module adds $variables['view'] to node templates.

This is impossible to provide cacheable metadata for, because we don't have the render context of the view. I've also never seen it used.

We should deprecate it in 11.1.0 for removal in 12.0.0

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 3458608-afterMR.png403.64 KBvinmayiswamy

Issue fork drupal-3458608

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Active
Issue tags: +Novice

nishtha.pradhan made their first commit to this issue’s fork.

nishtha.pradhan opened merge request !8663

nishtha.pradhan’s picture

nishtha.pradhan commented

Added a deprecation notice for use of $variables['view'].

nishtha.pradhan’s picture

nishtha.pradhan commented
Status: Active » Needs review
ankitv18’s picture

ankitv18 commented
Status: Needs review » Needs work

Hi,
I've reviewed the MR and left a comment.
Please cover the deprecation part and revert the other changes.

Note: Issue URL should be replaced with the document URL of the deprecation.

cc: @catch

nishtha.pradhan’s picture

nishtha.pradhan commented
Status: Needs work » Needs review

Thank you for your feedbacks @guptahemant & @ankitv18.
I have worked upon them and updated the MR.
Please do let me know if any other changes are required.

quietone’s picture

quietone commented

@nishtha.pradhan, thanks for creating a change record for this issue, that is most helpful. Change records are published after an issue is committed and doing that is part of a core committer's tasks. So, please leave change records at draft. Thanks.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment on the MR.

But can we also add a deprecation test for this.

nishtha.pradhan’s picture

nishtha.pradhan commented
Status: Needs work » Needs review
vinmayiswamy’s picture

vinmayiswamy commented
StatusFileSize
new 3458608-afterMR.png403.64 KB

Hi,

I have verified MR !8663 on Drupal 11.x. The suggested changes have been updated. I checked the deprecation message by creating a sample view for testing, and I can confirm that the deprecation message appears on the dump.

Please see the attached screenshot for reference.

Additionally, @smustgrave mentioned in comment #10 the possibility of adding a deprecation test for this. Do we need to include a deprecation test? If so, we may need to move this issue to NW. Otherwise, I'm giving a +1 to RTBC.

Thanks!

ankitv18’s picture

ankitv18 commented
Status: Needs review » Needs work

Tests are failing ~~ moving back to NW

ankitv18’s picture

ankitv18 commented

Rebased the forked branch and pipelines are executed without any failure, hence moving into review

ankitv18’s picture

ankitv18 commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Should have some sort of test coverage

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

There is no special logic related to the deprecation so tests should not be needed. See point 3 of How to deprecate. And we can see a previous example here, #3458183: Deprecate $variables['teaser'].

I updated the change record so the variable name and code are in code blocks. Also removed the phrase 'for now' as there is no intention to replace this.

quietone’s picture

quietone commented

Forgot to add that I searched for usages and found none.

 git grep view | grep node.html.twig

nod_ closed merge request !8663

  • nod_ committed 892c4041 on 11.x 
    Issue #3458608 by nishtha.pradhan, ankitv18, vinmayiswamy, quietone,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed 892c404 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

This missed the same in comment preprocess, I'm going to try and deprecate that as part of #2728419: Deprecate node/comment views-based theme suggestions and variables

I also updated the change record to clarify that this isn't about using node.view instead. Neither is safe and has caching bugs and should not be used at all.