This repository has been archived by the owner. It is now read-only.

Remove multisite support from plugin (de)activation hooks #113

Closed
danielbachhuber opened this Issue Mar 11, 2014 · 9 comments

Comments

Projects
None yet
3 participants
@danielbachhuber
Member

danielbachhuber commented Mar 11, 2014

Because switch_to_blog() doesn't load the full code context of a given site, flushing rewrite rules while looping through sites is unadvised — one can accidentally noop existing rewrite rules.

It would be better to have an admin notice or similar if we want to flag the user to the fact they need their rewrites flushed.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Mar 11, 2014

Member

Or, you could just add the two wp-json rules on top of the rewrite_rules array.

Member

danielbachhuber commented Mar 11, 2014

Or, you could just add the two wp-json rules on top of the rewrite_rules array.

@rachelbaker rachelbaker added the Bug label Mar 21, 2014

@rachelbaker rachelbaker modified the milestones: 0.9, 1.0 Mar 21, 2014

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Apr 6, 2014

Member

It would be better to have an admin notice or similar if we want to flag the user to the fact they need their rewrites flushed.

Ergh, I don't like that at all.

Could we instead use an option-per-site, and if not option set, flush rewrite rules? @rachelbaker?

Member

rmccue commented Apr 6, 2014

It would be better to have an admin notice or similar if we want to flag the user to the fact they need their rewrites flushed.

Ergh, I don't like that at all.

Could we instead use an option-per-site, and if not option set, flush rewrite rules? @rachelbaker?

@rachelbaker

This comment has been minimized.

Show comment
Hide comment
@rachelbaker

rachelbaker Apr 13, 2014

Member

@rmccue I think a per-site option to flag if the rewrite rules need to be flushed is a good idea, and gets around the blog switching issue with Multisite. Did you have any thoughts on when (and how often) we should check this option value?

Member

rachelbaker commented Apr 13, 2014

@rmccue I think a per-site option to flag if the rewrite rules need to be flushed is a good idea, and gets around the blog switching issue with Multisite. Did you have any thoughts on when (and how often) we should check this option value?

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Apr 15, 2014

Member

Assigned to @rachelbaker to handle.

Did you have any thoughts on when (and how often) we should check this option value?

Post-init, probably init at priority of 999 :)

Also, I'd recommend we use a version constant here so that if we ever change again, we can bump the version rather than using a new option.

Member

rmccue commented Apr 15, 2014

Assigned to @rachelbaker to handle.

Did you have any thoughts on when (and how often) we should check this option value?

Post-init, probably init at priority of 999 :)

Also, I'd recommend we use a version constant here so that if we ever change again, we can bump the version rather than using a new option.

@rachelbaker

This comment has been minimized.

Show comment
Hide comment
@rachelbaker

rachelbaker Apr 21, 2014

Member

@rmccue Should we also set the per-site option for flushing the rewrite rules on ACTIVATION?

I am still working on testing my current approach locally, and hope to have code pushed here tomorrow.

Member

rachelbaker commented Apr 21, 2014

@rmccue Should we also set the per-site option for flushing the rewrite rules on ACTIVATION?

I am still working on testing my current approach locally, and hope to have code pushed here tomorrow.

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Apr 23, 2014

Member

Should we also set the per-site option for flushing the rewrite rules on ACTIVATION?

We can avoid flushing on activation with this, and simply let it use the same code.

Basically:

function json_maybe_flush_on_update() {
    $version = get_option( 'json_plugin_version', null );
    if ( empty( $version ) || $version !== JSON_PLUGIN_VERSION ) {
        flush_rewrite_rules();
    }
}
Member

rmccue commented Apr 23, 2014

Should we also set the per-site option for flushing the rewrite rules on ACTIVATION?

We can avoid flushing on activation with this, and simply let it use the same code.

Basically:

function json_maybe_flush_on_update() {
    $version = get_option( 'json_plugin_version', null );
    if ( empty( $version ) || $version !== JSON_PLUGIN_VERSION ) {
        flush_rewrite_rules();
    }
}
@rachelbaker

This comment has been minimized.

Show comment
Hide comment
@rachelbaker

rachelbaker Apr 24, 2014

Member

@rmccue That is basically exactly how I have this working. Pull request incoming for this.

Member

rachelbaker commented Apr 24, 2014

@rmccue That is basically exactly how I have this working. Pull request incoming for this.

@rachelbaker

This comment has been minimized.

Show comment
Hide comment
@rachelbaker
Member

rachelbaker commented Apr 24, 2014

@rmccue or @danielbachhuber #reviewmerge

@rachelbaker

This comment has been minimized.

Show comment
Hide comment
@rachelbaker

rachelbaker Apr 26, 2014

Member

@rmccue Feedback applied: 8e5a060 and 71107a4

Member

rachelbaker commented Apr 26, 2014

@rmccue Feedback applied: 8e5a060 and 71107a4

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.