WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#12700 closed defect (bug) (invalid)

Malformed permalinks for feeds and paged / generate_rewrite_rules

Reported by: t31os_ Owned by: ryan
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Rewrite Rules Keywords: has-patch commit
Focuses: Cc:

Description

All rewrite rules(custom or standard options, non-default obviously) seem to suffer for a minor issue on rewrite generation, on iterations 5 and 6, $query is empty for those iterations (Line 1337 - the for loop).. resulting in malformed rules, ie. index.php?&feed ..

$feedquery
$feedquery2
$pagequery

Are effected as a result, 6 rules then contain the malformed query string..

Line 1349 sets this to an empty string.

$query = ( isset($queries) && is_array($queries) ) ? $queries[$num_toks - 1] : '';

Lines 1368, 1382, and 1386 then set rules incorrectly.

A simple fix, just conditionalise the three lines with a string comparison for those lines...

if( '' == $query )
  // etc..

Or just use a ternary comparison to do the same..

Could not find an existing report for this, so please close if is indeed duplicate.

First trac submission, go easy on me.. ;)

Attachments (1)

12700.diff (2.2 KB) - added by scribu 4 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 scribu4 years ago

  • Keywords needs-testing removed

It would be a lot easier to follow your reasoning if you provided a diff file.

See http://core.trac.wordpress.org/wiki#HowtoSubmitPatches

comment:2 scribu4 years ago

  • Keywords reporter-feedback added

Also, could you provide some steps to reproduce this issue (and what to look at)?

comment:3 nacin4 years ago

  • Milestone changed from Unassigned to 3.0

comment:4 t31os_4 years ago

I have to be honest guys, i don't properly understand the procedure for creating a diff file, and whether i need to commit or just attach something here, i've reviewed Peter Westwoods guide on using Tortoise (on Windows here), but seeing as i've done very little with SVN, it's a little beyond my understanding (wasn't going to let that stop me reporting a "possible" bug though).


wp-includes/rewrite.php is the file in question (function - generate_rewrite_rules), replication should only require an apache driven server and choosing any permalink structure(non-default).


Print / Dump the rewrite array after the permalinks are generated and you should see the problem query strings produced, or at least i'm hoping you do, because i'm not doing any customization to the rewrite array on the local install i'm testing under.


Please let me know if there's anything else i can do to help, bearing in mind my noviceness using trac and SVN(happy to learn, just be prepared for the possibily i may make errors).

scribu4 years ago

comment:5 scribu4 years ago

  • Keywords has-patch added; needs-patch reporter-feedback removed

12700.diff uses add_query_arg() to avoid malformed URLs.

comment:6 scribu4 years ago

t31os_, try applying the patch and see if everything is working properly.

comment:7 t31os_4 years ago

Bear with me, still figuring out the whole, apply/create patch routine..

Code looks sound to me though, i'd have never of thought to use add_query_arg, looks like a nice clean approach. Were you able to reproduce the problem?

comment:8 scribu4 years ago

Yeah, there were ?& all over the place.

comment:9 t31os_4 years ago

Spent a couple of hours trying to work the patch, tortoise seems to be giving me errors any which way i apply the patch, so i've had to give up for now (was becoming frustrated).

Managed to apply a self-created patch just fine, just couldn't get the patch downloaded here to work (wiped all files re-updated files via SVN, still no go).

Will reinstall tortoise soon and i'll try again, you'll have to excuse the noviceness for now, need to come back to the SVN lessons when i've got a fresh head on, else i'll drive myself insane getting nowhere.

Looks like you've verified the bug though, so i'll consider the report worthwhile all the same.. :)

comment:10 scribu4 years ago

  • Keywords commit added

Welcome to the bug squad. :)

comment:11 dd324 years ago

What is the actual bug here? Is it just the "invalid" url?

I only ask this because the URL patterns generated are only used internally, and from what i can see, does not cause error or bugs due to the way the PHP URL parsers are written.

I see no real objection to the attached patch in theory, however, It may be best to leave it as-is (at least for 3.0) for consistency sake, as thats whats used throughout the rest of the file (uses of $var .= '&' . $var . '='..etc..

So, Unless this is causing an actual error, this should probably be classed an enhancement, and left for 3.1's consideration.

comment:12 t31os_4 years ago

Enable permalinks(any structure), dump/print the rewrite array, and look at the rules..

print '<pre>';
print_r( get_option( 'rewrite_rules' ) );
print '<pre>';

.. or whichever method you prefer ..

You'll see six rules that look like so.


Example:
index.php?&feed
index.php?&paged

Are you saying these will still be mapped correctly by Apache even when they are malformed?

comment:13 dd324 years ago

Are you saying these will still be mapped correctly by Apache even when they are malformed?

They are never passed to Apache at all. They are never exposed outside of WordPress, They are never used directly by WordPress other than in parse_url() + parse_str() for internal querying purposes.

The result of index.php?&paged would be parsed as:

array
  'path' => string 'index.php' (length=9)
  'query' => 
    array
      'paged' => string '' (length=0)

comment:14 t31os_4 years ago

Perhaps it was just an oversight in the original code then.


Are you saying we should leave it be and let parse_url/str deal with them later on in the processing.


This might cause confusion for anyone examining the rewrite array, or mislead people to think there's a bug, as it has done with me.


What is perhaps most important here is that it doesn't break anything, and for me it looks like the permalinks act appropriately in each case. Of course this does leave what appears to be malformed rules in the array, not that it matters a great deal, i'd personally just prefer the rules to look as they should.

comment:15 dd324 years ago

  • Component changed from Permalinks to Rewrite Rules
  • Milestone changed from 3.0 to 3.1

I'm not 100% against changing it, I'd just prefer the entire file to follow the same URI building scheme.

I'm setting this to 3.1 however, I'd prefer not to change it in the leadup to a release since its not causing any issues.

comment:16 hakre4 years ago

Any traction on this one? I don not fully understand the patch and what this is about, so maybe some love by someone else?

comment:17 scribu4 years ago

  • Milestone Awaiting Triage deleted
  • Resolution set to invalid
  • Status changed from new to closed

If it ain't broke, don't fix it.

We have more important things to work on, like maybe a better rewrite engine altogether: #12935

Note: See TracTickets for help on using tickets.