Make WordPress Core

Opened 2 months ago

Closed 4 weeks ago

#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.


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

  • Milestone changed from Awaiting Review to 6.8

#4 @audrasjb
2 months ago

  • Component changed from I18N to Date/Time

#5 @apermo
2 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 2 months ago by apermo (next)

#6 @audrasjb
2 months ago

Ah, thanks for the context 👍

#7 @audrasjb
8 weeks 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
4 weeks 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
4 weeks 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
4 weeks ago

Done ty @sainathpoojary for the reminder :)

#11 @audrasjb
4 weeks 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.