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

Added test if request is a REST API request so that cart is not loaded. #21090

Merged
merged 9 commits into from Feb 19, 2019

Conversation

5 participants
@peterfabian
Copy link
Contributor

commented Aug 17, 2018

Code inspired by WC_REST_Authentication::is_request_to_rest_api, but included also legacy API .

All Submissions:

Changes proposed in this Pull Request:

Closes #20937 .

How to test the changes in this Pull Request:

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Changelog entry

Prevent frontend code from being loaded during REST API.

Added test if request is a REST API request so that cart is not loaded.
Code inspired by WC_REST_Authentication::is_request_to_rest_api, but included also legacy API .

@peterfabian peterfabian referenced this pull request Aug 17, 2018

Closed

Cart gets loaded during REST requests #20937

3 of 3 tasks complete
@codecov

This comment has been minimized.

Copy link

commented Aug 17, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@00a93ae). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #21090   +/-   ##
=========================================
  Coverage          ?   38.53%           
  Complexity        ?    13319           
=========================================
  Files             ?      349           
  Lines             ?    50325           
  Branches          ?        0           
=========================================
  Hits              ?    19390           
  Misses            ?    30935           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
includes/class-woocommerce.php 8.44% <0%> (ø) 72 <4> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00a93ae...d405c28. Read the comment docs.

@claudiulodro

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Code looks good and this seems to work well in my testing. Let's leave this open for now for feedback from @javorszky or others.

Thanks @peterfabian 🏆

@claudiosanches
Copy link
Member

left a comment

Checking for wc-api could generate backwards compatibility.

@claudiosanches

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

This will make plugins like this stop work: https://github.com/seb86/cart-rest-api-for-woocommerce
So we need to promote it, maybe also include a new parameter for woocommerce_rest_is_request_to_rest_api.

@peterfabian

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

Good points, @claudiosanches, thanks for your review. It's true to that REST cart plugin would stop working, but on the other hand, I thought our stance is that REST api doesn't and won't support cart and should be stateless, so it kind of conflicts with each other. The fact that we run suboptimally and load the cart anyway should not be something others should rely on, I think.

@mikejolley mikejolley added this to the 3.6.0 milestone Jan 22, 2019

@mikejolley

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

@peterfabian @claudiosanches My vote is to add a method for detecting REST API requests (the new rest api only) and using that. And when core has a function available (#20937 (comment)) we just swap it out behind the scenes. Make a private method?

@peterfabian

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

Do we want to target only v4 REST API coming up in 3.6 or all new REST API versions?

@claudiosanches

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

@peterfabian we are only leaving behind our legacy REST API, since we should get off that soon (we hope).

Renamed function
As legacy REST API will be removed soon anyway.

@mikejolley mikejolley added this to In Progress in 3.6 Performance++ Feb 14, 2019

@mikejolley
Copy link
Member

left a comment

Added some feedback but I think this is going to work 👍

Show resolved Hide resolved includes/class-woocommerce.php Outdated
Show resolved Hide resolved includes/class-woocommerce.php Outdated

3.6 Performance++ automation moved this from In Progress to Ready Feb 15, 2019

@peterfabian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

Updated according to review notes, please check.

@claudiosanches
Copy link
Member

left a comment

Just a last review to fix potential PHP warning.

peterfabian added some commits Feb 18, 2019

Added back check for empty REQUEST_URI to authentication function, as…
… it's still needed, the value is used further down.
@claudiosanches
Copy link
Member

left a comment

Great work!

@mikejolley mikejolley merged commit 15bcef8 into master Feb 19, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Scrutinizer 3 new issues, 1 updated code elements
Details

3.6 Performance++ automation moved this from Ready to Shipped 🚀 Feb 19, 2019

@mikejolley mikejolley deleted the fix/20937-v0.1 branch Feb 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.