Make WordPress Core

Opened 4 weeks ago

Last modified 3 weeks ago

#62824 reviewing defect (bug)

single_month_title() can cause a warning.

Reported by: apermo's profile apermo Owned by: audrasjb's profile audrasjb
Milestone: 6.8 Priority: normal
Severity: normal Version: 2.1
Component: Date/Time Keywords: has-patch changes-requested
Focuses: Cc:

Description

In my sentry setup I got this error message over the weekend.

Warning: Undefined array key "00" caused by single_month_title()

This is the relevante piece of code.

<?php
$m        = get_query_var( 'm' );

// ...

        } elseif ( ! empty( $m ) ) {
                $my_year  = substr( $m, 0, 4 );
                $my_month = $wp_locale->get_month( substr( $m, 4, 2 ) );
        }

This assumes that get_query_var( 'm' ) is always at least 6 digits, and always contains the year and the month.
But comparing to https://core.trac.wordpress.org/browser/tags/6.7/src/wp-includes/class-wp-query.php#L2048, this shows that this is not necessarily true.

I think that the most robust solution would be to add a sanitization to $wp_locale->get_month().

I will open a PR for that.

Change History (7)

This ticket was mentioned in PR #8142 on WordPress/wordpress-develop by @apermo.


4 weeks ago
#1

  • Keywords has-patch added

refactor(class-wp-locale.php): Added sanitization to WP_Locale::get_month()

This prevents a Warning: Undefined array key "00"

Co-authored-by: xateman

Refs: #62824

Trac ticket: https://core.trac.wordpress.org/ticket/62824#ticket

#2 @audrasjb
4 weeks ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Thanks for the ticket and the PR.
Maybe it would be more straightforward and performant to directly set a default value just in case? Like public function get_month( $month_number = '01' ). What do you think?

#3 @audrasjb
4 weeks ago

  • Milestone changed from Awaiting Review to 6.8

#4 @audrasjb
4 weeks ago

  • Component changed from I18N to Date/Time

#5 @apermo
4 weeks ago

Hey @audrasjb.

I should have copied a little bit more.

Here is the bigger context.

<?php
/**
 * Displays or retrieves page title for post archive based on date.
 *
 * Useful for when the template only needs to display the month and year,
 * if either are available. The prefix does not automatically place a space
 * between the prefix, so if there should be a space, the parameter value
 * will need to have it at the end.
 *
 * @since 0.71
 *
 * @global WP_Locale $wp_locale WordPress date and time locale object.
 *
 * @param string $prefix  Optional. What to display before the title.
 * @param bool   $display Optional. Whether to display or retrieve title. Default true.
 * @return string|false|void False if there's no valid title for the month. Title when retrieving.
 */
function single_month_title( $prefix = '', $display = true ) {
        global $wp_locale;

        $m        = get_query_var( 'm' );
        $year     = get_query_var( 'year' );
        $monthnum = get_query_var( 'monthnum' );

        if ( ! empty( $monthnum ) && ! empty( $year ) ) {
                $my_year  = $year;
                $my_month = $wp_locale->get_month( $monthnum );
        } elseif ( ! empty( $m ) ) {
                $my_year  = substr( $m, 0, 4 );
                $my_month = $wp_locale->get_month( substr( $m, 4, 2 ) );
        }

        if ( empty( $my_month ) ) {
                return false;
        }

        $result = $prefix . $my_month . $prefix . $my_year;

        if ( ! $display ) {
                return $result;
        }
        echo $result;
}

In that case that was caught by sentry, $m was just a 4 digit representation of a year.

So I ended up in the elseif() but sent an empty string to $wp_locale->get_month( substr( $m, 4, 2 ) ) resulting the warning and $my_month to be empty, and thus the whole function to return false.

Returning January by default would be wrong in my eyes, and as I wanted to point out, not necessary, as single_month_title() already accounts for it. I understand your point, and I also think this function as a whole could use an overhaul, but the one backwards compatible way me and my colleague (Props to @xate for digging into it with me) found was to add the sanitization as we did in the PR.

Last edited 4 weeks ago by apermo (previous) (diff)

#6 @audrasjb
4 weeks ago

Ah, thanks for the context 👍

#7 @audrasjb
3 weeks ago

  • Keywords changes-requested added

Note: I requested a change in the PR. Once done, we should be good to ship it :)

Note: See TracTickets for help on using tickets.