WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 2 weeks ago

#31521 assigned defect (bug)

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 needs-testing needs-unit-tests
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 5 weeks ago.
patch refreshed against trunk
Capture d’écran 2021-05-18 à 23.58.11.png (101.4 KB) - added by audrasjb 5 weeks ago.
Before patch
Capture d’écran 2021-05-18 à 23.59.39.png (66.1 KB) - added by audrasjb 5 weeks ago.
After patch
31521.4.diff (811 bytes) - added by audrasjb 2 weeks ago.
Patch refresh

Download all attachments as: .zip

Change History (22)

#1 @DrewAPicture
6 years ago

  • Focuses template added

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

#2 @DrewAPicture
6 years ago

  • Keywords reporter-feedback added

#3 @michelwppi
6 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
3 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.


4 months ago

#10 @SergeyBiryukov
4 months ago

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

@audrasjb
5 weeks ago

patch refreshed against trunk

#11 @audrasjb
5 weeks 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
5 weeks 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
2 weeks ago

Patch refresh

#13 @audrasjb
2 weeks ago

  • Keywords commit added

Patch refreshed against trunk. Marking for commit.

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


2 weeks ago

#15 @antpb
2 weeks ago

  • Owner changed from SergeyBiryukov to antpb

#16 @hellofromTonya
2 weeks 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.

Note: See TracTickets for help on using tickets.