WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#53729 closed enhancement (fixed)

Add readability comment to $title variable used in admin-header

Reported by: ravipatel Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Help/About Keywords: has-patch commit
Focuses: coding-standards Cc:

Description (last modified by hellofromTonya)

Add a readability comment above each instance of $title = __( '' ) in each admin panel's file. This $title is used to populate the <title> element which is built in the reusable wp-admin/admin-header.php file.

The comment will avoid confusion on panel file's where the translated content reads the same for the <title> and <h1>. For example, the confusion can occur in the wp-admin/privacy.php:

$title = __( 'Privacy' );
...
require_once ABSPATH . 'wp-admin/admin-header.php';
...
<div class="about__header-title">
        <h1>
                <?php _e( 'Privacy' ); ?>
        </h1>
</div>

Attachments (3)

53729.patch (593 bytes) - added by ravipatel 6 months ago.
Code Re-usablity
53729.2.patch (4.7 KB) - added by ravipatel 6 months ago.
Added new patch with comment as per suggested.
53729.3.diff (26.3 KB) - added by SergeyBiryukov 6 months ago.

Download all attachments as: .zip

Change History (13)

@ravipatel
6 months ago

Code Re-usablity

#1 @audrasjb
6 months ago

  • Component changed from Administration to Help/About
  • Keywords needs-refresh added

Hello, thanks for the ticket and the patch.

_e() function is meant to display thing, so you shouldn't write $title = _e( 'Privacy' );.

Plus, if the $title variable is never used, shouldn't we just get rid of it?

#2 @SergeyBiryukov
6 months ago

  • Keywords close 2nd-opinion added

Hi there, thanks for the patch!

It might not be immediately clear, but the $title variable is used in wp-admin/admin-header.php.

This is a common pattern used in many admin files, not just in wp-admin/privacy.php:

$title = __( ... );
...
require_once ABSPATH . 'wp-admin/admin-header.php';

Looking at the About page and related pages:

  • wp-admin/about.php:
    $title = _x( 'About', 'page title' );
    ...
    require_once ABSPATH . 'wp-admin/admin-header.php';
    ...
    <div class="about__header-title">
    	<h1>
    		<?php _e( 'WordPress' ); ?>
    		<?php echo $display_version; ?>
    	</h1>
    </div>
    
  • wp-admin/credits.php:
    $title = __( 'Credits' );
    ...
    require_once ABSPATH . 'wp-admin/admin-header.php';
    ...
    <div class="about__header-title">
    	<h1>
    		<?php _e( 'Contributors' ); ?>
    	</h1>
    </div>
    
  • wp-admin/freedoms.php:
    $title = __( 'Freedoms' );
    ...
    require_once ABSPATH . 'wp-admin/admin-header.php';
    ...
    <div class="about__header-title">
    	<h1>
    		<?php _e( 'The Four Freedoms' ); ?>
    	</h1>
    </div>
    
  • wp-admin/privacy.php:
    $title = __( 'Privacy' );
    ...
    require_once ABSPATH . 'wp-admin/admin-header.php';
    ...
    <div class="about__header-title">
    	<h1>
    		<?php _e( 'Privacy' ); ?>
    	</h1>
    </div>
    

So the Privacy page is the only one where $title is the same as the <h1> tag. We could technically reuse the $title variable for the <h1> tag here, but that would make it a bit inconsistent with the other pages.

The existing code seems good to me as is.

Last edited 6 months ago by SergeyBiryukov (previous) (diff)

#3 follow-up: @sabernhardt
6 months ago

  • Type changed from defect (bug) to enhancement

The About page includes a translator comment that helps give context when reading the file, too.

/* translators: Page title of the About WordPress page in the admin. */
$title = _x( 'About', 'page title' );

To explain the variable in each of the three related files, perhaps we could add a regular comment as simple as:

// Used in HTML title tag.
$title = __( 'Privacy' );

#4 in reply to: ↑ 3 @hellofromTonya
6 months ago

  • Keywords needs-patch added; has-patch needs-refresh close 2nd-opinion removed

The $title variable is for populating the <title> tag and not for the panel's <h1>. The <title> is built in wp-admin/admin-header.php. Why is this variable in each panel's file and not assigned in the admin header file? For reusability. It allows each panel to assign its title for use in building the <title>.

Should this variable be used to also populate the <h1>?

No, I don't think so. I agree with @SergeyBiryukov. Why?

As Sergey notes, the <h1> is different than the <title> on many of the panels. Using it for dual purpose on some panels but not others will be confusing to contributors as it breaks the pattern.

At the same time, I can also see confusion when reading it on the Privacy panel and it's the same. It looks like duplicate code. Right? I definitely get why this ticket was created.

What could we do to help with the readability and avoid this type of confusion?

Add an inline comment as suggestions:

To explain the variable in each of the three related files, perhaps we could add a regular comment as simple as:

// Used in HTML title tag.
$title = __( 'Privacy' );

Patch Notes

Also note @ravipatel in 53729.patch, the title variable would need to be echoed. Thought I'd note that as a learning opportunity for anyone reading this ticket.

Next Steps

Add comments above each $title = ... for readability. @ravipatel would you like to create the patch for this?

#5 follow-up: @ravipatel
6 months ago

@hellofromTonya yes i will do patch for this.

#6 in reply to: ↑ 5 @hellofromTonya
6 months ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 5.9
  • Owner set to hellofromTonya
  • Status changed from new to accepted
  • Summary changed from $title never used, Replaced with text to Add readability comment to $title variable used in admin-header

w00t!

Replying to ravipatel:

@hellofromTonya yes i will do patch for this.

Updated the ticket's title and description to match the new scope of this ticket (helps in discoverability).

Also moving into the 5.9 milestone.

@ravipatel
6 months ago

Added new patch with comment as per suggested.

#7 @hellofromTonya
6 months ago

  • Keywords has-patch commit added; needs-patch removed

Great job @ravipatel! 53729.2.patch looks good to me. Marking for commit.

#8 @SergeyBiryukov
6 months ago

It looks like 53729.2.patch is a good start but only patches a portion of the files.

53729.3.diff makes the change consistently for all the files in wp-admin.

Last edited 6 months ago by SergeyBiryukov (previous) (diff)

#9 @SergeyBiryukov
6 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 51475:

Docs: Add a comment about the $title global usage in various admin files.

This should make it clear that the variable is used as part of the HTML <title> tag on admin screens.

Props ravipatel, hellofromTonya, sabernhardt, audrasjb, SergeyBiryukov.
Fixes #53729.

#10 @desrosj
6 months ago

In 51499:

Coding Standards: Apply some alignment fixes from composer format.

Follow up to [51475].

See #53729.

Note: See TracTickets for help on using tickets.