Automattic / vip-support

Manages the VIP Support Users on your site
GNU General Public License v2.0
12 stars 4 forks source link

VIP Support User fails when creating a brand new site #90

Closed mikeyarce closed 3 years ago

mikeyarce commented 3 years ago

Problem: When a new site is created and no users exist, creating a support user fails due to the fact that the plugin relies on admin_init to add the roles.

What happens: The users are added, but without any roles. Doing a wp user list showed no roles assigned to the users.

Should we maybe run this on init on the first version < 2 to make sure our users are created properly? What if we ran it on init only when it's a proxied request?

cc @WPprodigy @mjangda

WPprodigy commented 3 years ago

Could potentially also hook onto some sort of wp-setup hook that is triggered when new sites are created.

mikeyarce commented 3 years ago

@htdat this might be a good bug to look at!

petervibert commented 3 years ago

Noting that I have been fixing this for myself by adding the administrator role to my user, and then shortly after the vip_support role gets added.

htdat commented 3 years ago

Sounds like this is not a new issue https://github.com/Automattic/vip-support/issues/58 and https://github.com/Automattic/vip-support/issues/87

As mentioned in #58, this is an issue for new sub-sites too.

A note here is this PR moved the add_role trigger from init to admin_init https://github.com/Automattic/vip-support/pull/89

htdat commented 3 years ago

There are two solutions like the following:

Hook to init but only if it's a WP-CLI request

Should we maybe run this on init on the first version < 2 to make sure our users are created properly? What if we ran it on init only when it's a proxied request?

I think we can conditionally run on init if it's a request is from WP CLI. I chose WP CLI because AFAIK, this plugin is being used via WP CLI as we interact with it mostly via https://admin.wpvip.com/

Basically, changing this line https://github.com/Automattic/vip-support/blob/1c47e4c9ae71c966c6c0c275e6f010e1f6fcba4d/class-vip-support-role.php#L67

To:

        if ( defined('WP_CLI') && WP_CLI ) {
            add_action( 'init', array( $this, 'action_admin_init' ) );
        } else {
            add_action( 'admin_init', array( $this, 'action_admin_init' ) );
        }

I tested on my local site and I confirm that it's working correctly for both single and multinetwork sites (including adding sub-sites).

For VIP Go sites, I could test this https://admin.wpvip.com/#/sites/358/test-subdir-ms-01-preprod.go-vip.co and confirm wp role list --url=http://test-subdir-ms-01-preprod.go-vip.co/new-site shows up VIP roles after adding a site via both WP CLI or wp-admin UI.

Hook onto an action during wp-setup

Could potentially also hook onto some sort of wp-setup hook that is triggered when new sites are created.

I also looked at this method, and used this code. It works for a new single site or multissite.

        add_action( 'wp_install', array( $this, 'action_admin_init' ) );
        add_action( 'admin_init', array( $this, 'action_admin_init' ) );

However, for adding sub-sites, it's pretty tricky as it's required to handle switch_to_blog like this https://developer.wordpress.org/reference/hooks/wp_insert_site/#comment-4878

That make scode a bit more complicated though doable.

Next

I think the WP_CLI approach is good and simple enough for this issue. Would love to hear your opinion on this issue?

WPprodigy commented 3 years ago

Nice digging! The first option is best imo. An added benefit here is that it can also run during cron jobs, so that guarantees execution within a minute or two as well.

So this is a pattern we often use throughout MU plugins: https://github.com/Automattic/Cron-Control/blob/11991dd41709b2e104c8be03e8a87dd21b49f5d5/includes/class-internal-events.php#L42-L47

IIRC, init isn't consistent in CLI context but wp_loaded is. The pattern is the same as your recommendation, just one hook change for consistency. Nice work.

htdat commented 3 years ago

@WPprodigy - thanks for your feedback. I've created PR #91 for this issue.