Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug where adding superadmins would crash if none existed before #22

Merged
merged 4 commits into from
Feb 21, 2019

Conversation

Nomafin
Copy link
Contributor

@Nomafin Nomafin commented Apr 17, 2018

The add command crashes if there are no super admins present when trying to add a new one thus prevents the admin fixing the situation.

Returning empty array instead of null from the get_admins() function prevents the crash because in_array() on line 107 expects an array.

@@ -194,6 +194,13 @@ public function remove( $args, $_ ) {

private static function get_admins() {
// We don't use get_super_admins() because we don't want to mess with the global
return get_site_option( 'site_admins', array('admin') );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just casting to (array) here should do the exact same thing.

@schlessera
Copy link
Member

Thanks for the pull request, @Nomafin !

I think the above problem can be solved with a simple cast to (array). This would turn a null result into an empty array instead.

@Nomafin
Copy link
Contributor Author

Nomafin commented Apr 17, 2018

Yeah, this is definitely simpler approach. I actually thought this as well, but decided for the if clause for better readability. But I agree that this would do as well.

@schlessera
Copy link
Member

Yes, I can see why the if clause might be more readable.

However, I think that the if clause also implies there is some logic being done here. This is not the case, though, we just fix the type of the return value of the WordPress function, as the WordPress return value is just borked in this case.

@schlessera
Copy link
Member

@Nomafin Would you be up to adding a test for this error case as well, to make sure the WP behavior never changes and we don't introduce a regression later on?

@schlessera schlessera added this to the 1.0.7 milestone Apr 17, 2018
@schlessera
Copy link
Member

@Nomafin I will work on the tests for this myself in the coming days to close this PR.

@javorszky
Copy link

I think I ran into this exact problem, with some more verbose errors:

Warning: count(): Parameter must be an array or an object that implements Countable in phar:///usr/local/bin/wp/vendor/wp-cli/super-admin-command/src/Super_Admin_Command.php on line 104
Warning: in_array() expects parameter 2 to be array, string given in phar:///usr/local/bin/wp/vendor/wp-cli/super-admin-command/src/Super_Admin_Command.php on line 107
Fatal error: Uncaught Error: [] operator not supported for strings in phar:///usr/local/bin/wp/vendor/wp-cli/super-admin-command/src/Super_Admin_Command.php:112
Stack trace:
#0 [internal function]: Super_Admin_Command->add(Array, Array)
#1 phar:///usr/local/bin/wp/php/WP_CLI/Dispatcher/CommandFactory.php(89): call_user_func(Array, Array, Array)
#2 [internal function]: WP_CLI\Dispatcher\CommandFactory::WP_CLI\Dispatcher\{closure}(Array, Array)
#3 phar:///usr/local/bin/wp/php/WP_CLI/Dispatcher/Subcommand.php(425): call_user_func(Object(Closure), Array, Array)
#4 phar:///usr/local/bin/wp/php/WP_CLI/Runner.php(353): WP_CLI\Dispatcher\Subcommand->invoke(Array, Array, Array)
#5 phar:///usr/local/bin/wp/php/WP_CLI/Runner.php(376): WP_CLI\Runner->run_command(Array, Array)
#6 phar:///usr/local/bin/wp/php/WP_CLI/Runner.php(1102): WP_CLI\Runner->_run_command_and_exit()
#7 phar:///usr/local/bin/wp/php/WP_CLI/Bootstrap/LaunchRunner.php(23): WP_CLI\Runner->start()
#8 phar:///usr/local/bin/wp/php/bootstrap.php(75): WP_CLI\Bootstrap\Laun in phar:///usr/local/bin/wp/vendor/wp-cli/super-admin-command/src/Super_Admin_Command.php on line 112

@schlessera schlessera removed this from the 1.0.7 milestone Aug 5, 2018
@schlessera schlessera added this to the 2.0.2 milestone Feb 21, 2019
@schlessera schlessera merged commit 9a56429 into wp-cli:master Feb 21, 2019
schlessera added a commit that referenced this pull request Jan 5, 2022
Fix a bug where adding superadmins would crash if none existed before
Soean pushed a commit to Soean/super-admin-command that referenced this pull request Jan 8, 2024
Backtick code references so they're properly formatted on the website
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants