Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
6.0 Backport to add support for custom taxonomies filtering for Query Loop (from Gutenberg) #2488
6.0 Backport to add support for custom taxonomies filtering for Query Loop (from Gutenberg) #2488
Changes from 8 commits
d453c19
3163816
e51fd0c
b081331
cde5c14
fef0000
c658eb8
e22f662
ceedc0a
a3d71fb
a7d1560
d7b77aa
fec3207
6fbf8e5
0af2058
a3e5009
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The
implode
is unnecessary. The API can accept real arrays.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 get the following error if I replace
with
:
This error doesn't occur if
implode
gets called.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 think you are right.
Gutenberg_REST_Pattern_Directory_Controller
has to be fixed to accept arrays instead of usingimplode
to concatenate values.I will submit a PR shortly.
Please ignore my previous message.
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've submitted a PR that fixes this issue.
I would appreciate your review, @TimothyBJacobs.
cc: @spacedmonkey
Trac issue: https://core.trac.wordpress.org/ticket/55574
Github PR: #2591
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 can't see why this outer if is needed, the same checks are being done inside it.
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.
Just to 'encapsulate' (let's say) the 'migration' code. Also, both old attributes merge into a single one and without the extra if we would have more checks about new value and merge, etc..
I think there is no harm to leave this as is.
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.
This looks like it will stomp on any categories and tags in the shim above if the query block has both a legacy value and
taxQuery
defined.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 not sure if it will be possible to have the old and new attributes, as there is the client parsing that migrates the block. So in the case of deprecated blocks, we just fill the new attribute from the old ones.
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.
Thanks, I gather that will come in as part of a package.
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.
Using an
else if
instead of anif
would make this explicit for the future readers of this code. I'm thinking:Not a blocker for the backport, though.