WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

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

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

@thakkarhardik
3 months ago

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

#3 @thomstark
3 months ago

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

Looks great. Thank you!

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

Thanks, yes. Closing was not intentional.

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

Thanks @garrett-eclipse and @SergeyBiryukov .

Note: See TracTickets for help on using tickets.