Skip to content
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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

dimofte
Copy link
Contributor

@dimofte dimofte commented Jun 8, 2018

Description

Fixes #7086
The SSR component doesn't properly encode attributes like arrays of objects.

How has this been tested?

Manually in Chrome on MacOS.
The component doesn't have automated tests and I haven't implemented any.

Screenshots

Types of changes

Bugfix

Checklist:

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

@aduth
Copy link
Member

aduth commented Jun 8, 2018

Would _.isObjectLike work here, in place of _.isPlainObject?

https://lodash.com/docs/4.17.10#isObjectLike

Aside: Also mentioned on the pull request where this component was introduced, but it really ought not be the responsibility of the component to do basic behaviors like encoding URL parameters. Instead, we should leverage existing modules, or at the very least extract to a common, generalized/shared/well-tested approach. Currently it's needlessly ad-hoc, which is why it doesn't surprise me there are small issues like this cropping up.

@chrisvanpatten
Copy link
Contributor

@aduth Thanks for taking a look!

While we were researching this one (this and #7229 are from us at @TomodomoCo) I came across a few JS equivalents of http_build_query, which can transform the various data structures into a query string that PHP (and thus the REST API) can read.

These two are both MIT and thus would be compatible w/ Gutenberg:

Of course Core could also provide this as a module.

Would that be a reasonable approach? That is effectively what getQueryUrlFromObject function is doing; just shifts away the responsibility.

@dimofte
Copy link
Contributor Author

dimofte commented Jun 8, 2018

_.isObjectLike would be more concise and would satisfy the current conditions, but it would also be true for more complex objects (and then _.map does apply to object methods, usually with undesired results). My knowledge of this system is limited, so I opted for the fix with the smallest possible splash

@antpb antpb mentioned this pull request Jun 11, 2018
@chrisvanpatten
Copy link
Contributor

@aduth would it be possible to get a review on this, and then look at reworking it later on? We're happy to own the ticket and create a better solution over the longer term (I think there are some other systemic issues here too that could be solved simultaneously), but right now we have to run off a Gutenberg fork in production to get our SSR blocks rendering, which isn't ideal 😬

@jasmussen
Copy link
Contributor

@gziolo @jorgefilipecosta @youknowriad does this issue have an easy resolution? Is this something we could get in 3.1?

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Jun 13, 2018

Thanks for the comment @jasmussen.

Just as a bit of background, I agree that either separating the code that builds the query string, or adding a dependency for an existing package that reimplements PHP's http_build_query for JS, would be ideal. However, I think something like that will obviously take more time, dev discussion, etc.

Adding another check for arrays will solve the problem today, giving us time to think through a more properly-architected solution.

As @dimofte noted: _.isObjectLike could work as well, but it could mean objects with methods pass the check and break serialisation in unexpected ways. Adding only the array check contains the fix to a really small surface area, which feels safer for now.

@danielbachhuber
Copy link
Member

As noted in #7229 (comment), we should exclude attributes from the request when it is empty.

@danielbachhuber
Copy link
Member

Also, in looking at this further, I think we should use vladzadvorny/http-build-query, either by pulling it in as a dependency or copying its implementation into our codebase with attribution.

I don't think it makes sense for us to re-implement the wheel with getQueryUrlFromObject()

@chrisvanpatten
Copy link
Contributor

@danielbachhuber agree completely. Would it be possible to merge this in as a temporary fix, and then we'll open a new PR with the http_build_query implementation?

@danielbachhuber
Copy link
Member

Would it be possible to merge this in as a temporary fix, and then we'll open a new PR with the http_build_query implementation?

I'd prefer to get it correct in this pull request, particularly because we don't have any test coverage of getQueryUrlFromObject() at this time.

@chrisvanpatten
Copy link
Contributor

@danielbachhuber Fair enough… we'll have to live with running our fork in production for another few days :) Will see how quickly we can get this updated.

@chrisvanpatten
Copy link
Contributor

@danielbachhuber I've replaced this PR with #7339! I've also asked @dimofte to pop in and add a test in that branch, if you think one would be appropriate for the getQueryStringFromAttributes method. Thanks! 🎉

@dimofte
Copy link
Contributor Author

dimofte commented Jun 18, 2018

I've removed the method getQueryStringFromAttributes (for reasons stated in a comment on that PR), but I've added the relevant tests to the package itself

@dimofte
Copy link
Contributor Author

dimofte commented Jun 18, 2018

closing this PR in favour of #7339

@dimofte dimofte closed this Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server side render encodes some attributes incorrectly
5 participants