Improve serialising JSON to PHP-compatible query strings #7339

chrisvanpatten · 2018-06-17T13:31:31Z

Description

This PR replaces #7232 and fixes #7086.

How has this been tested?

Manually in Chrome and Safari on macOS.

Types of changes

Adds a dependency: vladzadvorny/http-build-query (link)
Uses http-build-query to generate a PHP-compatible query string of block attributes for the Block Renderer API call

Note that there is a case where http-build-query diverges from the PHP implementation of http_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:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

…ress#7086)

dimofte · 2018-06-18T06:54:42Z

components/server-side-render/index.js

-				encodeURIComponent( key ) + '=' + encodeURIComponent( paramValue );
-		} ).join( '&' );
+	getQueryStringFromAttributes( attributes ) {
+		return isEmpty( attributes.attributes ) ? '' : '&' + httpBuildQuery( attributes );


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

…for attributes

dimofte · 2018-06-18T08:34:19Z

In the PR referenced above, I've added some tests to the package http-build-query, relevant to issues #7086 and #7214 . Of course, we don't need to wait for that to be merged (the tests pass)

danielbachhuber

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.

dimofte · 2018-06-18T14:05:43Z

@danielbachhuber what do you mean, do you want to add the tests for http-build-query to gutengerg?
Testing the parent function, fetch is, imo, outside of the scope of this PR.

danielbachhuber · 2018-06-18T14:07:46Z

what do you mean, do you want to add the tests for http-build-query to Gutenberg?

Yes please.

danielbachhuber · 2018-06-18T15:34:07Z

Flagging @aduth for approval of adding the library dependency. I'm unsure of our policy on adding dependencies.

chrisvanpatten · 2018-06-18T15:57:05Z

@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.

danielbachhuber

Two more minor nits, sorry.

danielbachhuber · 2018-06-18T17:25:08Z

components/server-side-render/index.js

@@ -46,9 +47,10 @@ export class ServerSideRender extends Component {
 		if ( null !== this.state.response ) {
 			this.setState( { response: null } );
 		}
-		const { block, attributes = {} } = props;


On second thought, I think attributes should default to null and we can continue to leave it optional.

danielbachhuber · 2018-06-18T17:25:32Z

components/server-side-render/index.js


-		const path = '/gutenberg/v1/block-renderer/' + block + '?context=edit&' + this.getQueryUrlFromObject( { attributes } );
+		const path = `/gutenberg/v1/block-renderer/${ block }?context=edit` +
+			( attributes ? '&' + httpBuildQuery( { attributes } ) : '' );


attributes ? should be null !== attributes, per above.

danielbachhuber · 2018-06-18T17:29:53Z

if a dependency isn’t viable we could copy the function with credit to the @wordpress/url package, perhaps?

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.

chrisvanpatten · 2018-06-19T11:58:01Z

@danielbachhuber @aduth I think we're ready for a new review! 🚀

youknowriad · 2018-06-19T12:30:50Z

components/server-side-render/index.js

@@ -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';


Should we use querystring instead? It's already bundled in webpack and used by several packages in Gutenberg?

@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.

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

I'm pretty sure there's an option in querystring to do the exact same thing.

@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.

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.

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.

youknowriad · 2018-06-19T12:31:36Z

components/server-side-render/index.js

@@ -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;


Is it possible to have a null attributes? I thought we always fallback to {}?

Is it possible to have a null attributes?

Nope.

That was based on this: #7339 (comment) but I'm happy to go either way :)

@danielbachhuber should I update this to use {} instead of null?

should I update this to use {} instead of null?

No. null is more precise.

youknowriad · 2018-06-19T12:56:23Z

components/server-side-render/test/index.js

@@ -0,0 +1,64 @@
+import httpBuildQuery from 'http-build-query';


Also why are we testing an external library?

@youknowriad #7339 (comment)

Better implementation in #7371

gziolo · 2018-06-19T13:21:14Z

components/server-side-render/test/index.js

+
+// 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() {


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.

gziolo · 2018-06-20T10:50:12Z

This PR also updated 1 000+ integrity sha values which makes package-lock.json to be updated locally with the clean master. What version of npm did you use and which platform? Trying to understand why it happens.

gziolo · 2018-06-20T10:59:01Z

Never mind, rm -rf node_modules && npm install solved it.

chrisvanpatten added 2 commits June 17, 2018 10:22

Improve serialising JSON to PHP-compatible query strings (fixes WordP…

92ca53b

…ress#7086)

Fix an eslint issue (unused map method)

a766084

chrisvanpatten mentioned this pull request Jun 17, 2018

Bugfix: ServerSideRender should serialize arrays, similarly to objects #7232

Closed

4 tasks

Update http-build-query to 0.7.0

3fc9c7a

dimofte reviewed Jun 18, 2018

View reviewed changes

ServerSideRender: remove an over-scoped function and a default value …

7a3cd9d

…for attributes

dimofte mentioned this pull request Jun 18, 2018

tests: empty input, arrays of objects vladzadvorny/http-build-query#5

Merged

danielbachhuber requested changes Jun 18, 2018

View reviewed changes

ServerSideRender: cleanup unused var (oups, linting..)

0ebc884

danielbachhuber requested a review from aduth June 18, 2018 15:33

ServerSideRender: tests for http-build-query

6a4d3d2

dimofte force-pushed the fix/ssr-api-url-serialisation branch from 4ae3563 to 6a4d3d2 Compare June 18, 2018 16:53

danielbachhuber requested changes Jun 18, 2018

View reviewed changes

allow null default for attributes

8dfdaee

danielbachhuber approved these changes Jun 19, 2018

View reviewed changes

danielbachhuber added the Framework Issues related to broader framework topics, especially as it relates to javascript label Jun 19, 2018

youknowriad reviewed Jun 19, 2018

View reviewed changes

danielbachhuber modified the milestones: 3.2, 3.1 Jun 19, 2018

danielbachhuber merged commit 74842d0 into WordPress:master Jun 19, 2018

youknowriad reviewed Jun 19, 2018

View reviewed changes

danielbachhuber mentioned this pull request Jun 19, 2018

Introduce rendererPathWithAttributes() for ServerSideRender #7371

Merged

gziolo reviewed Jun 19, 2018

View reviewed changes

danielbachhuber mentioned this pull request Jun 20, 2018

Regenerate integrity checks to sha512 #7399

Merged

youknowriad mentioned this pull request Jul 30, 2018

Use a unique querystring package instead of three different ones #8300

Merged

chrisvanpatten mentioned this pull request Nov 18, 2019

Packages: Stop using Node.js built-ins in browser-packaged code #13386

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve serialising JSON to PHP-compatible query strings #7339

Improve serialising JSON to PHP-compatible query strings #7339

chrisvanpatten commented Jun 17, 2018 •

edited

Loading

dimofte Jun 18, 2018 •

edited

Loading

dimofte commented Jun 18, 2018

danielbachhuber left a comment

dimofte commented Jun 18, 2018

danielbachhuber commented Jun 18, 2018

danielbachhuber commented Jun 18, 2018

chrisvanpatten commented Jun 18, 2018 •

edited

Loading

danielbachhuber left a comment

danielbachhuber Jun 18, 2018

danielbachhuber Jun 18, 2018

danielbachhuber commented Jun 18, 2018

chrisvanpatten commented Jun 19, 2018

youknowriad Jun 19, 2018

chrisvanpatten Jun 19, 2018 •

edited

Loading

danielbachhuber Jun 19, 2018

youknowriad Jun 19, 2018

chrisvanpatten Jun 19, 2018

youknowriad Jun 19, 2018

danielbachhuber Jun 19, 2018

youknowriad Jun 19, 2018

danielbachhuber Jun 19, 2018

chrisvanpatten Jun 19, 2018

chrisvanpatten Jun 19, 2018

danielbachhuber Jun 19, 2018

youknowriad Jun 19, 2018

chrisvanpatten Jun 19, 2018

danielbachhuber Jun 19, 2018

gziolo Jun 19, 2018

gziolo commented Jun 20, 2018

gziolo commented Jun 20, 2018

		@@ -0,0 +1,64 @@
		import httpBuildQuery from 'http-build-query';

Improve serialising JSON to PHP-compatible query strings #7339

Improve serialising JSON to PHP-compatible query strings #7339

Conversation

chrisvanpatten commented Jun 17, 2018 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

dimofte Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

dimofte commented Jun 18, 2018

danielbachhuber left a comment

Choose a reason for hiding this comment

dimofte commented Jun 18, 2018

danielbachhuber commented Jun 18, 2018

danielbachhuber commented Jun 18, 2018

chrisvanpatten commented Jun 18, 2018 • edited Loading

danielbachhuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Jun 18, 2018

chrisvanpatten commented Jun 19, 2018

Choose a reason for hiding this comment

chrisvanpatten Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jun 20, 2018

gziolo commented Jun 20, 2018

chrisvanpatten commented Jun 17, 2018 •

edited

Loading

dimofte Jun 18, 2018 •

edited

Loading

chrisvanpatten commented Jun 18, 2018 •

edited

Loading

chrisvanpatten Jun 19, 2018 •

edited

Loading