WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#46828 closed defect (bug) (fixed)

Undefined Offset -1 in wp-includes/rewrite.php on lines 379 and 381

Reported by: thomstark Owned by: SergeyBiryukov
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Permalinks Keywords: has-patch needs-testing
Focuses: Cc:

Description

This is what's currently in the function wp_resolve_numeric_slug_conflicts():

<?php
    $permastructs   = array_values( array_filter( explode( '/', get_option( 'permalink_structure' ) ) ) );
    $postname_index = array_search( '%postname%', $permastructs );
    if ( false === $postname_index ) {
        return $query_vars;
    }
    $compare = '';
    if ( 0 === $postname_index && ( isset( $query_vars['year'] ) || isset( $query_vars['monthnum'] ) ) ) {
        $compare = 'year';
    } elseif ( '%year%' === $permastructs[ $postname_index - 1 ] && ( isset( $query_vars['monthnum'] ) || isset( $query_vars['day'] ) ) ) {
        $compare = 'monthnum';
    } elseif ( '%monthnum%' === $permastructs[ $postname_index - 1 ] && isset( $query_vars['day'] ) ) {
        $compare = 'day';
    }

Here's the problem:

When get_option( 'permalink_structure' ) = '/%postname%/'
then $permastructs = array(0=>'%postname%');
and $postname_index = 0

Therefore, lines 379 and 381 are trying to access

$permastructs[ $postname_index - 1 ]

i.e.

$permastructs[-1]

which outputs a PHP Notice: Undefined offset: -1.

Needs to be adjusted as follows:

<?php
if ( 0 === $postname_index && ( isset( $query_vars['year'] ) || isset( $query_vars['monthnum'] ) ) ) {
        $compare = 'year';
    } elseif ($postname_index && '%year%' === $permastructs[ $postname_index - 1 ] && ( isset( $query_vars['monthnum'] ) || isset( $query_vars['day'] ) ) ) {
        $compare = 'monthnum';
    } elseif ($postname_index && '%monthnum%' === $permastructs[ $postname_index - 1 ] && isset( $query_vars['day'] ) ) {
        $compare = 'day';
    }

OR

<?php
if ( 0 === $postname_index) {
    if( isset( $query_vars['year'] ) || isset( $query_vars['monthnum'] ) ) ) {
        $compare = 'year';
    } 
} 
else {
    if ('%year%' === $permastructs[ $postname_index - 1 ] && ( isset( $query_vars['monthnum'] ) || isset( $query_vars['day'] ) ) ) {
        $compare = 'monthnum';
    } elseif ('%monthnum%' === $permastructs[ $postname_index - 1 ] && isset( $query_vars['day'] ) ) {
        $compare = 'day';
    }
}

Attachments (1)

46828.patch (961 bytes) - added by thakkarhardik 2 years ago.
Here is the patch for that resolves this warning. Please let me know if any changes are needed.

Download all attachments as: .zip

Change History (11)

#1 @thomstark
2 years ago

My second (last) bugfix has an error of its own, an extra closing parenthesis. Here's the fix to that:

<?php

if ( 0 === $postname_index) {
    if( isset( $query_vars['year'] ) || isset( $query_vars['monthnum'] ) ) {
        $compare = 'year';
    } 
} 
else {
    if ('%year%' === $permastructs[ $postname_index - 1 ] && ( isset( $query_vars['monthnum'] ) || isset( $query_vars['day'] ) ) ) {
        $compare = 'monthnum';
    } elseif ('%monthnum%' === $permastructs[ $postname_index - 1 ] && isset( $query_vars['day'] ) ) {
        $compare = 'day';
    }
}

#2 @SergeyBiryukov
2 years ago

  • Component changed from General to Permalinks
  • Keywords needs-patch good-first-bug added

@thakkarhardik
2 years ago

Here is the patch for that resolves this warning. Please let me know if any changes are needed.

#3 @thomstark
2 years ago

  • Resolution set to worksforme
  • Status changed from new to closed

Looks great. Thank you!

#4 @garrett-eclipse
2 years ago

  • Keywords has-patch needs-testing added; needs-patch good-first-bug removed
  • Resolution worksforme deleted
  • Status changed from closed to reopened

Thanks for the patch @thakkarhardik and for confirming it's working @thomstark.

I feel you may have closed this in error @thomstark as the patch wasn't committed. The 'worksforme' closure status is to indicate the issues is a non-issue as it's working on a clean install. When you're moving a ticket forward that has a working patch it's better to leave it open and apply the 'has-patch' 'needs-testing' keywords.

I've updated the ticket accordingly.

#5 @thomstark
2 years ago

Thanks, yes. Closing was not intentional.

#6 @thakkarhardik
2 years ago

Thanks @garrett-eclipse. I am kind of a newbie here and unaware of how the testing is done in these kind of enhancements. Do we need to write unit tests for the above patch? Can you please guide briefly?

#7 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 5.2

Reproduced the notice by visiting /?day=01 URL with the /%postname%/ structure. The patch looks good.

#8 @SergeyBiryukov
2 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from reopened to closed

In 45214:

Permalinks: Avoid a PHP notice in wp_resolve_numeric_slug_conflicts() when visiting a day archive with the /%postname%/ permalink structure.

Props thakkarhardik, thomstark.
Fixes #46828.

#9 @garrett-eclipse
2 years ago

Hi @thakkarhardik sorry I just got your note, but doesn't look like I'm needed here. @SergeyBiryukov already tested and committed this fix so no unit tests or other actions needed.

#10 @thakkarhardik
2 years ago

Thanks @garrett-eclipse and @SergeyBiryukov .

Note: See TracTickets for help on using tickets.