Problem/Motivation

There is a @todo comment in the theme_render_and_autoescape docblock that references a closed issue: https://www.drupal.org/node/2575081.

https://git.drupalcode.org/project/drupal/-/blob/9.1.x/core/includes/the...

Proposed resolution

Determine if this function should be marked as deprecated or if the @todo comment should be updated/removed.

Remaining tasks

Verify that this summary is accurate and propose a more detailed resolution.

User interface changes

None.

API changes

TBD.

Data model changes

None.

Release notes snippet

TBD.

CommentFileSizeAuthor
#25 3159871-25.patch1.9 KBsmustgrave
#16 Update_or_remove_todo_comment_in_theme_render_and_autoescape-3159871-16.patch1.22 KBrakhi soni
#6 interdiff_2_5.txt634 bytesanmolgoyal74
#6 3159871-5.patch1.22 KBanmolgoyal74
#2 3159871-2.patch495 bytessarvjeetsingh

Comments

richardbporter created an issue. See original summary.

sarvjeetsingh’s picture

sarvjeetsingh commented
Status: Active » Needs review
StatusFileSize
new 3159871-2.patch495 bytes

As the referenced issue is closed and the function theme_render_and_autoescape is not deprecated. I think we can remove this todo docblock.
uploaded a patch for the same.

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented

I have reviewed the above patch ,it removes closed issue reference @todo comment from theme_render_and_autoescape function.RTBC

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3159871-2.patch, failed testing. View results

anmolgoyal74’s picture

anmolgoyal74 commented
Status: Needs work » Needs review
StatusFileSize
new 3159871-5.patch1.22 KB
new interdiff_2_5.txt634 bytes

Removed the message from one more file as well.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

tanubansal’s picture

tanubansal commented

Tested #6, this can be moved to RTBC

richardbporter’s picture

richardbporter commented

I'm still unclear on this one. Yes the referenced issue was closed but was deprecating `theme_render_and_autoescape` ever discussed?

Also, I'm not sure this was addressed. 

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -467,8 +467,6 @@ public function preExecute(SelectInterface $query = NULL) {
-      //   https://www.drupal.org/node/2575081

Maybe it should point to the deprecation policy? https://www.drupal.org/node/2856615

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vikashsoni’s picture

vikashsoni commented

#6 patch applied successfully
After patch @todo comment has been removed
Thanks for the patch

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vighneshh’s picture

vighneshh commented
Status: Needs review » Reviewed & tested by the community

#6 patch applied moving issue to RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

We should update the @todo in theme_render_and_autoescape() to point to a new issue for deprecating and removing it - it's only used in test coverage and core no longer supports theme functions.

The query hunk looks unrelated to the title here, and should also point to a new issue for triggering a deprecation error when term_access is passed.

rakhi soni’s picture

rakhi soni commented
Status: Needs work » Needs review
StatusFileSize
new Update_or_remove_todo_comment_in_theme_render_and_autoescape-3159871-16.patch1.22 KB

Kindly review patch for version 9.5x,,

vinmayiswamy’s picture

vinmayiswamy commented

#16 patch applied successfully in Drupal 9.5.x-dev
Thanks @Rakhi Soni

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

As per #15 we can't just remove either of these comments.

Re deprecating theme_render_and_autoescape() the question is whether we should keep this in core for other (hypothetical?) theme engines to use, or whether we should leave escaping to be implemented by theme engine itself. If we do decide to deprecate and remove it we can also close #2575065: Refactor theme_escape_and_render() and TwigExtension::escapeFilter() to share reused code.

The term_access comment should be updated to point to #3261261: Properly deprecate term_access query tag

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I searched contrib and can find no valid uses of theme_render_and_autoescape() so I think it should be deprecated and removed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Opened #3299946: Deprecate theme_render_and_autoescape()

So to summarise:

theme_render_and_autoescape() can point to #3299946: Deprecate theme_render_and_autoescape()
term_access can point to #3261261: Properly deprecate term_access query tag

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

Patch #6 still applies to 9.5, so there was no reason for the reroll in #16 ant it is the same as the patch aa #6, so I'm preemptively removing credit.

Also, #16 shouldn't have had its status set to "needs review". It was set to "needs work" prior to that because specific changes were requested. It shouldn't be switched to "needs review" until those changes are implemented. Even if it were a necessary reroll, rerolls by themselves do not require review.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Moving to NW For #20

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review
StatusFileSize
new 3159871-25.patch1.9 KB

@longwave like this? I couldn't find an example of inline deprecation

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

This issue exists just to update the @todos. But instead of just repointing them, I added patches for both @todos instead:

#3299946: Deprecate theme_render_and_autoescape()
#3261261: Properly deprecate term_access query tag

smustgrave’s picture

smustgrave commented

Should this be closed then?

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.