Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gracefully handle multiple installed copies of the Gutenberg plugin #1020

Closed
wants to merge 3 commits into from

Conversation

nylen
Copy link
Member

@nylen nylen commented Jun 5, 2017

While finishing up #985 I discovered that installing multiple copies of the Gutenberg plugin on a site will cause the following error:

2017-06-05t10 58 58-0400

This is likely to be a problem for some users because we did an early release in Slack with the filename gutenberg-2017-05-24.zip. WordPress will install this plugin zip into a folder named gutenberg-2017-05-24. Then, when a user installs an updated version of the Gutenberg plugin, whether from Slack, the plugin directory once it is released there, or somewhere else, they will see the above error.

We should detect this condition and handle it gracefully (have the duplicate version(s) of the plugin just bail out). This is accomplished in this branch:

2017-06-05t11 06 28-0400

@aduth
Copy link
Member

aduth commented Jun 5, 2017

Should we avoid naming zip files this way to eliminate the underlying cause?

@nylen
Copy link
Member Author

nylen commented Jun 5, 2017

Possibly, though at least a bit of this damage is already done because we released gutenberg-2017-05-24.zip.

This PR may also be overkill for something that will only affect a couple of users, and maybe we just decide that we can address it satisfactorily by moving future releases to the plugin directory.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is another one I could go either way on. The impact does seem very very minimal at most, so added logic may be wasted for such an edge case.

@@ -9,6 +9,11 @@
* @package gutenberg
*/

require_once dirname( __FILE__ ) . '/lib/check-duplicate-plugins.php';
if ( defined( 'GUTENBERG_MULTIPLE_COPIES' ) && GUTENBERG_MULTIPLE_COPIES ) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interested to find some weird repercussions of return used outside functions like this:

http://php.net/manual/en/function.return.php#112515

I suppose it's not a problem here because the functions aren't truly part of the same file. Perhaps fragile all the same though, if ever we were to add any functions to this file, since we'll encounter the same problem meant to be addressed by these changes (duplicate function declaration).

Better solution might be to wrap the require_once with the condition instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our plugin entry point should do as little as possible, only constants and requires and perhaps some very simple checks like in this PR.

@nylen
Copy link
Member Author

nylen commented Jun 6, 2017

I think I'll close this one out. It's unlikely to be a big problem after we release to the plugin directory.

@nylen nylen closed this Jun 6, 2017
@nylen nylen deleted the fix/multiple-plugin-copies branch June 6, 2017 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants