Make WordPress Core

Opened 14 years ago

Closed 9 years ago

#14849 closed enhancement (fixed)

Rewrite rules should be flushed when you switch themes

Reported by: jorbin's profile jorbin Owned by: jorbin's profile jorbin
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.0
Component: Permalinks Keywords: needs-patch
Focuses: Cc:

Description

@nacin said so at WCMA, so here's a patch to do it.

Attachments (1)

theme.php.15608.diff (380 bytes) - added by jorbin 14 years ago.

Download all attachments as: .zip

Change History (22)

#1 @greenshady
14 years ago

Why should rewrite rules be flushed when switching themes?

#2 follow-up: @filosofo
14 years ago

I suppose in case a theme is using custom post types. But it seems that, like plugins, themes that need flushed rules should handle it themselves.

#3 in reply to: ↑ 2 @nacin
14 years ago

Replying to filosofo:

But it seems that, like plugins, themes that need flushed rules should handle it themselves.

Unfortunately, there's no activation hook for them to do so. (For example P2 uses an option to store whether rules have been flushed.) I suggested this in response to a question for how to handle things that add rewrite rules...

I don't think this patch will work, though, because the new theme isn't included on that pageload so the old rules will still be regenerated. Sounds to me that to do an activation hook for themes, we need to add an autoloaded option to keep track.

#4 @greenshady
14 years ago

Adding an activation hook sounds like the best solution for this. We have switch_theme, which works on deactivation. Themes that add custom post types and taxonomies should be flushing the rewrite rules themselves.

#5 @hakre
13 years ago

+1 for theme activation hook.

#6 @dd32
13 years ago

  • Keywords needs-patch added; has-patch removed

resetting keywords since the attached patch will not work as intended.

#7 @jane
13 years ago

  • Version set to 3.0

If anyone would like to get this in for 3.1, there are a few days left to submit patches for enhancements before freeze.

#8 @johnpbloch
13 years ago

  • Cc johnpbloch added

#9 @nacin
13 years ago

  • Milestone changed from 3.1 to Future Release

This is done in 3.1 on wp-admin/themes.php. We should do this better, in conjunction with a real theme activation hook, in the future.

Last edited 13 years ago by nacin (previous) (diff)

#10 @nacin
13 years ago

FYI, the jank was removed in 3.1. [17241]. Need to do this properly in 3.2.

#11 @WraithKenny
13 years ago

  • Cc Ken@… added

#12 @scribu
12 years ago

Related: #7795

#13 @chriscct7
9 years ago

Can't themes who want to do this simply hook into after_switch_theme and do this?

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


9 years ago

#15 @jorbin
9 years ago

  • Milestone changed from Future Release to 3.3
  • Resolution set to fixed
  • Status changed from new to closed

r18655 in 3.3 introduced after_switch_theme which can be used for this. Core shouldn't encourage themes to include there own custom rewrite rules.

Also, bad on me for such a bad description.

#16 @jorbin
9 years ago

  • Milestone changed from 3.3 to 4.4

I didn't consider the use case of needing to get rid of old rewrites from the last theme. Dion pointed it out. Let's fix this in 4.4

#17 @chriscct7
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#18 @chriscct7
9 years ago

  • Owner set to jorbin
  • Status changed from reopened to assigned

#19 @wonderboymusic
9 years ago

  • Keywords needs-unit-tests added

#20 @jorbin
9 years ago

  • Keywords needs-unit-tests removed

I can't figure out a good way to unit test this since:

1) None of the default themes that we use for testing have custom rewrite rules.
2) flush_rewrite_rules relies upon saving the in memory rules. As the tests all run in a single memory session, there isn't a good way to fake this that would produce reliable test results.

This is the test code I was playing around with before coming to this conclusion.

class Tests_Flush_On_Theme_Change extends WP_UnitTestCase {
    private $original_rewrite_rules;
    private $current_theme;

    function setUp() {
        global $wp_rewrite;
        parent::setUp();

        // Need rewrite rules in place to use url_to_postid
        $wp_rewrite->init();
        $wp_rewrite->set_permalink_structure( '/%year%/%monthnum%/%day%/%postname%/' );

        create_initial_taxonomies();

        $wp_rewrite->flush_rules();

        $this->original_rewrite_rules = get_option( 'rewrite_rules' );

        $this->current_theme = wp_get_theme();

    }

    function tearDown() {
        global $wp_rewrite;
        $wp_rewrite->init();

        $theme = $this->current_theme;

        switch_theme( $theme->Stylesheet );
        parent::tearDown();
    }

    /**
     * @ticket 14849
     */
    function test_switch_theme_rewrite_flush() {
        $themes = wp_get_themes();

        flush_rewrite_rules();

        add_rewrite_rule('^test/([0-9]+)/?', 'index.php?page_id=$matches[1]', 'top');

        flush_rewrite_rules();

        $updated_rewrite_rules =  get_option( 'rewrite_rules' );
        $this->assertNotEquals( $this->original_rewrite_rules , $updated_rewrite_rules );

        foreach ( $themes as $theme ) {
            switch_theme( $theme->Template, $theme->Stylesheet );
            $updated_rewrite_rules =  get_option( 'rewrite_rules' );
            $this->assertEquals( $this->original_rewrite_rules , $updated_rewrite_rules );
        }

    }


}

#21 @jorbin
9 years ago

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

In 34028:

Flush rewrite rules upon theme switch

Themes can ( and do ) but shouldn't include custom rewrite rules.This can lead to hard to debug issues for theme authors. Theme changes are not a ultra common conclusion. Flushing the rewrite rules on theme switch will lead to a clean slate for each theme which helps make debugging easier.

And @nacin said we should do this 5 years ago at WordCamp Mid Atlantic.

Fixes #14849

Note: See TracTickets for help on using tickets.