WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 2 months ago

Last modified 2 months ago

#31521 closed defect (bug) (fixed)

wp_title if archive of year w/o permalink fires php notice in locale.php

Reported by: michelwppi Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version: 4.1.1
Component: General Keywords: good-first-bug has-patch has-screenshots has-unit-tests commit
Focuses: template Cc:

Description

PHP Notice : Undefined index: 00 in /Applications/MAMP/htdocs/wp_svn41/wp-includes/locale.php on line 271

The concerned function : $wp_locale->get_month

Permalinks = default like: www.mysite.com/?m=2015

The calling function = wp_title in general-template.php

Here:

// If there's a month
        if ( is_archive() && !empty($m) ) {
                $my_year = substr($m, 0, 4);
                $my_month = $wp_locale->get_month(substr($m, 4, 2)); error_log("here calling...");
                $my_day = intval(substr($m, 6, 2));
                $title = $my_year . ( $my_month ? $t_sep . $my_month : '') . ( $my_day ? $t_sep . $my_day : '' );
        }

This part of function forget that 'm' query_tag can have a length from 4 to 9+ as well defined in query.php - not only month...

if ( $qv['m'] ) {
                                $this->is_date = true;
                                if ( strlen($qv['m']) > 9 ) {
                                        $this->is_time = true;
                                } else if ( strlen($qv['m']) > 7 ) {
                                        $this->is_day = true;
                                } else if ( strlen($qv['m']) > 5 ) {
                                        $this->is_month = true;
                                } else {
                                        $this->is_year = true;
                                }
                        }

and $wp_locale->get_month do not like empty string giving '00' index...

Suggestion (raw):

Add test ( strlen($m) >= 5 ) before to call $wp_locale->get_month

like

$my_month = ( strlen($m) >= 5 ) ? $wp_locale->get_month(substr($m, 4, 2))
: "";

Cheers,

M.

Attachments (6)

31521.patch (1.1 KB) - added by herregroen 4 years ago.
Cleans up date parsing in wp_title.
31521-2.diff (751 bytes) - added by davidmosterd 4 years ago.
Patch with my comment
31521.3.diff (811 bytes) - added by audrasjb 8 months ago.
patch refreshed against trunk
Capture d’écran 2021-05-18 à 23.58.11.png (101.4 KB) - added by audrasjb 8 months ago.
Before patch
Capture d’écran 2021-05-18 à 23.59.39.png (66.1 KB) - added by audrasjb 8 months ago.
After patch
31521.4.diff (811 bytes) - added by audrasjb 8 months ago.
Patch refresh

Download all attachments as: .zip

Change History (28)

#1 @DrewAPicture
7 years ago

  • Focuses template added

Hi @michelwppi, are you still experiencing this issue in 4.2?

#2 @DrewAPicture
7 years ago

  • Keywords reporter-feedback added

#3 @michelwppi
7 years ago

@DrewAPicture
This issue remains in WP 4.2 because the test of

strlen($m) >=5

is not done in function wp_title line 829 like done in wp_query of wp_query.php.

Cheers

#4 @SergeyBiryukov
4 years ago

  • Keywords needs-patch good-first-bug added; reporter-feedback removed
  • Milestone changed from Awaiting Review to Future Release

#5 @herregroen
4 years ago

Currently looking at this.

To help others, the steps to reproduce:

  1. set permalinks to plain.
  2. Add wp_title somewhere on your archive page.
  3. Go to yourdomain.com?m=2017.
  4. See the error before wp_title is output.

@herregroen
4 years ago

Cleans up date parsing in wp_title.

#6 @davidmosterd
4 years ago

I was discussing with @herregroen that we could also just move the $wp_locale->get_month( $my_month ) check to the conditional echo. That would minimize the adjustment to the code and solve the issue as well. Like so:

$title = $my_year . ( $my_month ? $t_sep . $wp_locale->get_month( $my_month ) : '' ) . ( $my_day ? $t_sep . $my_day : '' );

#7 @herregroen
4 years ago

  • Keywords has-patch added; needs-patch removed

Added a patch that cleans up the date parsing by reusing the same WP_Query globals is_time, is_day and is_month to do the checks.

Used an array to implode to avoid a huge assignment to $title.

More changes but I think overall more readable code.

@davidmosterd
4 years ago

Patch with my comment

#8 @DrewAPicture
4 years ago

  • Owner set to herregroen
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

This ticket was mentioned in Slack in #core by sergey. View the logs.


11 months ago

#10 @SergeyBiryukov
11 months ago

  • Milestone changed from Future Release to 5.8
  • Owner changed from herregroen to SergeyBiryukov
  • Status changed from assigned to reviewing

@audrasjb
8 months ago

patch refreshed against trunk

#11 @audrasjb
8 months ago

  • Keywords has-screenshots added
  • Owner changed from SergeyBiryukov to audrasjb
  • Status changed from reviewing to accepted

I was able to reproduce the issue and I can confirm the proposed patch solves it. In 31521.3.diff, I just refreshed the patch against trunk.

#12 @audrasjb
8 months ago

  • Owner changed from audrasjb to SergeyBiryukov
  • Status changed from accepted to assigned

Reassigning to @SergeyBiryukov for review, sorry I didn't see you were reviewing.

@audrasjb
8 months ago

Patch refresh

#13 @audrasjb
8 months ago

  • Keywords commit added

Patch refreshed against trunk. Marking for commit.

This ticket was mentioned in Slack in #core by antpb. View the logs.


7 months ago

#15 @antpb
7 months ago

  • Owner changed from SergeyBiryukov to antpb

#16 @hellofromTonya
7 months ago

  • Keywords needs-testing needs-unit-tests added; commit removed
  • Milestone changed from 5.8 to 5.9
  • Owner changed from antpb to hellofromTonya

In reviewing the patch, noticed it is using substr() and does not have guarding to check for failed state. In PHP 8, substr() return type changed for failure from boolean false to an '' empty string. This change could have impact on the patch or upstream from it.

Today is 5.8 Beta 1. After discussing with @antpb, decided to punt to 5.9 to give time for deeper review and adding tests.

Reassigning ownership to me to follow-up on the review and tests.

This ticket was mentioned in PR #1861 on WordPress/wordpress-develop by hellofromtonya.


2 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Adds tests to first validate the bug exists (test fail when the fix is not applied).

Fix:

  • Move substr() to assign to the variable $my_month as it can return an empty string.
  • If a month was found (not an empty string), then pass it to $wp_locale->get_month().

Makes the long concatenated title string multiline for readability.

Trac ticket: https://core.trac.wordpress.org/ticket/31521

#18 @hellofromTonya
2 months ago

  • Keywords commit added

Hello folks,

I too was able to reproduce the issue and 31521.4.diff resolves it.

I created PR 1861 which:

  • adds unit tests (which do fail when the fix is not applied)
  • applies the 31521.4.diff fix
  • for the $title concatenated string: puts each date part on a separate line for improved readability

Marking for commit. Props to @costdev for the code review.

#19 @hellofromTonya
2 months ago

  • Keywords needs-testing removed

Removing needs-testing as @audrasjb tested the original patch (which is used in the PR).

#20 @hellofromTonya
2 months ago

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

In 52136:

Template: Fix "undefined index: 00" when archive month query is empty in wp_title().

When m query_tag has a valid year, i.e. ?m=2021, and there are posts for that year, substr() returns a false on PHP 5.6 and an empty string on PHP 7.0+. Passing either of those values to $wp_locale->get_month() results in a PHP notice on PHP 5.6 to PHP 7.4 and a PHP Warning on PHP 8.0+.

Why? The $month lookup table has zeroized keys from '01' to '12'. A empty value is passed to zeroise() returns '00' which is directly passed as a key in the month property. That key does not exist.

While $wp_locale->get_month() would benefit from guarding/validation, this fix ensures a falsey value is not passed as a month.

Tests are added including a test that fails with this fix not applied.

Follow-up to [801], [35294], [35624].

Props antpb, audrasjb, costdev, davidmosterd, drewapicture, herregroen, hellofromTonya, michelwppi, sergeybiryukov.
Fixes #31521.

#22 @hellofromTonya
2 months ago

Thank you everyone for contributing! The fix will ship with 5.9.

Also note, on PHP 8.0+, this fixes a PHP Warning.

Note: See TracTickets for help on using tickets.