Issue #231: Added notifications when plugins are force activated #569
Conversation
| @@ -371,6 +380,9 @@ public function init() { | |||
| 'The following recommended plugins are currently inactive: %1$s.', | |||
| 'tgmpa' | |||
| ), | |||
| 'notice_force_activation' => _n_noop( | |||
| 'The following plugin has been automatically activated because it is required by the current theme: %s', 'The following plugins have been automatically activated because they are required by the current theme: %s' | |||
There was a problem hiding this comment.
This string will also need to be added to the example.php file for potential overruling.
|
Hi @creativecoder Thanks ever so much for updating the PR! I've added some inline notes about the code itself. One thing I'm wondering about is whether the 'generic' TGMPA notice is the best place to advise a user about a force activated plugin. Or should a force activated plugin have it's own admin notice ? IMHO I think splitting them off would be the more user friendly. The generic Also, I can imagine that the notice should only be shown to admin users (who may have tried to deactivate the plugin) and not to the wider range of users who get to see the call to action notice. What are your thoughts on this ? @GaryJones want to weight in as well ? |
|
I'm in agreement. It's a great feature, but it should be a separate notice, shown to just the relevant group of users. |
| @@ -2023,6 +2039,7 @@ public function force_activation() { | |||
| } elseif ( $this->can_plugin_activate( $slug ) ) { | |||
| // There we go, activate the plugin. | |||
| activate_plugin( $plugin['file_path'] ); | |||
| $this->force_activated_plugins[] = $slug; | |||
There was a problem hiding this comment.
I'm thinking a transient should be set with this information. The situation - a plugin being force (re-) activated - could come up during a bulk-install which uses frames which would prevent the message from becoming visible. Similarly, another program could with code try to silently deactivate a plugin.
So, what about setting a transient with the slugs of force-activated plugins whenever this occurs ? Then having an admin_notice action which checks if such a transient exists, that we're in the WP back-end (or maybe even check that we're on the WP admin plugins page) and that the current user can activate activate plugins and if so, shows a one-time notice to that user about the plugin(s) which were force activated ?
There was a problem hiding this comment.
Thanks for the thoughtful feedback. I like all of these ideas, both from
a UX and engineering perspective. I'm aiming to get to these changes and
additions in the next week or two and will update the pull request.
|
@creativecoder Just a quick question - we're about ready to release TGMPA v2.6.0 and I'm wondering whether we should hold off another week or so to include this PR or not. What do you think ? |
|
I can work on updating the pull request this weekend with specified
changes, if you want to hold back the release until next week, so it can
be included.
|
|
@creativecoder OK, will hold off until after the weekend. |
creativecoder
force-pushed
the
feature/231-notify-on-forced-activation
branch
3 times, most recently
from
364a5a2 to
16241d1
Compare
|
@jrfnl @GaryJones Sorry for the delay! I've update my pull request, as suggested. Please let me know if you see any issues that need to be addressed before merging. |
| @@ -371,6 +371,9 @@ public function init() { | |||
| 'The following recommended plugins are currently inactive: %1$s.', | |||
| 'tgmpa' | |||
| ), | |||
| 'notice_force_activation' => _n_noop( | |||
| 'The following plugin has been automatically activated because it is required by the current theme: %s', 'The following plugins have been automatically activated because they are required by the current theme: %s' | |||
| ), | |||
There was a problem hiding this comment.
_n_noop() needs the text-domain as the last argument.
|
Hi @creativecoder I'd like to take the time to properly look at what you've done now and as we've already started the process now of releasing v 2.6.0, I'm inclined to not rush this through and leave it for v2.6.1 if you don't mind. |
| @@ -448,6 +451,11 @@ public function init() { | |||
| if ( true === $this->has_forced_deactivation ) { | |||
| add_action( 'switch_theme', array( $this, 'force_deactivation' ) ); | |||
| } | |||
|
|
|||
| // Display forced activation notice, if present. | |||
| if ( current_user_can( 'manage_options' ) && is_admin() ) { | |||
There was a problem hiding this comment.
No need for the action if there are no plugins with the force activated flag set to true
if ( true === $this->has_forced_activation && ...
Edit: or is there ? the force activated flag might have been removed, but there may still be a lingering transient which hasn't been displayed .. ?
There was a problem hiding this comment.
Right... the property has_forced_activation indicates if there are plugins that need to be force activated, where the the transient and message are displaying only if they have already been force activated, but we still need to notify the user.
|
@creativecoder Very much like what you've done now! Added some thoughts to the code, but they are mainly about the details, not the principle of it. This is shaping up to be a great PR! |
|
@GaryJones Quick question - looks like Scrutinizer is not running for PRs from external forks... any ideas ? |
| * | ||
| * @since 2.6.0 | ||
| * | ||
| * @param array $message Associative array of actions: each key is an action and each value is an array of plugin slugs that need that action. |
There was a problem hiding this comment.
Sorry to be a pain, but the parameter comments should line up, so the text Associative.. needs some more spaces before it.
There was a problem hiding this comment.
Also, only a single space is needed after @param.
Yes, that means it won't line-up with the type in the @return string, but that's what WP does.
No idea. Didn't you recently update the config for that? |
|
The |
| protected function build_message( $message, $line_template = '%s' ) { | ||
| $message_output = ''; | ||
|
|
||
| krsort( $message ); // Sort messages. |
I did, but that shouldn't have any effect on when it's run, just what checks it runs. |
creativecoder
force-pushed
the
feature/231-notify-on-forced-activation
branch
5 times, most recently
from
450a017 to
6b84370
Compare
creativecoder
force-pushed
the
feature/231-notify-on-forced-activation
branch
from
6b84370 to
83cf630
Compare
|
Thanks for the feedback. I've responded and made several updates. Let me know if you see anything outstanding. |
I've added this with the goal of providing feedback to users that indicates why plugins are being automatically reactivated.
Updated from #232