Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#54127 closed defect (bug) (fixed)

Twenty Twenty-One: Missing esc_html__() in functions.php

Reported by: teucrium's profile teucrium Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: minor Version: 5.8.1
Component: Bundled Theme Keywords: has-patch
Focuses: coding-standards Cc:

Description (last modified by SergeyBiryukov)

The escaping function esc_html__() is missing in Twenty Twenty One functions.php, line 77:

register_nav_menus(
        array(
        'primary' => esc_html__( 'Primary menu', 'twentytwentyone' ),
        'footer'  => __( 'Secondary menu', 'twentytwentyone' ),
        )
);

We should find:

'footer'  => esc_html__( 'Secondary menu', 'twentytwentyone' ),

instead of:

'footer'  => __( 'Secondary menu', 'twentytwentyone' ),

Attachments (1)

54127.PNG (63.5 KB) - added by muhammadfaizanhaidar 3 years ago.
Added the escaping function esc_html() in Twenty Twenty One functions.php

Download all attachments as: .zip

Change History (12)

#2 @SergeyBiryukov
3 years ago

  • Component changed from Themes to Bundled Theme
  • Description modified (diff)
  • Summary changed from Missing esc_html__() in Twenty Twenty One functions.php to Twenty Twenty-One: Missing esc_html__() in functions.php

@muhammadfaizanhaidar
3 years ago

Added the escaping function esc_html() in Twenty Twenty One functions.php

#3 follow-ups: @muhammadfaizanhaidar
3 years ago

@SergeyBiryukov @teucrium I have uploaded a patch for this ticket but I think it's better if we add escape functions to all bundled themes? Because I can see them missing in other themes too.

#4 in reply to: ↑ 3 @teucrium
3 years ago

Replying to muhammadfaizanhaidar:

@SergeyBiryukov @teucrium I have uploaded a patch for this ticket but I think it's better if we add escape functions to all bundled themes? Because I can see them missing in other themes too.

You're right, the other default themes needs this patch as well ! Thanks a lot.

#5 in reply to: ↑ 3 ; follow-ups: @SergeyBiryukov
3 years ago

Replying to muhammadfaizanhaidar:

I have uploaded a patch for this ticket but I think it's better if we add escape functions to all bundled themes? Because I can see them missing in other themes too.

Thanks for the patch!

Previously, the point of view here was that core translations (including bundled themes) are considered safe because we have a review process for them, see #42639 and the discussion in #30724. (Also related: #32233.)

In WordPress core and older bundled themes, strings are generally only escaped in attributes or in <option> tags.

Some other related tickets: #47384, #47385, #49535, #49536, #49537.

This was recently reconsidered for the Twenty Twenty-One theme, see the discussion in #core-themes on Slack.

As the purpose of bundled themes is to demonstrate best practices, they should use proper escaping so that the code copied from or based on these themes also uses correct escaping. This has been addressed for Twenty Twenty-One and will be addressed for newer bundled themes going forward.

For updating the escaping in older themes though, there is no consensus yet, see the second part of the discussion. This should probably be discussed with the Themes team. Personally, I think either way is fine. As these themes are periodically updated for better block editor support, I guess we could address the escaping as well.

#6 @sabernhardt
3 years ago

Twenty Twenty-One history: This commit updated the translation functions. The footer (secondary) menu was added later, so it missed the escaping function.

#7 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.9

#8 in reply to: ↑ 5 @muhammadfaizanhaidar
3 years ago

I am up and ready to contribute whenever Themes team think its time to implement thanks.

Version 0, edited 3 years ago by muhammadfaizanhaidar (next)

#9 in reply to: ↑ 5 @mukesh27
3 years ago

Replying to SergeyBiryukov:

Personally, I think either way is fine. As these themes are periodically updated for better block editor support, I guess we could address the escaping as well.

Yes we should escape other old themes.

PR looks fine and ready to marge.

#10 @SergeyBiryukov
3 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 51820:

Twenty Twenty-One: Add missing escaping for the "Secondary menu" label.

Props muhammadfaizanhaidar, teucrium, sabernhardt, mukesh27, SergeyBiryukov.
Fixes #54127.

Note: See TracTickets for help on using tickets.