Skip to content
This repository has been archived by the owner on Oct 4, 2019. It is now read-only.

Fix regex used in currency API endpoint #79

Merged
merged 3 commits into from
Jan 10, 2018
Merged

Fix regex used in currency API endpoint #79

merged 3 commits into from
Jan 10, 2018

Conversation

ryelle
Copy link
Member

@ryelle ryelle commented Jan 3, 2018

Fixes #77 – the regex used in the API endpoint was a malformed pattern, at least for how gutenberg uses these. This caused gutenberg to crash. This PR fixes the endpoint regex by setting the currency code to exactly 3 letters.

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.

Change looks good - wondering if g-berg needs to have some logic to prevent other plugins from crashing it in this manner.

Does look like there is one failing test related to this change on the branch ( there are a couple other failures, but guessing those are on master too ):

There was 1 failure:

1) Data_API::test_register_routes
Failed asserting that an array has the key '/wc/v3/data/currencies/(?P<currency>[\w-]{3}+)'.

/srv/www/wordpress-develop/public_html/src/wp-content/plugins/wc-api-dev/tests/unit-tests/data.php:32

@ryelle
Copy link
Member Author

ryelle commented Jan 4, 2018

I tried to run the tests to fix them, and ran into this error:

Warning: require_once(/vagrant/content/plugins/woocommerce/tests/framework/factories/class-wc-unit-test-factory-for-webhook.php): failed to open stream: No such file or directory in /vagrant/content/plugins/wc-api-dev/tests/bootstrap.php on line 107

It looks like the webhook tests were removed, see woocommerce/woocommerce@0ac915d – so I've removed those lines from our tests too.

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 for picking up the webhook test trash too! I think i had an old version of WC on my vvv instance when I ran the tests yesterday.

@timmyc timmyc merged commit 96346f0 into master Jan 10, 2018
@timmyc timmyc deleted the fix/api-schema branch January 10, 2018 18:30
@timmyc timmyc mentioned this pull request Jan 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gutenberg conflict
2 participants