-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add user locale api-fetch middleware #10862
Conversation
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.
One small q from me. I'll defer to @WordPress/gutenberg-core for implementing this as middleware vs. other.
if ( | ||
typeof options.url === 'string' && | ||
-1 === options.url.indexOf( '?_locale=' ) && | ||
-1 === options.url.indexOf( '&_locale=' ) |
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.
It'd be nice if @wordpress/url
had a hasQueryArg
helper... Not critical for this PR though.
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.
@danielbachhuber Ha :-) I thought the same and created #10879 because of this.
3eeb076
to
4e16220
Compare
If anyone has an idea about the failing tests, please let me know. Otherwise I'll have a look tomorrow. |
packages/api-fetch/CHANGELOG.md
Outdated
@@ -1,6 +1,7 @@ | |||
## 2.1.0 (Unreleased) | |||
|
|||
- Support `per_page=-1` paginated requests. | |||
- Added new middleware to append `?_locale=user` to API requests ([#10862](https://github.com/WordPress/gutenberg/pull/10862)). |
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.
It's not documented anywhere the name of said middleware or how to use it.
Since we already have a section in the README, seems prudent to include there:
@aduth I didn't export the middleware though. It's "internal" like e.g. I doubt that there's much value in exporting that middleware in the same fashion as Happy to be proven wrong though. Otherwise I can update the changelog entry to reflect that it's more of an internal change (e.g. |
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.
Fair enough, I wasn't familiar with "internal" middlewares.
The CHANGELOG is wrongly formatted to begin with (not your doing). We should include sections describing the types of changes ("New Features", "Bug Fixes, "Internal").
https://github.com/WordPress/gutenberg/blob/master/CONTRIBUTING.md#maintaining-changelogs
Since 2.1.0 of this package was released in the meantime I updated the changelog now and added the correct |
Since #10885 has been merged now I updated the code to use |
Build seems to be failing so deleted build cache for this PR and restarted it. |
Hey peeps, I've had to mark the demo and embedding tests as skipped, because something in the travis environment is stopping us intercepting requests and mocking embed responses. I'll start taking a closer look at this tomorrow so we can reinstate the tests. cc @aduth |
@notnownikki Sounds good, thanks! |
* Add user locale api-fetch middleware See WordPress#10811 * Don't modify existing _locale query arg * Add changelog entry * Update changelog * Use new hasQueryArg helper function See WordPress#10885. * Match on simply the route path, instead of malformed query param * Fully mock embedding test * Skip the embedding test until we can make the travis env mock the requests * Skip demo test too, for now
I know this is 6 years old, but we use API Fetch in frontend and for frontend requests, the language should be the site one, not the user one. There is no way to unset a middleware, a workaround for now is that I added a |
@senadir might be good to open a follow up issue if there's something to revise, not sure folks will be monitoring this one |
I will open a new issue, this currently open issue #16805 describes a good outcome of removing middlewares, but is 5 years old. |
Fixes #10811.
Description
This PR adds a new middleware to the
api-fetch
package that adds?_locale=user
to every REST API request, in order to make sure the responses are in the user's locale.I'm not very familiar with the package itself, but figured that another middleware makes sense. If not, I am open to suggestions and adjust or close this PR accordingly.
How has this been tested?
After applying the patch from https://core.trac.wordpress.org/ticket/44758 and following the steps from #8449, the taxonomy labels are properly displayed in the user's locale.
Also, there are unit tests covering the functionality of the middleware.
Types of changes
Adds a new
api-fetch
middleware to include?_locale=user
in JavaScript REST API requests.Checklist: