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

Make sure self::$backfill is instantiated. #9285

Closed
wants to merge 1 commit into from

Conversation

JoryHogeveen
Copy link
Contributor

@JoryHogeveen JoryHogeveen commented Mar 20, 2018

Summary

This PR can be summarized in the following changelog entry:

Fatal error: Uncaught Error: Call to a member function remove_hooks() on null in /wp-content/plugins/wordpress-seo/inc/options/class-wpseo-options.php:226
Stack trace:
#0 /wp-content/plugins/wordpress-seo/admin/capabilities/class-register-capabilities.php(36): WPSEO_Options::get('access')
#1 /wp-includes/class-wp-hook.php(286): WPSEO_Register_Capabilities->register('')
#2 /wp-includes/class-wp-hook.php(310): WP_Hook->apply_filters('', Array)
#3 /wp-includes/plugin.php(465): WP_Hook->do_action(Array)
#4 /wp-content/plugins/wordpress-seo/admin/capabilities/class-capability-manager-integration.php(48): do_action('wpseo_register_...')
#5 /wp-includes/class-wp-hook.php(286): WPSEO_Ca in /wp-content/plugins/wordpress-seo/inc/options/class-wpseo-options.php on line 226

Relevant technical choices:

Test instructions

This PR can be tested by following these steps:

Quality assurance

  • I have tested this code to the best of my abilities

Fixes #

JoryHogeveen added a commit to JoryHogeveen/view-admin-as that referenced this pull request Mar 20, 2018
@moorscode
Copy link
Contributor

Hi Jory,

It seems that you are running this functionality on a timing that is not very self-explanatory.
Looking through the code of the plugin, I determined that you are triggering the code via:

https://github.com/JoryHogeveen/view-admin-as/blob/9f411160d9c1c138dd4c94ccdfdb2fabe702c1a7/includes/class-vaa.php#L120

As we are registering/initializing/bootstrapping our plugin here:
https://github.com/Yoast/wordpress-seo/blob/trunk/wp-seo-main.php#L386

The WPSEO_Option::get_instance() is being triggered on plugins_loaded priority 14.

The problem would be solved if the filter that triggers https://github.com/JoryHogeveen/view-admin-as/blob/8b94155b042aa4feaaae8b5c84eb3e807fdb867d/includes/class-compat.php#L255 would be ran after the plugins_loaded prio 14 instead of on prio -9999.

@JoryHogeveen
Copy link
Contributor Author

JoryHogeveen commented Mar 21, 2018

Hi @moorscode
View Admin As is a user/role/etc. switching plugin. That's why I hook so early in plugins_loaded, to make sure any plugins that use current_user_can at such early stage will also be compatible with the switching functionality of my plugin.

I already patched a quick fix for this error but it's not very neat and shouldn't be done by my plugin:
JoryHogeveen/view-admin-as@943fb47

I'm simply instantiating WPSEO_Options.

What I'm actually trying to achieve with this is fetching the WPSEO capabilities on the frontend since these are only available on the backend.

However, related to this PR.
I still think it's best to check resources before doing any logic in static methods when they rely on an instantiated class. There is no performance hit when the class is already instantiated this way.

@JoryHogeveen
Copy link
Contributor Author

JoryHogeveen commented Mar 21, 2018

To continue on this, I think its even better to create an API function/method for this.
For example, GravityForms has a simple method to fetch all capabilities used in the plugin: GFCommon::all_caps()

@moorscode
Copy link
Contributor

I agree that the lower-level problem that you are trying to solve should be the focus of changes in this repository. Could you create an issue which describes the need that you have and what API you suggest to use?

@JoryHogeveen
Copy link
Contributor Author

@moorscode
Agreed! Issue created: #9300

@moorscode
Copy link
Contributor

Closing this PR in favor of the new issue.

@moorscode moorscode closed this Mar 25, 2018
@JoryHogeveen JoryHogeveen deleted the patch-1 branch March 27, 2018 21:17
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.

3 participants