WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 15 months ago

Last modified 15 months 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 15 months 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
15 months 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
15 months ago

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

@thakkarhardik
15 months ago

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

#3 @thomstark
15 months ago

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

Looks great. Thank you!

#4 @garrett-eclipse
15 months 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
15 months ago

Thanks, yes. Closing was not intentional.

#6 @thakkarhardik
15 months 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
15 months 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
15 months 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
15 months 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
15 months ago

Thanks @garrett-eclipse and @SergeyBiryukov .

Note: See TracTickets for help on using tickets.