-
Notifications
You must be signed in to change notification settings - Fork 29
[WOOTAX-299] Fix - Don't register the continents fallback on unrelated REST requests #2991
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
Open
bartech
wants to merge
6
commits into
trunk
Choose a base branch
from
fix/wootax-299
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
dd46f56
[WOOTAX-299] Fix - Don't register the continents fallback on unrelate…
bartech bcdb906
[WOOTAX-299] Test - Assert the continents contract on the route table…
bartech b18691e
Merge remote-tracking branch 'origin/trunk' into fix/wootax-299
bartech c231949
[WOOTAX-299] Tweak - Move the changelog entry to the 3.6.13 block
bartech 8edb939
Merge branch 'trunk' into fix/wootax-299
Abdalsalaam 8a1691a
[WOOTAX-299] Test - Exercise the continents fallback branch, not just…
bartech File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woocommerce-services.php:1321 adds
require_once __DIR__ . '/classes/class-wc-connect-continents.php';to the fallback branch, presented in the PR body as the fix for the observedClass "WC_Connect_Continents" not founderror. But test_bundled_continents_controller_is_constructible requires that same file itself before constructing the controller, and test_wc_api_dev_init_defers_to_core_continents_controller returns at the new class_exists() guard and never enters the fallback branch. Deleting line 1321 from production leaves the entire suite green, so the test proves the controller is constructible GIVEN its dependency rather than that the fallback loads it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that nothing pinned that require. I checked it the way you did and got the same result: delete line 1321 and the whole suite stays green, because neither test ever enters the fallback branch. Fixed in 8a1691a.
I did not apply the suggested diff, though, because I don't think it gets there. I tried it:
composer test -- --filter test_bundled_continents_controller_is_constructiblefails withClass "WC_Connect_Continents" not foundcomposer testpassesIt passes in the suite only because
tests/php/classes/test-class-wc-rest-connect-shipping-label-controller.phpsorts earlier and loads that class for its own setup. So the test would be relying on process-global class-load state again — the thing your first comment (correctly) got rid of — and it still never enters the fallback branch, so line 1321 would stay unpinned either way.What I did instead was make the branch reachable.
wc_api_dev_init()'s "does core supply the controller?" question now lives in a small protected method, so a test can answer it "no" on a WooCommerce that does supply it. Everything else in the method runs for real: the threerequire_oncecalls, the controller construction, andregister_routes(). The new test asserts both bundled routes show up on a route table where core'swc/v3namespace is absent, so they can only have come from our fallback.Mutation results, all with
--filter test_wc_api_dev_init:$continents->register_routes();wc-api-devrequireWC_Connect_ContinentsrequireOne honest limit: that third row only holds when the file runs alone. In a whole-suite run an earlier file has already loaded
WC_Connect_Continents, so the missing require is invisible. Loading a file is process-global and monotonic, so the only way around it is a fresh process per test, and that is not affordable here — I measured PHPUnit's process-isolation annotation at ~26s for a single test (the child re-runs the WP/WC bootstrap), and that bootstrap reinstalls the shared test database underneath the tests that run after it. I wrote that limit into the test's docblock rather than leave it implied.Suite is 404 tests / 985 assertions green, and PHPCS is unchanged on both files (206 violations / 29 sources before and after, same path).