Make WordPress Core

Opened 16 years ago

Closed 9 years ago

Last modified 8 years ago

#6481 closed enhancement (fixed)

Fancy permalinks should be enabled on new sites

Reported by: denis-de-bernardy's profile Denis-de-Bernardy Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.2 Priority: normal
Severity: normal Version: 2.7
Component: Permalinks Keywords: has-patch
Focuses: Cc:

Description

Code to do this:

$permalink_structure = '';
$cat_base = '';
$tag_base = '';

if ( got_mod_rewrite() && is_file(ABSPATH . '.htaccess') && is_writable(ABSPATH . '.htaccess') )
{
	$permalink_structure = '/%year%/%monthnum%/%day%/%postname%/';
}

update_option('permalink_structure', $permalink_structure);
update_option('category_base', $cat_base);
update_option('tag_base', $tag_base);

$wp_rewrite->flush_rules();

The above has been tested by hundreds of users -- this has been built into my theme for over two years.

Attachments (21)

upgrade.diff (881 bytes) - added by mrmist 15 years ago.
wp-admin\includes\upgrade.php diff
upgrade_v2.diff (730 bytes) - added by mrmist 15 years ago.
Alternate version of patch with DD32's comments in mind
6481.diff (555 bytes) - added by Denis-de-Bernardy 15 years ago.
6481.2.diff (1.2 KB) - added by Denis-de-Bernardy 15 years ago.
wp-admin_includes_upgrade.php.diff (3.2 KB) - added by loushou 10 years ago.
wp-admin/includes/upgrade.php 4.0 file patch for pretty permalink test
6481.3.diff (2.4 KB) - added by ericlewis 9 years ago.
6481.4.diff (2.0 KB) - added by valendesigns 9 years ago.
6481.5.diff (2.0 KB) - added by valendesigns 9 years ago.
6481.6.diff (2.4 KB) - added by ericlewis 9 years ago.
6481.7.diff (2.7 KB) - added by valendesigns 9 years ago.
6481.8.diff (2.6 KB) - added by DrewAPicture 9 years ago.
syntax
6481.9.diff (3.7 KB) - added by valendesigns 9 years ago.
6481.10.diff (3.4 KB) - added by valendesigns 9 years ago.
Fixes a typo in the last patch.
6481.11.diff (4.6 KB) - added by dd32 9 years ago.
6481.12.diff (3.1 KB) - added by valendesigns 9 years ago.
6481.13.diff (3.4 KB) - added by valendesigns 9 years ago.
6481.14.diff (3.0 KB) - added by valendesigns 9 years ago.
6481.15.diff (3.0 KB) - added by ericlewis 9 years ago.
6481.16.diff (3.0 KB) - added by ericlewis 9 years ago.
6481.17.diff (656 bytes) - added by valendesigns 9 years ago.
Unplugged.
6481.18.diff (1.2 KB) - added by DrewAPicture 9 years ago.

Download all attachments as: .zip

Change History (128)

#1 @ffemtcj
16 years ago

  • Milestone changed from 2.5.1 to 2.7

Moved to 2.7.. I doubt it will be added to a maint. release. Can be moved to 2.6 if there is a patch added.

#2 @Denis-de-Bernardy
16 years ago

  • Priority changed from normal to low
  • Severity changed from normal to minor

turns out the above code stopped working with wp 2.5

corrected code recreates wp_rewrite before flushing:

$GLOBALS['wp_rewrite'] =& new WP_Rewrite();
$GLOBALS['wp_rewrite']->flush_rules();

#3 @mrmist
16 years ago

I think the "update" lines should be within the if block, otherwise you are calling them unnecessarily on servers that don't have the rewrite functionality.

#4 @ryan
15 years ago

  • Milestone changed from 2.7 to 2.8

#5 @jacobsantos
15 years ago

  • Component changed from General to Permalinks
  • Owner anonymous deleted

#6 @navjotjsingh
15 years ago

  • Milestone changed from 2.8 to 2.9

Do you really think this is needed? If you want it enabled, then what for is the permalink setting page? Leave it to the user. This should not be forced on users.

#7 @DD32
15 years ago

Do you really think this is needed? If you want it enabled, then what for is the permalink setting page? Leave it to the user. This should not be forced on users.

Do you really think this is not needed? If you dont want it enabled, then simply go to the permalink settings page? Set defaults to the most commonly used settings, make life easier for users.

(Kinda sarcastic there btw)

I've always wondered why fancy permalinks are not enabled by default, Granted, They're not hard to enable, And its simple to tell if its going to work (in most cases). I guess the fact it relies on the .htaccess being listened to is the main reasoning why its not, afterall, there are other web servers than apache.

#8 @mrmist
15 years ago

  • Keywords needs-patch added

If this is going in then it just needs patchifying really. I'd do it myself but I've no idea which file this would belong in.

#9 @Denis-de-Bernardy
15 years ago

@mrmist: maybe in wp-admin/includes/upgrade.php, in function wp_install()

@mrmist
15 years ago

wp-admin\includes\upgrade.php diff

#10 @mrmist
15 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from 2.9 to 2.8
  • Version changed from 2.5 to 2.7

Attached patch containing code suggested above.

Tested against stock 2.7 on a php5 / apache system and worked as expected (pretty permalinks on). Could probably do with a quick check on a rewrite system I guess.

#11 @DD32
15 years ago

attachment upgrade.diff added.

Questions:

  • Why is not everything inside the if block?
  • Why update the tag/cat base? They're just being set to the default anyway
  • Is a reference really required for the WP_Query object?

@mrmist
15 years ago

Alternate version of patch with DD32's comments in mind

#12 @Denis-de-Bernardy
15 years ago

@DD32: The reference was for old issues on the shutdown hook. I take it they've probably been fixed since.

#13 @mrmist
15 years ago

  • Keywords tested 2nd-opinion added

#14 @rowoot
15 years ago

  • Keywords 3rd-opinion added; 2nd-opinion removed

tried upgrade_v2.diff on the nightly build and seems to work fine.

#15 follow-up: @ryan
15 years ago

After flushing, we should check to make sure .htaccess exists and that it actually contains rules. If not, use unfancy links.

#16 @ryan
15 years ago

Also, use $wp_rewrite->set_permalink_structure(). You won't need to update the option or create a new WP_Rewrite option then.

#17 @Denis-de-Bernardy
15 years ago

  • Keywords commit added; 3rd-opinion removed

done

#18 in reply to: ↑ 15 @Denis-de-Bernardy
15 years ago

Replying to ryan:

After flushing, we should check to make sure .htaccess exists and that it actually contains rules. If not, use unfancy links.

won't this achieve this already?

is_file(ABSPATH . '.htaccess') && is_writable(ABSPATH . '.htaccess')

#19 follow-up: @DD32
15 years ago

won't this achieve this already?

That only checks the file exists, Not that it contains the correct rules, Nor does it check that the web server abides by those rules.

The check in my mind should be a HTTP Request to a rewritten URL, That'll ensure that the rules have taken and the server supports it.

#20 in reply to: ↑ 19 @Denis-de-Bernardy
15 years ago

Replying to DD32:

won't this achieve this already?

That only checks the file exists, Not that it contains the correct rules, Nor does it check that the web server abides by those rules.

The check in my mind should be a HTTP Request to a rewritten URL, That'll ensure that the rules have taken and the server supports it.

I would have hoped that, if the file is writable it would get written. but yeah, there are servers out there that do this wrongly. then again, these won't get caught by settings / permalinks either, will they?

#21 @DD32
15 years ago

I would have hoped that, if the file is writable it would get written.

Yeah.. you'd hope so..

then again, these won't get caught by settings / permalinks either, will they?

Nope, But thats beside the point, It should check it there as well (That was mentioned many moons ago too..). But more of the fact, Since this is a default, It needs to default to something that DOES work, non-fancy permalinks always do work, which is why they're the defacto standard, To change it, The checks need to make absolutely sure they work IMO

#22 @Denis-de-Bernardy
15 years ago

the updated patch moves the burden of making sure things got changed to the save_mod_rewrite_rules() function.

#23 @Denis-de-Bernardy
15 years ago

oh, I think I found the ticket where writable htaccess files don't get written: #9246

#24 follow-up: @ryan
15 years ago

Maybe do a head request on get_permalink(1) and make sure it doesn't 404?

#25 @Denis-de-Bernardy
15 years ago

or we could simply make sure that the file_get_contents(ABSPATH / .htaccess) contains the relevant rules, but the real point is: what should happen when the rules are not contained.

it could have two reasons:

  • file is not writable
  • file is writable but we failed to write to it

we currently manage the first, but not the second.

#26 @Denis-de-Bernardy
15 years ago

  • Keywords dev-feedback added; commit removed

#27 @janeforshort
15 years ago

  • Milestone changed from 2.8 to Future Release

Punting due to time constraints, will consider in next release cycle.

#28 @Denis-de-Bernardy
15 years ago

  • Milestone changed from Future Release to 2.8

#29 @hakre
15 years ago

What about offering (a basic) Permalink Config with the Installer? This could enable it as well and if something goes wrong it is not that problematic. Compare to if the Installer fails to init the DB.

#30 @Denis-de-Bernardy
15 years ago

  • Milestone changed from 2.8 to Future Release

moving this to future. maybe someone will give it some love.

#31 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added; has-patch tested dev-feedback removed

amazing how these new reports lead straight to bugs that need attention.

#32 @Denis-de-Bernardy
14 years ago

See aso #12404 and #12405, with an extra discussion on using query string based permalinks

#33 @nacin
14 years ago

I ended up closing #12404 as a duplicate, with some thoughts included in a comment there.

#34 @mikeschinkel
14 years ago

  • Cc mikeschinkel@… added

#35 follow-ups: @GaryJ
13 years ago

Since #16687 is now done that stops the basic /%postname%/ having a performance penalty, and r18547 adds the option to the UI, would it make more sense for default permalinks to use this most simplified pretty permalink format available, rather than something with extraneous time-based components?

#36 @mbijon
12 years ago

  • Cc mike@… added

#37 @dd32
11 years ago

#22147 was marked as a duplicate.

#38 in reply to: ↑ 35 @TomAuger
11 years ago

  • Cc tomaugerdotcom@… added

Replying to GaryJ:

Since #16687 is now done that stops the basic /%postname%/ having a performance penalty, and r18547 adds the option to the UI, would it make more sense for default permalinks to use this most simplified pretty permalink format available, rather than something with extraneous time-based components?

I'd agree with that. Anything blocking this patch from being committed?

#39 @Ipstenu
11 years ago

Since this patch came out, nginx has risen to be a common use case (vs edge). We're not even auto-inserting/suggesting the conf rules yet. Not to mention iis.

It certainly seems possible to get this up for .htaccess, but we're leaving the others out in the cold.

#40 @TomAuger
11 years ago

I haven't dug into the actual .htaccess/server part of rewrite, but I imagine we have solutions that work on nginx and iis? Just use those once we've detected the web server platform?

#41 in reply to: ↑ 24 @Denis-de-Bernardy
10 years ago

Replying to ryan:

Maybe do a head request on get_permalink(1) and make sure it doesn't 404?

That wouldn't work if a plugin flushes the content. That would yield no post, and a whole bunch of candidate test URIs return a 404 when they really should return a 200 with no content.

I did eventually locate a test uri that doesn't seem to fail when no posts are around, though. This from my plugin:

    /**
     * Enables permalinks on new installs
     *
     * References:
     *
     * - https://core.trac.wordpress.org/ticket/6481
     **/
    protected function setPermalinkStructure()
    {
        # Bail if a permalink structure is already set
        if (get_option('permalink_structure')) return;

        # Bail unless we're running on top of Apache
        # FIXME: look into making it work with nginx and iis
        global $is_apache;
        if (!isset($is_apache) || !$is_apache || !got_mod_rewrite()) return;
        
        # Bail if we fail to fetch the admin user
        $admin = isset($_POST['user_name']) ? get_user_by('login', $_POST['user_name']) : false;
        if (!$admin || is_wp_error($admin)) return;
    
        # Try to set permalink structure
        global $wp_rewrite;
        $wp_rewrite->set_permalink_structure(DEFAULT_PERMALINK_STRUCTURE);
        flush_rewrite_rules();
    
        # Rollback if permalinks aren't working
        $test_url = get_author_posts_url($admin->ID, $admin->user_nicename);
        $response = wp_remote_head($test_url, array('timeout' => 5, 'httpversion' => '1.1'));
        if (is_wp_error($response) || 200 != wp_remote_retrieve_response_code($response)) {
            $wp_rewrite->set_permalink_structure('');
            flush_rewrite_rules();
        }
    }

#42 @jenmylo
10 years ago

  • Priority changed from low to normal

I would really really like us to switch over to pretty permalinks as the default for the sake of a better user experience. If there are current reasons *not* to do this, can folks list them? I'm bumping this up to "normal" from "low," because while it is probably a low priority in terms of the code base, it's been a n ongoing annoyance on the ux side for years. Would be great to get this addressed in 3.9 if possible.

#43 follow-up: @aaroncampbell
10 years ago

So basically what we need is:

  • Update permalink structure using $wp_rewrite->set_permalink_structure();
  • Update .htaccess or web.config by hard flushing the rewrite rules flush_rewrite_rules()
  • Check for 404s or 500s using a head request
    • This means we need some sort of content to test. If no posts or terms exist, maybe the author page for the first user

This should give us pretty permalinks for sites on Apache and IIS that have a writable config file that allows for rewrites, but only if that file is processed immediately and not on a delay.

It's certainly not everyone, and I definitely think we need to code really defensively to make sure that we fall back to standard permalinks if we're not 100% sure pretty permalinks are working, but it seems doable. It also seems like a nice step forward for the web in general.

#44 in reply to: ↑ 43 @jenmylo
10 years ago

Replying to aaroncampbell:

It's certainly not everyone, and I definitely think we need to code really defensively to make sure that we fall back to standard permalinks if we're not 100% sure pretty permalinks are working, but it seems doable. It also seems like a nice step forward for the web in general.

Agreed on having a fallback if an install doesn't pass the test to allow pretty permalinks. But that would still mean some percentage of users that is much lower than 100% would have ugly permalinks as the default, and a percentage of users much higher than 0% would default to pretty, a definite win.

#45 @loushou
10 years ago

  • Keywords has-patch added; needs-patch removed

After reading this whole ticket, there are some good ideas here. It seems like we have also overcome at least half of the only real objection, since we write the .htaccess and web.config files. Only Nginx is on the outs. Thankfully, the code (and my patch) is written in such a way that even if we wind up adding the Nginx config writer later, another change should not be needed.

My approach is simple. During the installation process, after we create our dummy data, it simply attempts to set a basic pretty permalink structure, and to subsequently hit that dummy post at it's pretty url. If we get a 200 response, the we can use pretty permalinks by default. Otherwise we just fallback to the ugly permalinks (coding defensively, heh).

I'll attach the patch.

@loushou
10 years ago

wp-admin/includes/upgrade.php 4.0 file patch for pretty permalink test

This ticket was mentioned in Slack in #core by eric. View the logs.


9 years ago

@ericlewis
9 years ago

#47 @ericlewis
9 years ago

In attachment:6481.3.diff, I iterated on attachment:wp-admin_includes_upgrade.php.diff.

  • I believe pluggable functions are considered an anti-pattern at this point (correct me if I'm wrong) because of the headache in backwards compatibility they proffer, and if we want to offer flexibility in our API we can do it in more forward-thinking ways (e.g. hooks API).
  • Instead of setting the permalink structure here, let's set it at the options defaults, which delightfully brings together a fork of functionality between multisite and single site.
  • For now, keep this smoke testing separate from multisite. Preferably single and multi share this functionality, but for reasons I haven't dug into yet, the smoke test failed for a fresh multisite.
  • General code clean-up - wp_install_verify_permalink_structure() is a single-responsibility function to smoke test the "Hello World" HTTP response code, use wp_remote_head() instead of wp_remote_request, etc.

This ticket was mentioned in Slack in #core by eric. View the logs.


9 years ago

#49 @ericlewis
9 years ago

  • Keywords dev-feedback added
  • Milestone changed from Future Release to 4.1

#50 @ericlewis
9 years ago

  • Milestone changed from 4.1 to 4.2

@valendesigns
9 years ago

#51 follow-up: @valendesigns
9 years ago

My patch iterates on 6481.3.diff and removes the unnecessary wp_install_verify_permalink_structure() function and generally cleans things up a bit.

@valendesigns
9 years ago

#52 @valendesigns
9 years ago

Sorry, one more patch. I had to make a small change in the inline comments now that the code is not being called from a function.

#53 in reply to: ↑ 51 @ericlewis
9 years ago

Replying to valendesigns:

My patch iterates on 6481.3.diff and removes the unnecessary wp_install_verify_permalink_structure() function and generally cleans things up a bit.

Hi valendesigns, thanks for your work here. However, cramming more lines into an already quite long function increases our code's complexity. Creating single responsibility functions keeps our code legible and less complex. We should probably have developer documentation for practices like these.

Last edited 9 years ago by ericlewis (previous) (diff)

#54 follow-up: @nacin
9 years ago

valendesigns' patch moves the logic into wp_install_defaults(), which actually highlights an issue I was going to point out. If wp_install_defaults() is overridden and no defaults are actually created, then these would fail. But there's no reason we need to do that.

When get_permalink( 1 ) returns false, we could just as easily hit /some-random-404-page/ and as long as we get back a response from the WordPress install, then we know rewrites work. That could be as easy as looking for a X-Pingback header. I don't think we need to account for this but I'd like to see if we don't need make assumptions that any default data is in place, or even that its ID is 1.

Also, I don't think this code belongs in wp_install_defaults(). It's not default content; that function can be overridden; and there's really no harm in leaving it in its own function.

Really happy to see this on track for inclusion.

@ericlewis
9 years ago

#55 in reply to: ↑ 54 @ericlewis
9 years ago

Replying to nacin:

valendesigns' patch moves the logic into wp_install_defaults(), which actually highlights an issue I was going to point out. If wp_install_defaults() is overridden and no defaults are actually created, then these would fail. But there's no reason we need to do that.

Moved the invocation of wp_install_verify_pretty_permalinks_work() to wp_install().

we could just as easily hit /some-random-404-page/ and as long as we get back a response from the WordPress install, then we know rewrites work. That could be as easy as looking for a X-Pingback header.

Changed the smoke test accordingly.

@valendesigns
9 years ago

#56 @valendesigns
9 years ago

I've made a couple minor changes to 6481.6.diff, very nice iteration by the way. This new patch decouples the function completely. As well, I changed the function name, because adding work seems overly verbose and redundant when using verify. I also added additional information in the docblock and wrapped the function in a function_exists call like the ones around it.

Cheers,
Derek

@DrewAPicture
9 years ago

syntax

#57 @DrewAPicture
9 years ago

@valendesigns: FYI, in 6481.8.diff I've made a couple of tweaks to fix documentation and coding standards syntax:

  • The function_exists() call was spaced out
  • Spaced out the code a little bit and fixed the multi-line comment syntax
  • Documented the global, $wp_rewrite
  • Removed the @return void from the DocBlock per core inline docs standards

#58 follow-up: @nacin
9 years ago

Other thoughts:

  • Do we still want to avoid get_permalink(1) as a first attempt at getting a URL to hit? I thought it was clever, just perhaps deserved a fallback.
  • What about PATHINFO links?

#59 in reply to: ↑ 58 ; follow-up: @ericlewis
9 years ago

Repling to valendesigns:

I wrapped the function in a function_exists call like the ones around it.

I mentioned earlier wrapping in a function_exists check is probably the wrong way to go here.

Replying to nacin:

Other thoughts:

  • Do we still want to avoid get_permalink(1) as a first attempt at getting a URL to hit? I thought it was clever, just perhaps deserved a fallback.

If we can count on a 404 page with the x-pingback header, checking the Hello World post that may or may not exist seems unnecessary.

  • What about PATHINFO links?

What are PATHINFO links?

#60 in reply to: ↑ 59 ; follow-up: @valendesigns
9 years ago

Replying to ericlewis:

I mentioned earlier wrapping in a function_exists check is probably the wrong way to go here.

Probably, or is the wrong way to go? I would prefer a function_exists check around all functions, but that's just my preference and one I'm willing change if needed. Where did you read or hear that it's an anti-pattern? If we remove the function_exists then I suggest we include a filter on the URL or an action at the end, or both. Something to make it extensible.

Replying to DrewAPicture:

@valendesigns: FYI, in 6481.8.diff​ I've made a couple of tweaks to fix documentation and coding standards syntax:

Thank you! I'm still new around here so there are some coding nuances I still need to get use to.

Replying to nacin:

What about PATHINFO links?

I don't see how pathinfo() would benefit us in this situation. We're trying to establish a fake, but valid URI, not information about a file path; you must know a trick that I've not seen before. In any case, as long as we can count on the 404 x-pingback header then we're good. However, if you think that's not the case, we should definitely add a second check to fallback on.

#61 follow-up: @dd32
9 years ago

I don't see how pathinfo() would benefit us in this situation.

Not the pathinfo() function, but rather the Pathinfo-based rewrites which some web servers (including apache) support

Various forms of permalinks:

  • None: /site/index.php?p=123
  • Rewrites: /site/2014/12/25/my-post-title/
  • Pathinfo: /site/index.php/2014/12/25/my-post-title/

See http://codex.wordpress.org/Using_Permalinks#PATHINFO:_.22Almost_Pretty.22

Last edited 9 years ago by dd32 (previous) (diff)

#62 in reply to: ↑ 61 ; follow-up: @valendesigns
9 years ago

Replying to dd32:

Not the pathinfo() function, but rather the Pathinfo-based rewrites which some web servers (including apache) support

Thanks for the link. Learn something new everyday. However, since they are "Almost Pretty Permalinks" it seems we would be testing for something that isn't "Pretty Permalinks" during the verification process, and therefore not equal.

#63 in reply to: ↑ 60 @ericlewis
9 years ago

Replying to valendesigns:

Probably, or is the wrong way to go? I would prefer a function_exists check around all functions, but that's just my preference and one I'm willing change if needed. Where did you read or hear that it's an anti-pattern?

JeremyClarke added this to the Pluggable Functions Codex page a while ago:

Pluggable functions are no longer being added to WordPress core. All new functions instead use filters on their output to allow for similar overriding of their functionality.

I wasn't around for 1.5 when pluggable functions were introduced. Someone with a better historical perspective can chime in here, but my thought is pluggable functions make backwards compatibility harder. You have to support a developer doing anything in the world they want to in their version of the function. Offering developers a hook to noop a function with their callback as in the options API gives more control over changing the internals of a function. Also, as only one function can be defined in PHP, two plugins can't both play nicely in this context. The hooks API is a stable, robust way to extend functionality.

#64 in reply to: ↑ 62 @dd32
9 years ago

Replying to valendesigns:

However, since they are "Almost Pretty Permalinks" it seems we would be testing for something that isn't "Pretty Permalinks" during the verification process, and therefore not equal.

Sure, they're not called the same thing in the Codex, but it's effectively the same thing, and doesn't require significant amounts of code.
The user experience is far better though than a ?p=123, so it seems worthwhile including it.

I'd also rename /some-random-404-page/ to /wordpress-check-for-rewrites/ or something descriptive.

Pluggable functions are no longer being added to WordPress core. All new functions instead use filters on their output to allow for similar overriding of their functionality.

This might be one of the locations that Pluggable functions are still expected, purely because of the install.php dropin that can be added, although I don't see the need to make it pluggable other than for consistency.

@valendesigns
9 years ago

#65 @valendesigns
9 years ago

Here's a patch that includes a fallback and filters the boolean value of $pretty_permalinks. Maybe someone can add a third check using PATHINFO links. Which order should the test be in?

@valendesigns
9 years ago

Fixes a typo in the last patch.

@dd32
9 years ago

#66 follow-up: @dd32
9 years ago

6481.11.diff builds off 6481.10.diff and takes a slightly different direction

  • Permalinks are assumed not to work, and enables them if possible (rather than enabling, then verifying later it works)
  • Supports index.php prefixed paths
  • Applies a 5 second request timeout to the loopback connection
  • Tested under nginx with the following configurations:
    • No Rewrites possible (all 404)
    • /%year%/%monthnum%/%day%/%postname%/ rewrite support
    • /index.php/%year%/%monthnum%/%day%/%postname%/ rewrite support
    • Rewrites hitting a different blog (x-pingback != current install)

Thoughts?

I thought of adding a filter onto $permalink_structures, however if you wish to customize it, it's probably better to hook onto wp_install and perform all custom site setup after WordPress is done.

#67 in reply to: ↑ 66 @valendesigns
9 years ago

Replying to dd32:

6481.11.diff builds off 6481.10.diff and takes a slightly different direction

  • Permalinks are assumed not to work, and enables them if possible (rather than enabling, then verifying later it works)
  • Supports index.php prefixed paths
  • Applies a 5 second request timeout to the loopback connection
  • Tested under nginx with the following configurations:
    • No Rewrites possible (all 404)
    • /%year%/%monthnum%/%day%/%postname%/ rewrite support
    • /index.php/%year%/%monthnum%/%day%/%postname%/ rewrite support
    • Rewrites hitting a different blog (x-pingback != current install)

Thoughts?

I thought of adding a filter onto $permalink_structures, however if you wish to customize it, it's probably better to hook onto wp_install and perform all custom site setup after WordPress is done.

+1 This is a very nice iteration. One change that isn't entirely necessary, but might be useful, would be a filter on the $permalink_structures array. Otherwise, I vote commit.

EDIT: Sorry I should have also said, so you don't have to overload the function to change it.

Last edited 9 years ago by valendesigns (previous) (diff)

@valendesigns
9 years ago

#68 @valendesigns
9 years ago

I've added a patch that includes the filter. Which would mean you could set the first item in the array to the structure you want to use in your config.php file before you install and have permalinks setup exactly how you want from the very beginning without overloading the function. Which to me sounds like a better option that is more backwards compatible.

#69 follow-up: @DrewAPicture
9 years ago

@valendesigns: In 6481.12.diff, a couple of adjustments need to be made to the docs:

  • Syntax for single-line comments is simply //
  • We use a standard vernacular in hook doc summaries, e.g. "Filter the permalink structures for X" followed by a blank line, then the description, wrapped at 80-120 characters depending on how far the docblock is already indented.
  • Hook docblocks open with /** instead of /* to differentiate them from multi-line comments for the parser.

#70 in reply to: ↑ 69 @valendesigns
9 years ago

Replying to DrewAPicture:

@valendesigns: In 6481.12.diff, a couple of adjustments need to be made to the docs:

  • Syntax for single-line comments is simply //
  • We use a standard vernacular in hook doc summaries, e.g. "Filter the permalink structures for X" followed by a blank line, then the description, wrapped at 80-120 characters depending on how far the docblock is already indented.
  • Hook docblocks open with /** instead of /* to differentiate them from multi-line comments for the parser.

Working on a refresh now. Thank you for the critique and not just creating another patch. I need to learn these things and your comments are helpful.

@valendesigns
9 years ago

#71 @valendesigns
9 years ago

I've added patch 6481.13.diff that should now conform to documentation standards. Cheers!

#72 follow-up: @dd32
9 years ago

I've added a patch that includes the filter.

The only issues I see with the filter (and I'm not totally against adding it) is

  1. Encouraging all customizations to take place on the wp_install hook is advisable
  2. The new filter won't be used for MultiSite (which is arguably the most useful reason for a filter) as the option will already be set for MultiSite (and MultiSite requires rewrites to be working, so there's no need to maybe enable it)
  3. You can't use either filter from wp-config.php, you have to use a install.php dropin for single-site install, and a mu-plugin for multisite
  4. Based on the above, to me it seems simplest to leave the filter out of this method, as we've done with the others.

Filtering everything is great and all, but adding filters everywhere that adds no real benefit that can be taken advantage of isn't advisable, it just causes confusion for those reading the documentation.

@valendesigns
9 years ago

#73 in reply to: ↑ 72 @valendesigns
9 years ago

Replying to dd32:

Filtering everything is great and all, but adding filters everywhere that adds no real benefit that can be taken advantage of isn't advisable, it just causes confusion for those reading the documentation.

Well. Can't really argue with that logic, if you can't use the filter from config.php. I've refreshed the patch to conform to documentation standards and removed the filter. Cheers!

This ticket was mentioned in Slack in #core by eric. View the logs.


9 years ago

#75 @dd32
9 years ago

In 31089:

Enable Pretty Permalinks for new sites during install if the server supports it.
See #6481, Props valendesigns, DrewAPicture & ericlewis

#76 @dd32
9 years ago

  • Keywords dev-feedback removed

This ticket was mentioned in Slack in #core by jorbin. View the logs.


9 years ago

#78 in reply to: ↑ 35 ; follow-ups: @GaryJ
9 years ago

Replying to GaryJ:

Since #16687 is now done that stops the basic /%postname%/ having a performance penalty, and r18547 adds the option to the UI, would it make more sense for default permalinks to use this most simplified pretty permalink format available, rather than something with extraneous time-based components?

What's the rationale for sticking with the time-based format, instead of the shorter /%postname%/ format?

#79 in reply to: ↑ 78 @valendesigns
9 years ago

Replying to GaryJ:

What's the rationale for sticking with the time-based format, instead of the shorter /%postname%/ format?

Doesn't /%postname%/ have worse performance than the ones with dates, because of the amount of potential matches?

#80 in reply to: ↑ 78 @helen
9 years ago

Replying to GaryJ:

What's the rationale for sticking with the time-based format, instead of the shorter /%postname%/ format?

Per nacin:

/year/month/day/postname/ is a classic and a stalwart of clean, pretty, informative URLs.

I would agree. I also still find that most sites that use posts as a major vehicle (as opposed to pages or other content types) are date-sensitive.

#81 @ericlewis
9 years ago

  • Severity changed from minor to normal

#82 follow-up: @joostdevalk
9 years ago

I would disagree on that rationale, switching to just %%postname%% makes much more sense to me... Every SEO guide on the planet tells you to do this for all the right reasons...

#83 in reply to: ↑ 82 ; follow-up: @ericlewis
9 years ago

Replying to joostdevalk:

I would disagree on that rationale, switching to just %%postname%% makes much more sense to me... Every SEO guide on the planet tells you to do this for all the right reasons...

Care to expand on the right reasons?

This ticket was mentioned in Slack in #core by eric. View the logs.


9 years ago

@ericlewis
9 years ago

@ericlewis
9 years ago

#85 @ericlewis
9 years ago

In attachment:6481.16.diff, more concise documentation.

#86 @nacin
9 years ago

I would like to unplug this function. There's no need for it to be overridden. I understand the consistency argument, but only five of the functions in this entire file are overridable. They fall into either two categories: some crazy, complicated thing you must be insane to want to override, or wp_install_defaults(), which is pluggable because it makes a decent amount of sense.

At this time in the install process, the only stuff that is actually loaded is wp-content/install.php. This suggests an advanced use case already. Plugging this function would only serve to stop it (which, no, there's no desire to encourage ugly permalinks) or to set a custom permalink structure. If you wanted to set a custom permalink structure, you could simply do so on the wp_install hook or in wp_install(). I just don't see a wide use case for needing to override this particular function. (And I am one person on a very small list of people who have ever used wp-content/install.php, I bet.)

The five other pluggable functions here were all added in 2.1. Across all of core, the last one we added was in 3.0. They're a dead pattern (anti-pattern, even) we should avoid.

#87 in reply to: ↑ 83 ; follow-ups: @joostdevalk
9 years ago

Replying to ericlewis:

Replying to joostdevalk:

I would disagree on that rationale, switching to just %%postname%% makes much more sense to me... Every SEO guide on the planet tells you to do this for all the right reasons...

Care to expand on the right reasons?

From a usability perspective, good URLs should be short and memorable, including the date automatically makes them longer and harder to remember, without adding much. We only need postname, why include more? The date is meta data, no more important than say the author, we don't include that in the permalink either.

Also, if you decide to re-publish a piece because you've updated it (after all, that's what a Content Management System is for) and change the date publication date, the URL would change.

Going for date / postname assumes far too much about the type of news posts people write. If the general feeling is towards adding dates, we could also make this a question during the installation process?

"Do you want to include the date in the URLs for your posts? Yes / No"

My personal choice would be to go for just %%postname%% by default though.

This ticket was mentioned in Slack in #core by joostdevalk. View the logs.


9 years ago

@valendesigns
9 years ago

Unplugged.

#89 @valendesigns
9 years ago

6481.17.diff unplugs the function.

#90 in reply to: ↑ 87 ; follow-up: @cais
9 years ago

Replying to joostdevalk:

Replying to ericlewis:

Replying to joostdevalk:

I would disagree on that rationale, switching to just %%postname%% makes much more sense to me... Every SEO guide on the planet tells you to do this for all the right reasons...

Care to expand on the right reasons?

From a usability perspective, good URLs should be short and memorable, including the date automatically makes them longer and harder to remember, without adding much. We only need postname, why include more? The date is meta data, no more important than say the author, we don't include that in the permalink either.

...

"Do you want to include the date in the URLs for your posts? Yes / No"

My personal choice would be to go for just %%postname%% by default though.

I think this must be the most succinct and simple reason to move to %%postname%% as the default that I have read ... compelling enough that I am going to change my sites from "Month and name" to "Post name" right now.

#91 in reply to: ↑ 90 ; follow-ups: @Denis-de-Bernardy
9 years ago

Replying to cais:

I think this must be the most succinct and simple reason to move to %%postname%% as the default that I have read ... compelling enough that I am going to change my sites from "Month and name" to "Post name" right now.

Then again it's not necessarily advisable for other reasons, at least in the majority of cases. Unless you're constantly producing evergreen content, you actually *want* the date to be around for usability reasons. If it's in the url all the better. Plus, you get the archive links to work by simply removing the post name, and then the day, month and year.

#92 in reply to: ↑ 91 @cais
9 years ago

Replying to Denis-de-Bernardy:

Replying to cais:

I think this must be the most succinct and simple reason to move to %%postname%% as the default that I have read ... compelling enough that I am going to change my sites from "Month and name" to "Post name" right now.

Then again it's not necessarily advisable for other reasons, at least in the majority of cases. Unless you're constantly producing evergreen content, you actually *want* the date to be around for usability reasons. If it's in the url all the better. Plus, you get the archive links to work by simply removing the post name, and then the day, month and year.

An interesting point to make with the ease of locating date based archives by manually adjusting the URL in the address bar, this would definitely be of use if the site's current theme provides a date.php template (or similar code) to emphasize that distinction.

Although the changes are already in place (I've been using "Month and name" for almost as long as this ticket has been open) I still may switch back; but, I am willing to give it at least a week or two to see what happens to traffic flow on my sites.

Last edited 9 years ago by cais (previous) (diff)

#93 in reply to: ↑ 91 @Kenshino
9 years ago

Replying to Denis-de-Bernardy:

Replying to cais:

I think this must be the most succinct and simple reason to move to %%postname%% as the default that I have read ... compelling enough that I am going to change my sites from "Month and name" to "Post name" right now.

Then again it's not necessarily advisable for other reasons, at least in the majority of cases. Unless you're constantly producing evergreen content, you actually *want* the date to be around for usability reasons. If it's in the url all the better. Plus, you get the archive links to work by simply removing the post name, and then the day, month and year.

What is this usability that we keep talking about?

If we think about the actual use case -

Users are more likely to encounter a page or a post via 2 main ways

A menu or a listing on the WordPress installation or a Google Search Result.

A well designed listing that is date important would have been sorted and labeled according using the date meta. The URL would not have been in consideration for actual User Experience.

A Google Search result would not show the url directly, it would show the SEO title. A well implemented SiteMap would provide the date data to Google Search results and show it accordingly.

The only time when one actually likely looks at the date in the url is after one actually clicks on it. Good usability, good user experience prevents people from clicking on the wrong things. Looking at the URL for the correct date is a step too late.

#94 @dd32
9 years ago

In 31159:

The new wp_install_maybe_enable_pretty_permalinks() function doesn't need to be pluggable.
See #6481. Props nacin, valendesigns

#95 @dd32
9 years ago

In 31160:

More concise documentation for wp_install_maybe_enable_pretty_permalinks()
See #6481. Props ericlewis.

#96 in reply to: ↑ 87 ; follow-up: @greenshady
9 years ago

We should all probably take a step back and think about what blog posts are. They are date-sensitive content types. Outside of the post content and title, the date is often the most important information about the post. Of course, this is not true for every blog, just most of them.

The main reason I'd argue to keep the /%year%/%monthnum%/%day%/%postname%/ structure is because that's always been (AFAIK) WP's default pretty permalink structure. It's that first option to get away from ugly permalinks. I don't see any reason to change that.

Is that structure bad SEO? It's never been an issue for me. I imagine it plays a minor, if any, role in SEO, despite what "every SEO guide on the planet" has to say about it. I've used various structures on different sites over the years and have yet to see a benefit of any of them as far as SEO is concerned.

The best thing about all of this is that you have the same choice of changing the permalink structure that you've always had.

Replying to joostdevalk:

From a usability perspective, good URLs should be short and memorable, including the date automatically makes them longer and harder to remember, without adding much.

I can see remembering a Web site's URL, but who actually remembers post URLs? That's what bookmarks and other systems for saving URLs are for.

The date is meta data, no more important than say the author, we don't include that in the permalink either.

I've worked with several people who include the author in the permalink. It just depends on what's needed for the specific site.

Going for date / postname assumes far too much about the type of news posts people write.

Going for /%postname% equally assumes far too much, maybe even more considering that blog posts are date-sensitive content types on most blogs. Anything we go with would be making an assumption one way or another.

If the general feeling is towards adding dates, we could also make this a question during the installation process?

Many, I'd wager, wouldn't know what to do with that question when first installing WordPress. That's more of an advanced setting that probably doesn't belong in installation.

Replying to Kenshino:

The only time when one actually likely looks at the date in the url is after one actually clicks on it. Good usability, good user experience prevents people from clicking on the wrong things. Looking at the URL for the correct date is a step too late.

The URL is the first place I and many others look for the date. Since my eyes are already focused on the URL when clicking a link, I immediately notice a date. That's not too late. It's right on time. And, it's very helpful info. It tells me two things right off the bat:

  • What the actual published date is.
  • That I can most likely drill down through the archive hierarchy provided there are no links to do so.

Neither of these things are actually necessary. However, they do add some additional value for some users.

#97 in reply to: ↑ 96 ; follow-up: @GaryJ
9 years ago

Any date indicator in a URL gives no definitive indication of when post content was last modified. If I write an amazing post at example.com/2010/foobar but update it regularly, it sounds like you would not even click through to it if it were linked from somewhere else? Even if I turn that into a page, and point the 2010 post to it, any old link is still misdirecting about the content which is the important thing here. While you're looking for the published date, it's the last modified date that, if present, is potentially more informative.

Take W3C specs as an example - there might be many revisions, and date-laden URLs, but the true or latest version can (nearly?) always be found at a URL without a date.

As a counter example to not remembering or discovering URLs, take a URL that indicates a series: example.com/foobar-part-1. Even if there were no links to the later post, a curious visitor could make an educated guess where part 2 would be. With a URL like example.com/2011/11/30/foobar-part-1, it's simply not directly guessable.

As for drilling down through the date archives - sure, you couldn't do that with %postname%, but most WP themes display some sort of linked categorical taxonomy, irrespective of the URL, and that is arguably more useful than searching by date.

I do agree that it shouldn't be part of the install process.

#98 in reply to: ↑ 97 @Denis-de-Bernardy
9 years ago

Replying to GaryJ:

If I write an amazing post at example.com/2010/foobar but update it regularly, it sounds like you would not even click through to it if it were linked from somewhere else?

Not "sounds like you would not even click." It's simply "will not click." Because depending on the type of information, I and other users who have been trained to discriminate information based on its author or age will go the extra mile to actually get this type of meta data before even bothering to read anything of non-trivial length.

Even if I turn that into a page, and point the 2010 post to it, any old link is still misdirecting about the content which is the important thing here.

Google will show the newer url if you do the redirection properly. Old links won't, but that matters little because they'd be discussing your old information anyway.

While you're looking for the published date, it's the last modified date that, if present, is potentially more informative.

It's not. If you're posting evergreen information in blog posts, I'm sorry to say but you're doing it wrong. Use static pages instead. And use blog posts (or better yet, your newsletter) to mention your new page and updates to it.

Take W3C specs as an example - there might be many revisions, and date-laden URLs, but the true or latest version can (nearly?) always be found at a URL without a date.

It's not a blog post. It's a spec. It's draft versions (dated, or should be) vs evergreen content (non-dated).

As a counter example to not remembering or discovering URLs, take a URL that indicates a series: example.com/foobar-part-1. Even if there were no links to the later post, a curious visitor could make an educated guess where part 2 would be. With a URL like example.com/2011/11/30/foobar-part-1, it's simply not directly guessable.

Then that's a UI/UX problem on your blog. If your foobar-part-1 post isn't linking to foobar-part-2 post and vice-versa, you need to revisit the navigation of your site's blog section or update your posts to work around its clunkiness. It has nothing to do with the url format.

As for drilling down through the date archives - sure, you couldn't do that with %postname%, but most WP themes display some sort of linked categorical taxonomy, irrespective of the URL, and that is arguably more useful than searching by date.

And their UI/UX sucks. But that is another problem altogether, which has nothing to do with good url usability. The latter has three simple rules:

  1. The url should give good hints on what the page is about.
  2. And some kind of date information (year/month or year/month/day) if it's time sensitive.
  3. And some kind of author information if it's author sensitive. (Think academics rather than newspapers.)

As noted by Justin, most blog posts fall under the time sensitive rule, so using a date in the url is an excellent default.

Last edited 9 years ago by Denis-de-Bernardy (previous) (diff)

#99 follow-up: @dd32
9 years ago

The main reason I'd argue to keep the /%year%/%monthnum%/%day%/%postname%/ structure is because that's always been (AFAIK) WP's default pretty permalink structure. It's that first option to get away from ugly permalinks. I don't see any reason to change that.

This is why I stuck with the classic WordPress permalinks, Most of the rest of this discussion is bikeshedding, different people prefer different permalink formats for different reasons, and others will strongly disagree with those reasons.

The user still has a choice that they can optionally make post-install, we're not locking them into any specific format, simply making the experience better out-of-the-box.


One remaining thing for this ticket is, do we rename the "Default" ?p=123 format on the Permalinks page? do we leave it as Default? Do we want to update the description text?

By default WordPress uses web URLs which have question marks and lots of numbers in them; however, WordPress offers you the ability to create a custom URL structure for your permalinks and archives. This can improve the aesthetics, usability, and forward-compatibility of your links. A number of tags are available, and here are some examples to get you started.

#100 in reply to: ↑ 99 @valendesigns
9 years ago

Replying to dd32:

The user still has a choice that they can optionally make post-install, we're not locking them into any specific format, simply making the experience better out-of-the-box.

I fully agree. This is an enhancement to the UX during installation, so any choices we make about default permalink structures in this ticket are an assumption and subjective. We're not all going to agree with each other, but we should be able to agree that it's a step in the right direction, which is away from ugly permalinks.

One remaining thing for this ticket is, do we rename the "Default" ?p=123 format on the Permalinks page? do we leave it as Default? Do we want to update the description text?

Technically the default is still ugly permalinks. So any string changes should only be made if our new function updates the default away from the ?p=123 format.

We could add a new option, like update_option( 'uses_pretty_permalinks', true);, that could be used to change the string when it's warranted and not assume ugly permalinks are or are not the default.

Alternatively, we could write a more ambiguous string to cover both scenarios, but I would prefer two separate descriptions for clarity.

#101 @dd32
9 years ago

Alternatively, we could write a more ambiguous string to cover both scenarios, but I would prefer two separate descriptions for clarity.

I think I'd rather not add an option; think about the case where an install is moved between two servers or the server configuration changes, you can't guarantee that the condition remains the same.

Realistically, the existing text may not even need to change. it still holds true mostly (although I see Pretty Permalinks now as one of the defaults, since it's set during install without prompting the user).

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

#103 @dd32
9 years ago

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

Marking as fixed for 4.2!

Yes, the permalink format isn't going to suit every site out there, so it may still need customizing, but it enhances the default ?p=123
Yes, the text on the permalink page isn't 100% accurate, but covering both "defaults" results in awkward text and the existing text isn't completely wrong - If someone has a better idea for the text though, please feel free to open a new ticket for that.

This ticket was mentioned in Slack in #core-flow by sabreuse. View the logs.


9 years ago

@DrewAPicture
9 years ago

#105 @DrewAPicture
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

We should have a fallback return of false in wp_install_maybe_enable_pretty_permalinks() for the case where permalinks were not enabled. The null return should also return true. Core does not leverage the return value, but the function also is pluggable, so consistency in a return fallback would be good.

See 6481.18.diff.

Please reference #31888 (4.2 docs audit) in the commit message.

#106 @SergeyBiryukov
9 years ago

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

In 32027:

wp_install_maybe_enable_pretty_permalinks() should have a consistent @return value.

props DrewAPicture.
fixes #6481. see #31888.

#107 @chriscct7
8 years ago

#25109 was marked as a duplicate.

Note: See TracTickets for help on using tickets.