Add hooks for create and delete sites. #7494
Conversation
|
CC @schlessera and @westonruter as this change could be ported back to the amp plugin. I am interested to know the amp plugin handles multisite. |
|
Size Change: 0 B Total Size: 2.12 MB ℹ️ View Unchanged
|
As of AMP 2.1 we actually no longer rely on rewrite rules at all. We didn't have a good multisite solution before. Users would have to manually flush their rewrite rules for each site on the network since we only flushed rules at the activation hook, which would only work in a non-multisite context. In 2.1, we do our own rewrite logic which allows us much more flexibility to add |
| @@ -36,7 +38,7 @@ | |||
| * | |||
| * @package Google\Web_Stories\User | |||
| */ | |||
| class Capabilities implements Activateable, Deactivateable { | |||
| class Capabilities implements Activateable, Deactivateable, Initialize_Site, Delete_Site { | |||
There was a problem hiding this comment.
I don't think this will have any effect, because Capabilities is not a service, and these interfaces are all meant to be used on services.
Since it`s not a service, there's no way the plugin can know it needs to instantiate it.
There was a problem hiding this comment.
Yeah, I was looking into this last night. You are right as the unit tests failed. It is a nice pattern, so I would like to use it.
There was a problem hiding this comment.
You should be able to just turn Capabilities into a Service without much work. Just make sure you check all the places where you're using it to ensure it still works, and keep in mind that it will be shared by default if retrieved through the Services container, so it might make sense to just build it as a shared service right away.
| @@ -35,7 +36,7 @@ | |||
| * | |||
| * @package Google\Web_Stories | |||
| */ | |||
| class Database_Upgrader extends Service_Base implements Activateable { | |||
| class Database_Upgrader extends Service_Base implements Activateable, Initialize_Site { | |||
There was a problem hiding this comment.
Since this runs on admin_init, it won't necessarily be available during activation or initialization (think WP-CLI context).
We could change the registration action to init though and call is_admin() in register().
activate() would then run the migration directly and not call register().
There was a problem hiding this comment.
Again, this is what I get for developing something at 11pm without testing it.
This is less about flushing rewrite rules. To be honest, switch_to_site and then a rewrite rule flush, would like not work anyway. It is more about running activation / decactivation hooks when site is deleted/created. In our case, creating user roles and running database migrations. Surprised to hear a lack of multisite support. I willing to help in this context as maintainer of multisite in core. |
|
We do have multisite support for some parts of the plugin. The lack of multisite support was for the rewrite rules, which are not used anymore. We have code in place for clean up on deactivation across multisites, for example: https://github.com/ampproject/amp-wp/blob/develop/src/BackgroundTask/BackgroundTaskDeactivator.php What we are missing though is actual tests for multisite: ampproject/amp-wp#5701 |
Context
Summary
Tweak, the base plugin template to hook in on new site creation and deletion in multisite context. The pattern of having a method run on new site / delete follows the pattern of activation / deactivation.
Relevant Technical Choices
To-do
User-facing changes
Testing Instructions
QA
This PR can be tested by following these steps:
UAT
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
Type: XYZlabel to the PRFixes #