fix: refactor AbstractConnectionResolver::get_nodes() to prevent children double slicing#2294
Conversation
| if ( isset( $this->args['after'] ) ) { | ||
| $key = array_search( $this->get_offset(), array_keys( $nodes ), true ); | ||
| $nodes = array_slice( $nodes, $key + 1, null, true ); | ||
| public function get_ids_for_nodes() { |
There was a problem hiding this comment.
Function get_ids_for_nodes has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
| * @throws Exception | ||
| */ | ||
| public function get_nodes() { | ||
| public function get_ids_for_nodes() { |
There was a problem hiding this comment.
Function get_ids_for_nodes has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
| if ( isset( $this->args['after'] ) ) { | ||
| $key = array_search( $this->get_offset(), array_keys( $nodes ), true ); | ||
| $nodes = array_slice( $nodes, $key + 1, null, true ); | ||
| public function get_ids_for_nodes() { |
There was a problem hiding this comment.
Function get_ids_for_nodes has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
| if ( isset( $this->args['after'] ) ) { | ||
| $key = array_search( $this->get_offset(), array_keys( $nodes ), true ); | ||
| $nodes = array_slice( $nodes, $key + 1, null, true ); | ||
| public function get_ids_for_nodes() { |
There was a problem hiding this comment.
Function get_ids_for_nodes has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
| if ( isset( $this->args['after'] ) ) { | ||
| $key = array_search( $this->get_offset(), array_keys( $nodes ), true ); | ||
| $nodes = array_slice( $nodes, $key + 1, null, true ); | ||
| public function get_ids_for_nodes() { |
There was a problem hiding this comment.
Function get_ids_for_nodes has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
|
Code Climate has analyzed commit 329268f and detected 10 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
While the code changes are independent, adding tests require #2293 (to avoid dealing with merge conflicts). Marking this as blocked in the interim. |
What does this implement/fix? Explain your changes.
This PR refactors
AbstractConnectionResolver::get_nodes()to use a separate function (AbstractionConnectonResolver::get_ids_for_nodes()for slicing the query ids by thequery_amountandoffset.This serves three purposes:
get_nodes()methods, since the models are only fetched for the actual nodes to return.AbstractConnectionResolvermore DRY, since for most use cases in core,get_nodes()is really only used to change how the offsets are handled.pageInfocursors forpluginsandthemesare not correctly returningarrayconnection:{id}#2087 , since currentlyparent::get_nodes()slices byquery_amountbefore slicing byoffset, which is the entire reasonget_nodes()needs to be overwritten to begin with.Does this close any currently open issues?
#2087
Any other comments?
query_amountbefore slicing byoffest, while others need it to happen after. I see no solution to this that isn't a breaking change (if it is possible at all).AbstractConnectionResolver::get_nodes()by relying onparent::get_nodes()will still be broken as the latter is still slicing byquery_amountbefore `offset.Where has this been tested?
Operating System: Ubuntu 20.04 (WSL + Devilbox) + PHP 8.0.15
WordPress Version: 5.9.1