WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 2 months ago

#6481 new enhancement

Fancy permalinks should be enabled on new sites

Reported by: Denis-de-Bernardy Owned by:
Milestone: Future Release Priority: normal
Severity: minor Version: 2.7
Component: Permalinks Keywords: needs-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 (4)

upgrade.diff (881 bytes) - added by mrmist 5 years ago.
wp-admin\includes\upgrade.php diff
upgrade_v2.diff (730 bytes) - added by mrmist 5 years ago.
Alternate version of patch with DD32's comments in mind
6481.diff (555 bytes) - added by Denis-de-Bernardy 5 years ago.
6481.2.diff (1.2 KB) - added by Denis-de-Bernardy 5 years ago.

Download all attachments as: .zip

Change History (48)

comment:1 ffemtcj6 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.

comment:2 Denis-de-Bernardy6 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();

comment:3 mrmist6 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.

comment:4 ryan6 years ago

  • Milestone changed from 2.7 to 2.8

comment:5 jacobsantos5 years ago

  • Component changed from General to Permalinks
  • Owner anonymous deleted

comment:6 navjotjsingh5 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.

comment:7 DD325 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.

comment:8 mrmist5 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.

comment:9 Denis-de-Bernardy5 years ago

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

mrmist5 years ago

wp-admin\includes\upgrade.php diff

comment:10 mrmist5 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.

comment:11 DD325 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?

mrmist5 years ago

Alternate version of patch with DD32's comments in mind

comment:12 Denis-de-Bernardy5 years ago

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

comment:13 mrmist5 years ago

  • Keywords tested 2nd-opinion added

comment:14 rowoot5 years ago

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

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

comment:15 follow-up: ryan5 years ago

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

comment:16 ryan5 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.

Denis-de-Bernardy5 years ago

comment:17 Denis-de-Bernardy5 years ago

  • Keywords commit added; 3rd-opinion removed

done

comment:18 in reply to: ↑ 15 Denis-de-Bernardy5 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')

comment:19 follow-up: DD325 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.

comment:20 in reply to: ↑ 19 Denis-de-Bernardy5 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?

comment:21 DD325 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

Denis-de-Bernardy5 years ago

comment:22 Denis-de-Bernardy5 years ago

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

comment:23 Denis-de-Bernardy5 years ago

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

comment:24 follow-up: ryan5 years ago

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

comment:25 Denis-de-Bernardy5 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.

comment:26 Denis-de-Bernardy5 years ago

  • Keywords dev-feedback added; commit removed

comment:27 janeforshort5 years ago

  • Milestone changed from 2.8 to Future Release

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

comment:28 Denis-de-Bernardy5 years ago

  • Milestone changed from Future Release to 2.8

comment:29 hakre5 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.

comment:30 Denis-de-Bernardy5 years ago

  • Milestone changed from 2.8 to Future Release

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

comment:31 Denis-de-Bernardy5 years ago

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

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

comment:32 Denis-de-Bernardy4 years ago

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

comment:33 nacin4 years ago

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

comment:34 mikeschinkel4 years ago

  • Cc mikeschinkel@… added

comment:35 follow-up: GaryJ3 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?

comment:36 mbijon3 years ago

  • Cc mike@… added

comment:37 dd3219 months ago

#22147 was marked as a duplicate.

comment:38 in reply to: ↑ 35 TomAuger19 months 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?

comment:39 Ipstenu19 months 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.

comment:40 TomAuger19 months 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?

comment:41 in reply to: ↑ 24 Denis-de-Bernardy2 months 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();
        }
    }

comment:42 jenmylo2 months 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.

comment:43 follow-up: aaroncampbell2 months 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.

comment:44 in reply to: ↑ 43 jenmylo2 months 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.

Note: See TracTickets for help on using tickets.