WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#18991 closed defect (bug) (fixed)

Unnecessarily cryptic code when checking pagename= for verbose rules

Reported by: duck_ Owned by:
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.3
Component: Rewrite Rules Keywords: has-patch
Focuses: Cc:

Description

[18541] for #16687 contains:

if ( $wp_rewrite->use_verbose_page_rules && preg_match( '/pagename=\$([^&\[]+)\[([0-9]+)\]/', $query, $varmatch ) ) { 
    // this is a verbose page match, lets check to be sure about it 
    if ( ! get_page_by_path( ${$varmatch[1]}[$varmatch[2]] ) )

The variable variable is unnecessary as we know we want to be looking in the $matches array.

The regular expression could also be made clearer with /pagename=\$matches\[([0-9]+)\]/. We already assume it is (see above) and if it's not $matches then WP_MatchesMapMatches isn't going to work anyway.

Attachments (2)

18991.diff (1.5 KB) - added by duck_ 3 years ago.
18991.matches.diff (1.5 KB) - added by duck_ 3 years ago.

Download all attachments as: .zip

Change History (9)

@duck_3 years ago

@duck_3 years ago

comment:1 follow-up: @Otto423 years ago

While you're correct in the context of the core pagename rules, I wrote it this way in order to account for pagename rules that might be added by other systems/plugins. Basically, we don't know that pagename=matches here, not for sure.

Now, the regexp may be off for other cases, but I'd prefer to see some of those other cases before saying that pagename is always going to be =$matches[x].

Maybe I'm being overly cautious?

comment:2 @Otto423 years ago

That said, this would be a very good optimization, if you want to assume pagename will always = $matches[x]. I was originally thinking of pagename=something-static here, but the regexp won't account for that as written, so it's broken anyway for that case.

comment:3 in reply to: ↑ 1 @duck_3 years ago

Reply got delayed until after your 2nd one, but will post anyway.

Replying to Otto42:

Maybe I'm being overly cautious?

The variable variable doesn't make sense if it's not "matches" as there's no other array to use.

As for the regular expression. If the query is not pagename=$matches[n] then it's not going to be used by WP_MatchesMapRegex anyway since WP_MatchesMapRegex::$_pattern is '(\$matches\[[1-9]+[0-9]*\])'.

I wasn't sure about this at first too. I even followed the code back to [881] to see if a different variable name was ever useful/worked here.

comment:4 @Otto423 years ago

  • Cc nacin added

My first couple of stabs at this were attempting to grab whatever-the-heck was after pagename and use it appropriately, under the consideration that I don't know everything and somebody might have something else there in some weird rewrite rule. This turned out to not work because of the [array] parameter bits. So it sort of evolved into this over time.

If we're sure that $matches[x] is the only thing that will ever work here anyway, then this patch is perfectly fine and should get pushed into 3.3.

comment:5 @nacin3 years ago

  • Component changed from General to Rewrite Rules

Per WP_MatchesMapRegex's $_pattern: '(\$matches\[[1-9]+[0-9]*\])'. Seems like the [1-9]+[0-9]* should also venture into these regexes.

Looks good otherwise. If it's not $matches, it won't work.

comment:6 @nacin3 years ago

In [19017]:

We know that pagename is being assigned to $matches-something, so skip the variable variable. props duck_. see #18991.

comment:7 @nacin3 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.