-
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
Bugfix: ServerSideRender should serialize arrays, similarly to objects #7232
Conversation
Would 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. |
@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 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 |
|
@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 😬 |
@gziolo @jorgefilipecosta @youknowriad does this issue have an easy resolution? Is this something we could get in 3.1? |
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 Adding another check for arrays will solve the problem today, giving us time to think through a more properly-architected solution. As @dimofte noted: |
As noted in #7229 (comment), we should exclude |
Also, in looking at this further, I think we should use I don't think it makes sense for us to re-implement the wheel with |
@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 |
I'd prefer to get it correct in this pull request, particularly because we don't have any test coverage of |
@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. |
@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 |
I've removed the method |
closing this PR in favour of #7339 |
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: