Widgets: Prepend widgets with Jetpack_ to minimize conflicts

[ebinnion]

Fixes #6145.

In 3016571-t, a user reported a conflict with this plugin.

To fix this issue, and avoid conflicts in the future, this PR seeks to rename widgets which do not have Jetpack_ or WPCOM_ at the beginning of their class name.

To test:

• Checkout branch
• Go to Customize
• Add an Upcoming Events widget
• Add a Google Translate widget
• There should be no errors

Note: The follow button widget is not currently functional due to performance concerns.

[gravityrail]

👍

[georgestephanis]

While changing the class names, we're not modifying the $id_base passed in the parent::__construct() call as the first parameter. Is there any danger of collisions there with the other plugins that had been using the same class name?

[ebinnion]

Thanks for the review @georgestephanis! I pushed another commit that updates the upcoming events widget ID that we pass to the parent constructor as well.

I found at least one conflict with the upcoming_events_widget ID and did not find one for google_translate_widget, although I didn't look exhaustively.

The issue with updating the ID is that the widgets will no longer appear in the sidebar after the user updates, is that right? Do you know of a migration process where we could migrate the old widgets to the new ID?

It looks like the Google Translate Widget went in with 4.4 and the Upcoming Events Widget went in with 4.5. Because of this, I didn't update the Google Translate Widget ID.

[georgestephanis]

I don't know of a translation way offhand. Unsure if it's better for ux to just not change it if we don't have a good migration.

[ebinnion]

cc @rickybanister - We need to update the widget name for one of the widgets that went out in 4.5 because there are known conflicts with other widgets.

There's not currently a known migration path. We could likely find one, but without lots of testing, it may be brittle.

So, the question I put to you is, should we update the widget ID name in order to prevent conflicts? The worst case here is that users widget might disappear on upgrade.

Or do we disregard and hope users don't use a conflicting widget from another plugin?

[beaulebens]

So, just to clarify, our options at the moment are:

1. Rename the id of the widgets, and any users who had previously configured those widgets will "lose" their widget (and have to re-add/configure it)
2. Leave the name and risk conflicts with other plugins (what happens when there's a conflict? Confusing data? Fatal error?)

Is that correct? No other options that we know of?

[ebinnion]

After some quick searching the other day, there are no other options that I know of.

That being said, I would guess there is some way that we can migrate widgets with the old id to the new id. The edge case there would be figuring out how to handle widgets where the widget id clashes with another plugin.

If we don't handle this case properly, and we migrate, we could potentially cause fatals instead of widgets just dropping off.

So, I guess what I'm trying to say is I'm sure there's probably some path there to migrate these widgets. But, I worry that we'd be causing more issues by trying to migrate.

[georgestephanis]

I wonder if we could do some late check of if some other widget is using that ID, and if not alias it back? I'm unsure about the feasibility.

[ebinnion]

After thinking more about it, I have removed the second commit that changes the ID name. This means that we are updating the class names but not handling cases where widgets have the same ID name.

This gets us to a point where I think we can merge this PR in for beta tomorrow, which we should push for since there is at least one case where a user's site is broken upon upgrade to 4.5.

Then, we can decide how to handle potentially migrating the IDs.

[beaulebens]

Fair enough. I agree that at least prefixing the classes makes sense, and is just general good practice.

@georgestephanis's suggestion above is an interesting one -- if we can maybe be really defensive in adding these widgets, and hook them in really late, check if there's an id conflict, and if so then we could just not load that specific widget, and maybe throw an admin notice up to let the user know?

[eliorivero]

LGTM! 🐑
This PR does what it intends to do and the id issue and defensive loading can be handled in a new one.

[lezama]

👍

[dereksmart]

Rename the id of the widgets, and any users who had previously configured those widgets will "lose" their widget (and have to re-add/configure it)

This is the reason I left the class names the same when imported from wpcom, as I didn't have a solution to migrate wpcom widgets at the time. If we merge this we'll have to make sure to account for wpcom widgets as well.

[dereksmart]

I missed this bit

I have removed the second commit that changes the ID name

Let's go ahead with it for now