-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Improve serialising JSON to PHP-compatible query strings #7339
Improve serialising JSON to PHP-compatible query strings #7339
Conversation
encodeURIComponent( key ) + '=' + encodeURIComponent( paramValue ); | ||
} ).join( '&' ); | ||
getQueryStringFromAttributes( attributes ) { | ||
return isEmpty( attributes.attributes ) ? '' : '&' + httpBuildQuery( attributes ); |
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.
attributes
is the property of the function argument, not the argument itself ("object" was more appropriate)&
is beyond the responsibility of this function, it should be up to the caller to glue the query params to the url with?
or&
I suggest ditching the function and moving the line to fetch
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.
As a point of sanity checking, can we include tests on this PR too? This way, if there are ever regressions in the upstream library, we'll have visibility into them here.
@danielbachhuber what do you mean, do you want to add the tests for |
Yes please. |
Flagging @aduth for approval of adding the library dependency. I'm unsure of our policy on adding dependencies. |
@danielbachhuber @aduth if a dependency isn’t viable we could copy the function with credit to the @wordpress/url package, perhaps? Not sure how that works from a license perspective but it seems like a good place for a utility like this. |
4ae3563
to
6a4d3d2
Compare
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.
Two more minor nits, sorry.
@@ -46,9 +47,10 @@ export class ServerSideRender extends Component { | |||
if ( null !== this.state.response ) { | |||
this.setState( { response: null } ); | |||
} | |||
const { block, attributes = {} } = props; |
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.
On second thought, I think attributes
should default to null
and we can continue to leave it optional.
|
||
const path = '/gutenberg/v1/block-renderer/' + block + '?context=edit&' + this.getQueryUrlFromObject( { attributes } ); | ||
const path = `/gutenberg/v1/block-renderer/${ block }?context=edit` + | ||
( attributes ? '&' + httpBuildQuery( { attributes } ) : '' ); |
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.
attributes ?
should be null !== attributes
, per above.
Yes, we could copy with attribution. I'm not opposed to including the dependency though. I simply haven't merged a pull request with a dependency, so I want to get a sanity check from someone who has. |
@danielbachhuber @aduth I think we're ready for a new review! 🚀 |
@@ -12,6 +12,7 @@ import { | |||
} from '@wordpress/element'; | |||
import { __, sprintf } from '@wordpress/i18n'; | |||
import apiRequest from '@wordpress/api-request'; | |||
import httpBuildQuery from 'http-build-query'; |
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.
Should we use querystring
instead? It's already bundled in webpack and used by several packages in Gutenberg?
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.
@youknowriad PHP has its own unique format for query strings of arrays/objects.
Here's a rough example showing how each method encodes the query string differently:
qs = require('querystring');
hbq = require('http-build-query');
const obj = { "array": [ "a", "b", "c" ] };
qs.stringify(obj);
// array=a&array=b&array=c
hbq(obj);
// array[0]=a&array[1]=b&array[2]=c
Because http-build-query
is just re-implementing PHP's native http_build_query
function, it's transforming into the format that PHP can properly decode. If the querystring
version of that object was passed onto PHP, it would be decoded as array( 'array' => 'c' )
and drop the other values in the array.
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.
I tested querystring-es3
locally but it doesn't pass our tests:
It appears the project is relatively abandoned too: https://github.com/SpainTrain/querystring-es3
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.
I'm pretty sure there's an option in querystring
to do the exact same thing.
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.
@youknowriad I don't believe it does. I also looked at querystringify
which is also included in Gutenberg and also doesn't support this format.
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.
Ok so discussed in Slack, it's fine to keep http-build-query
but I'd appreciate if we don't rush into merging when there's unresolved comments.
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.
I'd appreciate if we don't rush into merging when there's unresolved comments.
At the time I merged, it wasn't clear there were unresolved comments.
@@ -46,9 +47,10 @@ export class ServerSideRender extends Component { | |||
if ( null !== this.state.response ) { | |||
this.setState( { response: null } ); | |||
} | |||
const { block, attributes = {} } = props; | |||
const { block, attributes = null } = props; |
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.
Is it possible to have a null
attributes? I thought we always fallback to {}
?
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.
Is it possible to have a
null
attributes?
Nope.
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.
That was based on this: #7339 (comment) but I'm happy to go either way :)
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 should I update this to use {}
instead of null
?
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.
should I update this to use
{}
instead ofnull
?
No. null
is more precise.
@@ -0,0 +1,64 @@ | |||
import httpBuildQuery from 'http-build-query'; |
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.
Also why are we testing an external library?
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.
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.
Better implementation in #7371
|
||
// The following tests are adapted to the behavior of http-build-query v0.7.0, | ||
// to help mitigating possible regressions in the upstream library. | ||
describe( 'http-build-query', function() { |
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.
Shouldn't we test our own code interacting with this library instead? I don't see any value in having this kind of tests. We use dozens of external libraries and we never test them directly.
This PR also updated 1 000+ |
Never mind, |
Description
This PR replaces #7232 and fixes #7086.
How has this been tested?
Manually in Chrome and Safari on macOS.
Types of changes
vladzadvorny/http-build-query
(link)http-build-query
to generate a PHP-compatible query string of block attributes for the Block Renderer API callNote that there is a case wherehttp-build-query
diverges from the PHP implementation ofhttp_build_query
, where unneeded&
symbols can be added to a query string for empty array or object values, however it does not cause any issues (PHP simply ignores these empty parameters when parsing the query string) and I've reported the issue upstream.The bug has already been fixed! I've updated to
http-build-query
0.7.0 in the PR.Checklist: