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

Merged
merged 7 commits into from Jun 19, 2018

Conversation

Projects
None yet
5 participants
@chrisvanpatten
Contributor

chrisvanpatten commented Jun 17, 2018

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.
encodeURIComponent( key ) + '=' + encodeURIComponent( paramValue );
} ).join( '&' );
getQueryStringFromAttributes( attributes ) {
return isEmpty( attributes.attributes ) ? '' : '&' + httpBuildQuery( attributes );

This comment has been minimized.

@dimofte

dimofte Jun 18, 2018

Contributor
  • 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

@dimofte

This comment has been minimized.

Contributor

dimofte commented Jun 18, 2018

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

This comment has been minimized.

Contributor

dimofte commented Jun 18, 2018

@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

This comment has been minimized.

Member

danielbachhuber commented Jun 18, 2018

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

Yes please.

@danielbachhuber danielbachhuber requested a review from aduth Jun 18, 2018

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 18, 2018

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

@chrisvanpatten

This comment has been minimized.

Contributor

chrisvanpatten commented Jun 18, 2018

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

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

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 18, 2018

Member

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 } ) : '' );

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 18, 2018

Member

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

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 18, 2018

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

This comment has been minimized.

Contributor

chrisvanpatten commented Jun 19, 2018

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

This comment has been minimized.

@youknowriad

youknowriad Jun 19, 2018

Contributor

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

This comment has been minimized.

@chrisvanpatten

chrisvanpatten Jun 19, 2018

Contributor

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

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 19, 2018

Member

I tested querystring-es3 locally but it doesn't pass our tests:

image

It appears the project is relatively abandoned too: https://github.com/SpainTrain/querystring-es3

This comment has been minimized.

@youknowriad

youknowriad Jun 19, 2018

Contributor

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

This comment has been minimized.

@chrisvanpatten

chrisvanpatten Jun 19, 2018

Contributor

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

This comment has been minimized.

@youknowriad

youknowriad Jun 19, 2018

Contributor

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.

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 19, 2018

Member

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;

This comment has been minimized.

@youknowriad

youknowriad Jun 19, 2018

Contributor

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

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 19, 2018

Member

Is it possible to have a null attributes?

Nope.

This comment has been minimized.

@chrisvanpatten

chrisvanpatten Jun 19, 2018

Contributor

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

This comment has been minimized.

@chrisvanpatten

chrisvanpatten Jun 19, 2018

Contributor

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

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 19, 2018

Member

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

No. null is more precise.

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

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

2 checks passed

codecov/project 46.7% remains the same compared to 60b57db
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -0,0 +1,64 @@
import httpBuildQuery from 'http-build-query';

This comment has been minimized.

@youknowriad

youknowriad Jun 19, 2018

Contributor

Also why are we testing an external library?

This comment has been minimized.

@chrisvanpatten

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 19, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo Jun 19, 2018

Member

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

This comment has been minimized.

Member

gziolo commented Jun 20, 2018

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

This comment has been minimized.

Member

gziolo commented Jun 20, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment