Opened 9 years ago
Last modified 5 years ago
#36406 reviewing defect (bug)
$network_wide is unreliable
Reported by: | mensmaximus | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | critical | Version: | 3.0 |
Component: | Plugins | Keywords: | has-patch needs-refresh needs-unit-tests |
Focuses: | multisite | Cc: |
Description
This issue may be related to ticket #31104
Scenario:
In a WordPress network an admin decides a plugin must not be network activated. Only a per site or even a site specific activation shall be allowed.
Idea:
Conditional check during plugin activation and die with an error message if $network_wide is true.
Result:
Plugin does not get activated but the custom error message is not displayed
Assume the following function run during activation (it is simplified and the real activation sequence does not have echos!)
function test_activation_hook( $network_wide ) { echo '<span>1 | </span>'; // exit( 1 ); if ( is_multisite() ) { echo '<span>2: | </span>'; // exit( 2 ); if ( $network_wide ) { echo '<span>3: | </span>'; // exit( 3 ); } else { echo '<span>4: | </span>'; // exit( 4 ); } } else { echo '<span>5: | </span>'; // exit( 5 ); } echo '<span>6: | </span>'; exit( 6 ); }
If you exit after if(is_multisite) the message displayed will be '1 | 2 |'
If you exit after if($network_wide) during a networkwide activation the message displayed is '1 | 2 | 4 | 6' instead of '1 | 2| 3|'
Exiting at 4, 5, or 6 will display '1 | 2| 4|', '1 | 2| 5|' and '1 | 2| 6|' as expected.
After many hours of debuging I realized the function (the action filter 'activate_' . $plugin) gets executed 3 times if you exit it early. And only the first call has the argument $network_wide set to true if it is an network_wide activation.
If you like to exit at 3, that is you do not want a network activation, the flow is therefore as follows:
- The first run (click on network activate) has $network_wide set to true and will exit at 3
- the second run (no idea why) has $network_wide set to false and will pass 1 | 2 | 4 | 6
- the third run (no idea why) has $network_wide set to false and will pass 1 | 2 | 4 | 6
Imho calling the activation function (or the code) three times in case you exit it (better say terminate because you cant end a plugin installiton clean) is wrong at all. However if this is by design then $network_wide must be reliably set to true if it originally was.
Attachments (2)
Change History (37)
#2
@
9 years ago
Well now I understand the issue arises from calling plugin_sandbox_scrape() in /wp-admin/plugins.php. Just for a test I set
$_GET['networkwide'] = 1
within plugin_sandbox_scrape() and replaced
if ( $network_wide )
with
if ( $_GET['networkwide'] )
in my activation test (the back compat for plugins). As a result the message I want to show is displayed. However this is not a (good) solution and still I think calling plugin_sandbox_scrape() twice is wrong.
#3
@
9 years ago
And I think there is one more bug in /wp-admin/plugins.php within the 'error_scrape' case from the switch statement. Line 166 shows
do_action( "activate_{$plugin}" );
but I think this should be
do_action( "activate_{$plugin}", is_network_admin() );
like within activate_plugin() in /wp-admin/includes/plugin.php where in shows in line 573
do_action( 'activate_' . $plugin, $network_wide );
This ticket was mentioned in Slack in #core-multisite by mensmaximus. View the logs.
9 years ago
#6
@
9 years ago
After some more investigation I can definitely say passing true
to do_action( "activate_{$plugin}" );
in plugins.php line 166 solves the described problem.
The question now is how can we determine whether the plugin activation was started on the network admin screen? is_network_admin()
is not working in case error_scrape
because WP_NETWORK_ADMIN
is not defined and $GLOBALS['current_screen']->in_admin( 'network' )
is empty. I even tried to pass is_network_admin()
as an argument to $iframe_url
in /wp-admin/plugins.php
line 438.
#7
@
8 years ago
No feedback? Really nobody? Not a single statement for 3 month? Just me talking to myself?
This ticket was mentioned in Slack in #core-multisite by mensmaximus. View the logs.
8 years ago
#9
@
8 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
#10
follow-up:
↓ 11
@
8 years ago
If I'm reading this correctly, you want to add support in core for plugin authors to prevent network activation of their plugins? Right now this could be done manually within the code of the plugin, but you would like to facilitate this by supporting it directly within core?
I could see maybe something like the opposite of the Network: true
header.
So you could have 3 options for plugins on multisite:
- Network activation only
- Individual site activation only
- Either network or individual site activations
Right now 3 is the default and 1 is possible if the Network: true
header is set. I could see declaring Network: false
as serving that purpose.
I could work on a patch for this ticket, but want to make sure I understand your goal.
#11
in reply to:
↑ 10
@
8 years ago
Replying to earnjam:
Right now 3 is the default and 1 is possible if the
Network: true
header is set. I could see declaringNetwork: false
as serving that purpose.
This is what the ticket is about and what the title says. $network_wide is not set consistently and therefore you cant rely on it. Please read my opening post again. You can exit the code after the if ($network_wide)
but you can't send a admin message to display a warning like "this plugin cant be activated networkwide". The problem stems from the error checking as explained in my comments 2, 3 and 6. The $network_wide argument passed to the plugin activation gets lost if the activation process raises an error.
This ticket was mentioned in Slack in #core-multisite by mensmaximus. View the logs.
8 years ago
#13
@
8 years ago
The best solution I can come up with is the attached patch. The only place I could find to check for is_network_admin() is before error_scrape is called. I have added the value to the GET parameters so I can fetch it and pass it to activate_{plugin}. In my tests this works well and has no negative impact.
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
8 years ago
#15
@
8 years ago
- Milestone changed from Future Release to 4.7
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
The patch appears solid to me. What we need to think about is whether the is_network_admin()
check will be reliable enough to know that we're network-activating the plugin. I think it is in this case - but maybe we can find a more accurate solution.
This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#22
@
8 years ago
- Keywords needs-patch added; has-patch needs-testing removed
- Milestone changed from 4.7 to Future Release
- Owner jeremyfelt deleted
- Version changed from 4.4.2 to 3.0
I think I've wrapped my head around what's possible and what's missing here. Thanks for your patience @mensmaximus!
First, I don't think there's a way to short-circuit network activation of a plugin and show a custom or configurable error message in our current state. When the plugin's activation hook fires, WordPress expects to get null
back. If anything else is output, then a predetermined path to an error condition occurs.
- If
echo 'error';
is used, an error explaining that unexpected characters were output will appear. The plugin will still be activated with just a warning to be cautious. - If
exit()
is used, a fatal error will occur during activation and a different error will be showing.
It's when this fatal error is show that the plugin goes through additional validations and the plugin activation hook is run again with varying $network_wide
values. If the plugin is network activated with no error, the activation hook fires once.
It's possible to work around the activation by using the activated_plugin
hook to reverse activation immediately after. Something like this:
function remove_this_plugin( $plugin ) { $current = get_site_option( 'active_sitewide_plugins', array() ); unset( $current[ $plugin ] ); update_site_option( 'active_sitewide_plugins', $current ); } register_activation_hook( __FILE__, 'test_activation_hook' ); function test_activation_hook( $network_wide ) { if ( $network_wide ) { add_action( 'activated_plugin', 'remove_this_plugin' ); } }
This works, but provides no feedback to the user that the plugin activation was not successful.
It seems like there should be room for a filter that allows a plugin to set the WP_Error
message in a way that a proper error will be displayed. I'm not yet sure if I'm missing anything in thinking that way.
I'm going to push this back to Future Release. We should continue discussing possible approaches.
#23
follow-up:
↓ 24
@
8 years ago
Thank you for the feedback. However I don't understand the final statement to push it back again. My patch is working as expected. If I exit the activation while network_wide was set the activation is stopped and my custom error message ist shown.
#24
in reply to:
↑ 23
@
8 years ago
Replying to mensmaximus:
Thank you for the feedback. However I don't understand the final statement to push it back again. My patch is working as expected. If I exit the activation while network_wide was set the activation is stopped and my custom error message ist shown.
I'm not yet sure that working around the generation of a fatal error is the proper way to handle this. It may be, but pushing it back provides some time to think about other possible approaches.
#25
@
8 years ago
- Keywords has-patch added; needs-patch removed
- Severity changed from normal to major
Now as 4.7 is launched I would really like to see my patch or any other reliable solution to make it into 4.7.1.
Still I do not understand what you are saying @jeremyfelt. My solution does not produce any fatal errors. My patch just fixes the missing $network_wide parameter. This is neither an enhancement nor a new feature. This is a bug fix.
I really would like to avoid patching 72 wordpress multisite installations after each update for a proper error handling :-).
#26
@
8 years ago
- Keywords needs-refresh needs-unit-tests added
- Severity changed from major to normal
Now as 4.7 is launched I would really like to see my patch or any other reliable solution to make it into 4.7.1.
4.7.1 is specifically for regressions introduced in 4.7. Tickets like this one would get into a major release. No agreement could be reached for 4.7, that's why it was pushed back.
Still I do not understand what you are saying @jeremyfelt. My solution does not produce any fatal errors. My patch just fixes the missing $network_wide parameter. This is neither an enhancement nor a new feature. This is a bug fix.
Nobody said your solution would produce any fatal errors, but that the patch works around the handling of fatal errors. Quote:
I'm not yet sure that working around the generation of a fatal error is the proper way to handle this.
A quick note on the patch:
- Patches should ideally be made from the root directory of the develop repository, so that the path reads like
src/wp-admin/plugins.php
, not.mensmaximus.com/htdocs/wp-admin/plugins.php
orplugins.
like in the latest patch. Otherwise the patch cannot be applied easily. $_REQUEST[ 'network_wide' ]
is not being sanitized. Since you only want a boolean value, that line could be simply changed todo_action( "activate_{$plugin}", isset( $_REQUEST['network_wide'] ) );
Further:
- The
activate_plugin
hook really needs to be fixed. At one point the$network_wide
argument passed and at another point it is not — without any documentation about it. - Sharing the same concern as Felix:
What we need to think about is whether the
is_network_admin()
check will be reliable enough to know that we're network-activating the plugin. I think it is in this case - but maybe we can find a more accurate solution.
Adding needs-unit-tests
to see if we can add unit tests for this. Not sure how testable the code is though.
#27
@
8 years ago
@swissspidy thank you for the explanation. I understand I have to wait until the core development team takes care of it.
#28
follow-up:
↓ 29
@
8 years ago
- Severity changed from normal to critical
It looks like that there is no solution currently at all, because I cannot put do_action from no reason at some random point of code, and I must use:
<?php register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'activate'));
and in the:
public function activate() { // there is NO way to check is that is single site, or network activation }
So the only hack might be to put:
<?php if(is_network_admin()) { register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'networkActivate')); } else is_admin()) { register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'activate')); }
And this is critical, because this do not allow normal activation of network plugin at all, without leaving ability for plugin then being activated for single site only in multisite mode.
#29
in reply to:
↑ 28
;
follow-up:
↓ 30
@
8 years ago
Replying to KestutisIT:
It looks like that there is no solution currently at all, because I cannot put do_action from no reason at some random point of code, and I must use:
<?php register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'activate'));and in the:
public function activate() { // there is NO way to check is that is single site, or network activation }So the only hack might be to put:
<?php if(is_network_admin()) { register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'networkActivate')); } else is_admin()) { register_activation_hook($this->coreConf->getPluginPathWithFilename(), array(&$this, 'activate')); }And this is critical, because this do not allow normal activation of network plugin at all, without leaving ability for plugin then being activated for single site only in multisite mode.
And even my hack with is_network_admin() doesn't work here, because if I do debug test for network-install click, it says that is a regular admin page, not a network. So that works only after plugin install, when I browse plugin pages, but not during plugin activation. So there is absolutely NO SOLUTION AT THIS MOMENT.
#30
in reply to:
↑ 29
@
8 years ago
Replying to KestutisIT:
and in the:
public function activate() { // there is NO way to check is that is single site, or network activation }
$network_wide
is available in activate()
here and can be used to detect if this is a network activation.
The issue described by this ticket is that there's no clean way to avoid network activation when $network_wide
is seen as true
.
#32
@
5 years ago
- Severity changed from critical to blocker
3 years and no progress made. I believe the severity of this ticket has to be set to "blocker". As there is NO WAY further develop plugins, as multisite is now one of the main parts of the WP Core.
Also nobody published any workaround.
I described the situation more clear in my ticket:
https://core.trac.wordpress.org/ticket/47646
#33
@
5 years ago
Just an additional suggestion to this is that we should have two hooks in the future, instead of playing with IF/ELSE statements in the same hook, meaning:
<?php register_activation_hook(..);
which would be called only on local activations, doesn't matter if the site is based on multisite or not,
and
<?php register_network_activation_hook(..);
which would be called only after call from 'network plugins admin' with 'network activate click'.
That would make WordPress much more user friendly and developers would be able to write more cleaner and do make less bugs in the code.
#34
@
5 years ago
- Severity changed from blocker to normal
blocker
should be reserved for issues that make WordPress completely unusable and completely prevent a release from occurring. This does not meet that criteria.
#35
@
5 years ago
- Severity changed from normal to critical
It definitelly not 'normal'. There is NO WAY to develop plugins at all in WordPress at this point for WordPress network. I'd say it is 'blocker'. But if @earnjam claims that it is regarding WordPress core only, not the WP plugins, then it should remain as 'critical'. In any case - 3 years way too long for this, meaning that no network plugins can be developed.
To visualize this issue better I have captured some information at the beginning of the function (where exit(1) is located):
Captured during a successful network activation
Captured while exiting at exit(3) to avoid network activation of the plugin