Ensure comments REST controller returns note type children #10420
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
adamsilverstein
requested review from
Mamaduka,
TimothyBJacobs,
kadamwhite and
swissspidy
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
Would also include the patch from WordPress/gutenberg#72561 here. I was planning to create a sync PR, but I think it makes sense to do it here. |
| array( | ||
| 'count' => true, | ||
| 'orderby' => 'none', | ||
| 'type' => $comment->comment_type, |
There was a problem hiding this comment.
When a type is not provided, doesn't it mean all? I'm surprised that the it wasn't returning the note children already.
There was a problem hiding this comment.
I think this is required after #10405, where note type will be intentionally excluded.
There was a problem hiding this comment.
~Even before #10405 the test in this pr fails without the controller change on trunk. ~
There was a problem hiding this comment.
From the wording of #10405 it looks like all really does mean all, and would include Notes. Shouldn't we use that, then?
If we always assume the children will share type, is there a risk that this could cause unexpected behavior if you have a system where multiple comment types are structured together in a hierarchy, such as for example,
-+- Parent comment
+- Child comment
+- Custom Emoji reaction comment type
There was a problem hiding this comment.
I will test using 'all'. Until the other pr merges, not providing a type should imply 'all' though. I can see how this could break certain mixed type parent/children use cases, although developers could likely filter around that.
There was a problem hiding this comment.
kadamwhite
left a comment
kadamwhite
left a comment
There was a problem hiding this comment.
Broadly, this is working for me in the simple case and makes logical sense for our uses. I do have one question about whether we'd need to support checking for children across types in other scenarios
| array( | ||
| 'count' => true, | ||
| 'orderby' => 'none', | ||
| 'type' => $comment->comment_type, |
There was a problem hiding this comment.
From the wording of #10405 it looks like all really does mean all, and would include Notes. Shouldn't we use that, then?
If we always assume the children will share type, is there a risk that this could cause unexpected behavior if you have a system where multiple comment types are structured together in a hierarchy, such as for example,
-+- Parent comment
+- Child comment
+- Custom Emoji reaction comment type
|
I am closing this ticket in favor of #10405 - I moved the fix and unit test over to that PR since the issue being fixed is introduced in that PR. |
4 participants
Trac ticket: https://core.trac.wordpress.org/ticket/64152
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.