Make WordPress Core

#62824 closed defect (bug) (fixed)

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 (11)

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


16 months 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
16 months 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
16 months ago

  • Milestone changed from Awaiting Review to 6.8

#4 @audrasjb
16 months ago

  • Component changed from I18N to Date/Time

#5 @apermo
16 months 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 found was to add the sanitization as we did in the PR.

Version 0, edited 16 months ago by apermo (next)

#6 @audrasjb
16 months ago

Ah, thanks for the context 👍

#7 @audrasjb
16 months ago

  • Keywords changes-requested added

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

#8 follow-up: @sainathpoojary
15 months ago

Hi @apermo, just checking in on this. Do you plan to update the requested changes? If not, I’d be happy to submit a PR with the requested changes if that works for you.

#9 in reply to: ↑ 8 @apermo
15 months ago

Replying to sainathpoojary:

Hi @apermo, just checking in on this. Do you plan to update the requested changes? If not, I’d be happy to submit a PR with the requested changes if that works for you.

Thanks for the heads up, I'll take care of that later this week, feel free to ping me again if I haven't done it by the end of the week.

#10 @apermo
15 months ago

Done ty @sainathpoojary for the reminder :)

#11 @audrasjb
15 months ago

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

In 59870:

Date/Time: Add sanitization to WP_Locale::get_month().

By adding a sanitization to $wp_locale->get_month(), this changeset prevents a PHP Warning: Undefined array key "00" caused by single_month_title(). This function previously assumed that get_query_var( 'm' ) is always at least 6 digits, and always contains the year and the month, which is not necessarily true.

Props apermo, audrasjb, xateman.
Fixes #62824.

Note: See TracTickets for help on using tickets.