Disable 'Featured Plugins' if they're included in Core

[Ipstenu]

I tossed this ticket out four times before finally posting it. It's so ... iffy.

​http://wordpress.org/support/topic/wrodpress-38-beta-suggestion-to-check-for-activated-feature-plugins

tl;dr: Should we set up 'feature plugins' to auto-disable when they arrive in core? When Dashboard came in, it White-Screen'd my front page (which yes, I reported up stream in the plugin forum), until I deactivated it. But not everyone's going to be smart enough, and we really did invite the less techy with the plugins. Mind you if you ARE testing a plugin you SHOULD know better, but ... this can go both ways, y'know?

I think that, since if you're on the beta releases, there's a high probability of the features in a plugin showing up before you can disable a plugin (like I know to disable MP6 when 3.8 comes out), maybe we should add a check to the upgrader, similar to the delete files call.

If plugin is on this list, disable.

It'd be nice if we could prevent them from even being enabled on their versions.

[markoheijnen]

Why is this a core ticket? Shouldn't the plugins take care of this? I strongly object to have something like this in core.

[dd32]

We do have precedent for this, when we added Sidebars, we deactivated the common Sidebar plugins upon upgrade.

This does re-enforce that even a plugin being developed "for core inclusion" should ALWAYS be developed with prefixes though so it doesn't fatal when/if it's included in core.

[enollo]

I agree that this might be plugin territory and some rules could be enforced for future feature plugins. But, it's a little late for MP6, THX38, and DASH plugins.
Sure, they can be updated with prefixes before the release of 3.8, but there is no guarantee that people will press that Update button.

As a lot of people are already on 3.7 and enjoying the benefits of automatic core updates, I suspect even some techy people will be confused when they are not able to access the admin due to fatal errors.

Because I wasn't sure if this should be in core or not, I never created a ticket and just added it to the suggestion forum to let the core devs decide what to do with it.

Thanks for taking the suggesting into consideration :)

[samuelsidler]

We should definitely ensure new feature plugins – and currently-in-development feature plugins – build in prefixes. But as mentioned above, MP6, THX, and DASH are already committed to core and there's no guarantee users will update their plugins before 3.8 ships.

Fatal errors are not good. We should disable those plugins in core directly.

[Ipstenu]

Replying to markoheijnen:

I strongly object to have something like this in core.

I'm curious as to why?

If we're pushing plugins as how we develop, we have to make a way to graceful remove the featured plugin as it's really no longer needed. Yes, a perfect world has prefixes etc preventing goobering, but mistakes happen. Since these are 'our' plugins, a disable/deactivate of the plugin on a site shows we're taking care of ourselves and our beta testers :)

(Which reminds me, Sams, I think we should have a workflow plan for deactivating/disabling the plugin in the repo once it's in core. A disable will serve updates so we can push emergency fixes for people on pre 3.8 with MP6, for example, but no longer allow people to install it anew. That would help limit the oops! My bad! later on. All that has to happen for that is someone on the core team/plugin team emailing plugins at wordpress.org with a link to the plugin, asking for a disable because it's in core now.)

[markoheijnen]

A plugin is a plugin and nothing more. So MP6 has the same value as any other random plugin. WordPress itself shouldn't know anything about plugins and what they do. This has nothing to do with building new features as plugins. If they shouldn't exists after it is in core then it maybe shouldn't be on WordPress.org in the first place.

I also don't see why we should care much if users don't upgrade their plugin first before core. We also didn't care when we introduced has_shortcode(). Then we also didn't check if that function maybe already existed and this did broke quite a few websites. So we don't live in a perfect and that does mean that sites break when doing updates.

That said deactivation isn't the solution. Users can activate it again. Removing isn't the solution since it's confusion. Even when showing a notice about it since the person seeing it doesn't mean to be responsible for it. Even when someone has a new installation they can still install the plugins if they still in the repo. And removing the plugin can be risky if there are security issues in that part of the code. Then we can't push those changes.

[dd32]

I also don't see why we should care much if users don't upgrade their plugin first before core. We also didn't care when we introduced has_shortcode().

I don't think not doing it in the past is an excuse not to do it now.
We definitely can't stop someone from re-activating it (well, we can, but shouldn't), but we can prevent an unexpected issue upon upgrade - we should be aiming for our upgrades to never break anything.

This would serve more than just fatal error protection though, It'll prevent bug reports from anyone running 3.8+MP6 for example, those people who left it enabled not realising they needed to disable it, "X is broken, it's displaying the wrong image, or has the wrong colour", etc.

Ultimately, this is as simple as deactivate_plugins( array( 'plugin-slug/plugin-slug.php', 'plugin-two.php' ), true, true ); in upgrade_380()

[azaozz]

Tend to agree with @markoheijnen, core shouldn't be dealing with plugin fixes. On the other hand, knowingly letting an upgrade to 3.8 crash and burn is not acceptable either.

All feature plugins should *always* start with something like this:

global $wp_version;

if ( empty( $wp_version ) )
  exit;

if ( version_compare( $wp_version, '3.8', '>=' ) ) // or 3.9, 4.0
  return;

Unfortunately neither of the three merged plugins has that.

A few other things can be done:

1. Update the three plugins and add the above snippet.
2. (Maybe) In the 3.8 "upgrade" message, include a reminder to turn off these plugins before upgrading.
3. As far as I see only DASH causes fatal errors. It seems it used dashboard as prefix but the functions weren't renamed on merging with core (it also used the wp_ prefix which should never happen in a plugin...). Rename the offending functions in core:

   wp_dashboard_activity()           => wp_dash_activity()
   dashboard_show_published_posts()  => wp_dash_published_posts()
   dashboard_comments()              => wp_dash_comments()
   dashboard_relative_date()         => wp_dash_relative_date()
   

[dd32]

FWIW, doing a version compare like that is probably not a great idea overall..
Prefixing their functions with their plugin name is the best (ie. thx38_dashboard_comments()

[azaozz]

Version compare is the easiest (the only?) way to ensure a plugin runs only where it is supposed to.

Of course, all plugins function names and class names have to be prefixed in all cases!

[SergeyBiryukov]

26334.patch​ follows the pattern of maybe_disable_automattic_widgets():
tags/3.7.1/src/wp-admin/includes/upgrade.php#L1960​

Disables MP6, THX38, and DASH plugins. Widgets Area Chooser has the same file name as the Automattic widgets plugin (widgets.php), and therefore is handled by the above function.

[markoheijnen]

I still disagree that this should be in core but if we add something then 26334.patch​ can be dangerous to add. We should check for the plugin filename and not the basename of that.

[matt]

Updating the plugins to return or deactivate on their own prior to release seems like the cleanest approach.

[georgestephanis]

Replying to SergeyBiryukov:

26334.patch​ follows the pattern of maybe_disable_automattic_widgets():
tags/3.7.1/src/wp-admin/includes/upgrade.php#L1960​

Disables MP6, THX38, and DASH plugins. Widgets Area Chooser has the same file name as the Automattic widgets plugin (widgets.php), and therefore is handled by the above function.

If we're testing against active plugins, we should also test against network-activated plugins.

[nacin]

I absolutely think that core should see to it that sites do not break horribly when we merge in a plugin, especially an official one that is being built as — and was accepted as — a feature for core. As dd32 said, there is precedent here.

Replying to matt:

Updating the plugins to return or deactivate on their own prior to release seems like the cleanest approach.

That should absolutely occur. I think I am going to also rename the functions in dashboard.php anyway since they could benefit from it. That allows us to sidestep fatal errors.

We should possibly also deactivate mp6/mp6.php on upgrade. It won't clash like "widgets.php" and, when it is still activated, it causes all sorts of little issues here and there. We can also neuter it in a new version of itself, but this simple change will cut down on support requests.

[nacin]

In 26690:

Dashboard backwards compatibility updates.

• Restore missing wp_dashboard_rss_control() helper.
• Restore all original 3.7 functions, deprecating many, reusing others.
• Rename and remove functions so as not to clash with the original dash plugin.
• Filter cleanup/restoration.

see #25824, #26334.

[nacin]

In 26692:

Deactivate MP6 on update to 3.8.

If left on, it introduces a lot of weird issues not easily diagnosed. MP6 will be updated before release to prevent itself from functioning 3.8+, but this will cut down on support requests by people who update core but not MP6, which was a fairly popular plugin.

fixes #26334.

[nofearinc]

nacin, would the fix work if mp6 is installed in another folder instead of 'plugins/mp6' or it's not considered as a normal practice?