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

If someone tries to load WC_REST class to early, throw notice #23102

Merged
merged 5 commits into from
Mar 27, 2019

Conversation

mikejolley
Copy link
Member

As of 3.6, rest API classes only get included during the rest_api_init hook to prevent performance issues on non-rest api pages.

Some extensions extend these classes early/outside of this hook.

To prevent this causing fatal errors, this PR adds some code to the autoloader to include those files if missing, outputting a notice to inform the developer whats going on:

image

You can test by installing the printful intgration from WordPress.org which suffers from this. You should see the above notice with DEBUG mode on, but there will be no fatal error.

cc @timmyc

@mikejolley mikejolley added this to the 3.6.0 milestone Mar 21, 2019
Copy link
Contributor

@timmyc timmyc left a comment

Choose a reason for hiding this comment

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

This indeed fixes the fatal I was seeing in Printful, but I think we might need to add one more conditional to the logic to ensure the current request is not a REST request. As-is right now on this branch if I load up /wp-json the error is still thrown which invalidates the JSON response from the API

image

@mikejolley
Copy link
Member Author

@timmyc good catch. The function which outputs the notice should be patched to do this, it already handles ajax requests for the very same reason.

@codecov
Copy link

codecov bot commented Mar 21, 2019

Codecov Report

Merging #23102 into master will increase coverage by <.01%.
The diff coverage is 16.67%.

Impacted Files Coverage Δ Complexity Δ
includes/class-woocommerce.php 9.03% <0%> (+0.93%) 74 <0> (ø) ⬇️
includes/class-wc-autoloader.php 75% <33.33%> (-3.38%) 20 <0> (+2)
includes/blocks/class-wc-block-library.php 0% <0%> (ø) 17% <0%> (+2%) ⬆️

@timmyc
Copy link
Contributor

timmyc commented Mar 25, 2019

@mikejolley testing this out again locally, and still seeing:

image

when going to /wp-json

It seems doing other REST requests are working okay - at least clicking around in Gutenberg I can perform RESTful actions like saving a draft.

Copy link
Contributor

@timmyc timmyc left a comment

Choose a reason for hiding this comment

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

Thanks @mikejolley - the last commit fixed up all things for me. Verified with printful 2.0.8 installed the warning is shown, no exception is thrown, and REST API requests do not see the warning.

@timmyc timmyc merged commit 8b24528 into master Mar 27, 2019
@claudiosanches claudiosanches deleted the update/prevent-fatal-errors-when-extending-rest-api branch May 3, 2019 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants