From 2fbbe59d5cdcfe9a33143b89a572906471a509f5 Mon Sep 17 00:00:00 2001 From: David Levine Date: Sat, 30 Mar 2024 11:23:40 +0000 Subject: [PATCH 1/5] dev: deprecate camelCased methods in `AbstractConnectionResolver` --- .../Connection/AbstractConnectionResolver.php | 125 +++++++++++++----- 1 file changed, 92 insertions(+), 33 deletions(-) diff --git a/src/Data/Connection/AbstractConnectionResolver.php b/src/Data/Connection/AbstractConnectionResolver.php index d720b1688..e12b0a419 100644 --- a/src/Data/Connection/AbstractConnectionResolver.php +++ b/src/Data/Connection/AbstractConnectionResolver.php @@ -145,7 +145,7 @@ public function __construct( $source, array $args, AppContext $context, ResolveI } // Get the loader for the Connection. - $this->loader = $this->getLoader(); + $this->loader = $this->get_loader(); /** * @@ -184,15 +184,6 @@ public function __construct( $source, array $args, AppContext $context, ResolveI $this->query_args = apply_filters( 'graphql_connection_query_args', $this->get_query_args(), $this, $args ); } - /** - * Returns the source of the connection - * - * @return mixed - */ - public function getSource() { - return $this->source; - } - /** * Returns the $args passed to the connection. * @@ -204,27 +195,6 @@ public function get_args(): array { return $this->args; } - /** - * Returns the AppContext of the connection - */ - public function getContext(): AppContext { - return $this->context; - } - - /** - * Returns the ResolveInfo of the connection - */ - public function getInfo(): ResolveInfo { - return $this->info; - } - - /** - * Returns whether the connection should execute - */ - public function getShouldExecute(): bool { - return $this->should_execute; - } - /** * Get_loader_name * @@ -341,7 +311,6 @@ public function get_offset_for_cursor( string $cursor = null ) { return is_numeric( $offset ) ? absint( $offset ) : $offset; } - /** * Validates Model. * @@ -356,6 +325,36 @@ protected function is_valid_model( $model ) { return isset( $model->fields ) && ! empty( $model->fields ); } + /** + * Returns the source of the connection + * + * @return mixed + */ + public function get_source() { + return $this->source; + } + + /** + * Returns the AppContext of the connection. + */ + public function get_context(): AppContext { + return $this->context; + } + + /** + * Returns the ResolveInfo of the connection. + */ + public function get_info(): ResolveInfo { + return $this->info; + } + + /** + * Returns whether the connection should execute. + */ + public function get_should_execute(): bool { + return $this->should_execute; + } + /** * Returns the amount of items to query from the database. * @@ -555,7 +554,7 @@ public function one_to_one() { * @return \WPGraphQL\Data\Loader\AbstractDataLoader * @throws \Exception */ - protected function getLoader() { + protected function get_loader() { $name = $this->get_loader_name(); if ( empty( $name ) || ! is_string( $name ) ) { throw new Exception( esc_html__( 'The Connection Resolver needs to define a loader name', 'wp-graphql' ) ); @@ -1009,4 +1008,64 @@ public function get_offset() { return $this->get_offset_for_cursor( $cursor ); } + + /** + * Returns the source of the connection. + * + * @deprecated @todo in favor of $this->get_source(). + * + * @return mixed + */ + public function getSource() { + _deprecated_function( __METHOD__, '@todo', static::class . '::get_source()' ); + + return $this->get_source(); + } + + /** + * Returns the AppContext of the connection. + * + * @deprecated @todo in favor of $this->get_context(). + */ + public function getContext(): AppContext { + _deprecated_function( __METHOD__, '@todo', static::class . '::get_context()' ); + + return $this->get_context(); + } + + /** + * Returns the ResolveInfo of the connection. + * + * @deprecated @todo in favor of $this->get_info(). + */ + public function getInfo(): ResolveInfo { + _deprecated_function( __METHOD__, '@todo', static::class . '::get_info()' ); + + return $this->get_info(); + } + + /** + * Returns whether the connection should execute. + * + * @deprecated @todo in favor of $this->get_should_execute(). + */ + public function getShouldExecute(): bool { + _deprecated_function( __METHOD__, '@todo', static::class . '::should_execute()' ); + + return $this->get_should_execute(); + } + + /** + * Returns the loader. + * + * @deprecated @todo in favor of $this->get_loader(). + * + * @return \WPGraphQL\Data\Loader\AbstractDataLoader + * @throws \Exception + */ + protected function getLoader() { + _deprecated_function( __METHOD__, '@todo', static::class . '::get_loader()' ); + + return $this->get_loader(); + } } From 70e6919a5f72520a17cba2537c5ed5eb666d36ab Mon Sep 17 00:00:00 2001 From: David Levine Date: Sat, 30 Mar 2024 13:12:42 +0000 Subject: [PATCH 2/5] refactor: add `::prepare_page_info()`and only instantiate once --- .../Connection/AbstractConnectionResolver.php | 92 ++++++++++++------- 1 file changed, 58 insertions(+), 34 deletions(-) diff --git a/src/Data/Connection/AbstractConnectionResolver.php b/src/Data/Connection/AbstractConnectionResolver.php index d720b1688..aa2ad0374 100644 --- a/src/Data/Connection/AbstractConnectionResolver.php +++ b/src/Data/Connection/AbstractConnectionResolver.php @@ -115,6 +115,15 @@ abstract class AbstractConnectionResolver { */ protected $edges; + /** + * The page info for the connection. + * + * Filterable by `graphql_connection_page_info`. + * + * @var ?array + */ + protected $page_info; + /** * The query amount to return for the connection. * @@ -486,41 +495,40 @@ public function get_edges() { * @return array */ public function get_page_info() { - $page_info = [ - 'startCursor' => $this->get_start_cursor(), - 'endCursor' => $this->get_end_cursor(), - 'hasNextPage' => (bool) $this->has_next_page(), - 'hasPreviousPage' => (bool) $this->has_previous_page(), - ]; + if ( ! isset( $this->page_info ) ) { + $page_info = $this->prepare_page_info(); - /** - * Filter the pageInfo that is returned to the connection. - * - * This filter allows for additional fields to be filtered into the pageInfo - * of a connection, such as "totalCount", etc, because the filter has enough - * context of the query, args, request, etc to be able to calculate and return - * that information. - * - * example: - * - * You would want to register a "total" field to the PageInfo type, then filter - * the pageInfo to return the total for the query, something to this tune: - * - * add_filter( 'graphql_connection_page_info', function( $page_info, $connection ) { - * - * $page_info['total'] = null; - * - * if ( $connection->query instanceof WP_Query ) { - * if ( isset( $connection->query->found_posts ) { - * $page_info['total'] = (int) $connection->query->found_posts; - * } - * } - * - * return $page_info; - * - * }); - */ - return apply_filters( 'graphql_connection_page_info', $page_info, $this ); + /** + * Filter the pageInfo that is returned to the connection. + * + * This filter allows for additional fields to be filtered into the pageInfo + * of a connection, such as "totalCount", etc, because the filter has enough + * context of the query, args, request, etc to be able to calcuate and return + * that information. + * + * example: + * + * You would want to register a "total" field to the PageInfo type, then filter + * the pageInfo to return the total for the query, something to this tune: + * + * add_filter( 'graphql_connection_page_info', function( $page_info, $connection ) { + * + * $page_info['total'] = null; + * + * if ( $connection->query instanceof WP_Query ) { + * if ( isset( $connection->query->found_posts ) { + * $page_info['total'] = (int) $connection->query->found_posts; + * } + * } + * + * return $page_info; + * + * }); + */ + $this->page_info = apply_filters( 'graphql_connection_page_info', $page_info, $this ); + } + + return $this->page_info; } /** @@ -861,6 +869,22 @@ protected function get_cursor_for_node( $id ) { return base64_encode( 'arrayconnection:' . (string) $id ); } + /** + * Prepares the page info for the connection. + * + * @used-by self::get_page_info() + * + * @return array + */ + protected function prepare_page_info(): array { + return [ + 'startCursor' => $this->get_start_cursor(), + 'endCursor' => $this->get_end_cursor(), + 'hasNextPage' => $this->has_next_page(), + 'hasPreviousPage' => $this->has_previous_page(), + ]; + } + /** * Determine the start cursor from the connection * From cb4e6168c8d66023e07b4b0bc75ebb6b3b212e90 Mon Sep 17 00:00:00 2001 From: David Levine Date: Sat, 30 Mar 2024 14:13:21 +0000 Subject: [PATCH 3/5] dev: add `AbstractConnectionResolver::get_unfiltered_args()` --- .../Connection/AbstractConnectionResolver.php | 34 ++++++++++++++++--- .../Connection/CommentConnectionResolver.php | 2 +- .../Connection/MenuItemConnectionResolver.php | 4 +-- .../PostObjectConnectionResolver.php | 4 +-- .../TermObjectConnectionResolver.php | 4 +-- 5 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/Data/Connection/AbstractConnectionResolver.php b/src/Data/Connection/AbstractConnectionResolver.php index d720b1688..fcda3d7e3 100644 --- a/src/Data/Connection/AbstractConnectionResolver.php +++ b/src/Data/Connection/AbstractConnectionResolver.php @@ -24,9 +24,18 @@ abstract class AbstractConnectionResolver { */ protected $source; + /** + * The args input before it is filtered and prepared by the constructor. + * + * @var array + */ + protected $unfiltered_args; + /** * The args input on the field calling the connection. * + * Filterable by `graphql_connection_args`. + * * @var array */ protected $args; @@ -134,10 +143,16 @@ abstract class AbstractConnectionResolver { */ public function __construct( $source, array $args, AppContext $context, ResolveInfo $info ) { // Set the source (the root object), context, resolveInfo, and unfiltered args for the resolver. - $this->source = $source; - $this->args = $args; - $this->context = $context; - $this->info = $info; + $this->source = $source; + $this->unfiltered_args = $args; + $this->context = $context; + $this->info = $info; + + /** + * @todo This exists for b/c, where extenders may be directly accessing `$this->args` in ::get_loader() or even `::get_args()`. + * We can remove this once the rest of lifecyle has been updated. + */ + $this->args = $args; // Bail if the Post->ID is empty, as that indicates a private post. if ( $source instanceof Post && empty( $source->ID ) ) { @@ -157,7 +172,7 @@ public function __construct( $source, array $args, AppContext $context, ResolveI * * @since 1.11.0 */ - $this->args = apply_filters( 'graphql_connection_args', $this->get_args(), $this, $args ); + $this->args = apply_filters( 'graphql_connection_args', $this->get_args(), $this, $this->get_unfiltered_args() ); /** * Determine the query amount for the resolver. @@ -356,6 +371,15 @@ protected function is_valid_model( $model ) { return isset( $model->fields ) && ! empty( $model->fields ); } + /** + * Returns the $args passed to the connection, before any modifications. + * + * @return array + */ + public function get_unfiltered_args(): array { + return $this->unfiltered_args; + } + /** * Returns the amount of items to query from the database. * diff --git a/src/Data/Connection/CommentConnectionResolver.php b/src/Data/Connection/CommentConnectionResolver.php index 6033c0dd6..2d773e1a0 100644 --- a/src/Data/Connection/CommentConnectionResolver.php +++ b/src/Data/Connection/CommentConnectionResolver.php @@ -183,7 +183,7 @@ public function get_ids_from_query() { * @return array */ public function get_args(): array { - $args = $this->args; + $args = $this->get_unfiltered_args(); if ( ! empty( $args['where'] ) ) { // Ensure all IDs are converted to database IDs. diff --git a/src/Data/Connection/MenuItemConnectionResolver.php b/src/Data/Connection/MenuItemConnectionResolver.php index 4ff9b2aac..36710c599 100644 --- a/src/Data/Connection/MenuItemConnectionResolver.php +++ b/src/Data/Connection/MenuItemConnectionResolver.php @@ -80,7 +80,7 @@ public function get_query_args() { * {@inheritDoc} */ public function get_args(): array { - $args = $this->args; + $args = $this->get_unfiltered_args(); if ( ! empty( $args['where'] ) ) { // Ensure all IDs are converted to database IDs. @@ -106,6 +106,6 @@ public function get_args(): array { * * @since 1.11.0 */ - return apply_filters( 'graphql_menu_item_connection_args', $args, $this->args ); + return apply_filters( 'graphql_menu_item_connection_args', $args, $this->get_unfiltered_args() ); } } diff --git a/src/Data/Connection/PostObjectConnectionResolver.php b/src/Data/Connection/PostObjectConnectionResolver.php index 1963510cc..288bd8c36 100644 --- a/src/Data/Connection/PostObjectConnectionResolver.php +++ b/src/Data/Connection/PostObjectConnectionResolver.php @@ -533,7 +533,7 @@ static function ( $status ) use ( $post_type_objects ) { * {@inheritDoc} */ public function get_args(): array { - $args = $this->args; + $args = $this->get_unfiltered_args(); if ( ! empty( $args['where'] ) ) { // Ensure all IDs are converted to database IDs. @@ -580,7 +580,7 @@ static function ( $id ) { * * @since 1.11.0 */ - return apply_filters( 'graphql_post_object_connection_args', $args, $this, $this->args ); + return apply_filters( 'graphql_post_object_connection_args', $args, $this, $this->get_unfiltered_args() ); } /** diff --git a/src/Data/Connection/TermObjectConnectionResolver.php b/src/Data/Connection/TermObjectConnectionResolver.php index 8b8b060a3..40acc6715 100644 --- a/src/Data/Connection/TermObjectConnectionResolver.php +++ b/src/Data/Connection/TermObjectConnectionResolver.php @@ -243,7 +243,7 @@ public function sanitize_input_fields() { * {@inheritDoc} */ public function get_args(): array { - $args = $this->args; + $args = $this->get_unfiltered_args(); if ( ! empty( $args['where'] ) ) { // Ensure all IDs are converted to database IDs. @@ -284,7 +284,7 @@ static function ( $id ) { * * @since 1.11.0 */ - return apply_filters( 'graphql_term_object_connection_args', $args, $this, $this->args ); + return apply_filters( 'graphql_term_object_connection_args', $args, $this, $this->get_unfiltered_args() ); } /** From 596d571ef46918683f6e6ef4a24f768e0e226e93 Mon Sep 17 00:00:00 2001 From: David Levine Date: Sat, 30 Mar 2024 15:50:43 +0000 Subject: [PATCH 4/5] refactor: improve query amount handling in `AbstractConnectionResolver` --- .../Connection/AbstractConnectionResolver.php | 139 ++++++++++-------- .../Connection/CommentConnectionResolver.php | 5 +- .../EnqueuedScriptsConnectionResolver.php | 34 +---- .../EnqueuedStylesheetConnectionResolver.php | 33 +---- .../PostObjectConnectionResolver.php | 7 +- .../TermObjectConnectionResolver.php | 5 +- .../PostObjectConnectionQueriesTest.php | 61 +++++++- 7 files changed, 151 insertions(+), 133 deletions(-) diff --git a/src/Data/Connection/AbstractConnectionResolver.php b/src/Data/Connection/AbstractConnectionResolver.php index d720b1688..a1714eb08 100644 --- a/src/Data/Connection/AbstractConnectionResolver.php +++ b/src/Data/Connection/AbstractConnectionResolver.php @@ -118,7 +118,7 @@ abstract class AbstractConnectionResolver { /** * The query amount to return for the connection. * - * @var int + * @var ?int */ protected $query_amount; @@ -159,16 +159,7 @@ public function __construct( $source, array $args, AppContext $context, ResolveI */ $this->args = apply_filters( 'graphql_connection_args', $this->get_args(), $this, $args ); - /** - * Determine the query amount for the resolver. - * - * This is the amount of items to query from the database. We determine this by - * determining how many items were asked for (first/last), then compare with the - * max amount allowed to query (default is 100), and then we fetch 1 more than - * that amount, so we know whether hasNextPage/hasPreviousPage should be true. - * - * If there are more items than were asked for, then there's another page. - */ + // Get the query amount for the connection. $this->query_amount = $this->get_query_amount(); /** @@ -277,6 +268,15 @@ abstract public function get_query(); * @return bool */ abstract public function should_execute(); + + /** + * The maximum number of items that should be returned by the query. + * + * This is filtered by `graphql_connection_max_query_amount` in ::get_query_amount(). + */ + protected function max_query_amount(): int { + return 100; + } /** * Is_valid_offset @@ -367,33 +367,45 @@ protected function is_valid_model( $model ) { * @throws \Exception */ public function get_query_amount() { + if ( ! isset( $this->query_amount ) ) { + /** + * Filter the maximum number of posts per page that should be queried. This prevents queries from being exceedingly resource intensive. + * + * The default is 100 - unless overloaded by ::max_query_amount() in the child class. + * + * @param int $max_posts the maximum number of posts per page. + * @param mixed $source source passed down from the resolve tree + * @param array $args array of arguments input in the field as part of the GraphQL query + * @param \WPGraphQL\AppContext $context Object containing app context that gets passed down the resolve tree + * @param \GraphQL\Type\Definition\ResolveInfo $info Info about fields passed down the resolve tree + * + * @since 0.0.6 + */ + $max_query_amount = (int) apply_filters( 'graphql_connection_max_query_amount', $this->max_query_amount(), $this->source, $this->args, $this->context, $this->info ); - /** - * Filter the maximum number of posts per page that should be queried. The default is 100 to prevent queries from - * being exceedingly resource intensive, however individual systems can override this for their specific needs. - * - * This filter is intentionally applied AFTER the query_args filter, as - * - * @param int $max_posts the maximum number of posts per page. - * @param mixed $source source passed down from the resolve tree - * @param array $args array of arguments input in the field as part of the GraphQL query - * @param \WPGraphQL\AppContext $context Object containing app context that gets passed down the resolve tree - * @param \GraphQL\Type\Definition\ResolveInfo $info Info about fields passed down the resolve tree - * - * @since 0.0.6 - */ - $max_query_amount = apply_filters( 'graphql_connection_max_query_amount', 100, $this->source, $this->args, $this->context, $this->info ); - - $requested_amount = $this->get_amount_requested(); - - if ( $requested_amount > $max_query_amount ) { - graphql_debug( - sprintf( 'The number of items requested by the connection (%s) exceeds the max query amount. Only the first %s items will be returned.', $requested_amount, $max_query_amount ), - [ 'connection' => static::class ] + // We don't want the requested amount to be lower than 0. + $requested_query_amount = (int) max( + 0, + /** + * This filter allows to modify the number of nodes the connection should return. + * + * @param int $amount the requested amount + * @param self $resolver Instance of the connection resolver class + */ + apply_filters( 'graphql_connection_amount_requested', $this->get_amount_requested(), $this ) ); + + if ( $requested_query_amount > $max_query_amount ) { + graphql_debug( + sprintf( 'The number of items requested by the connection (%s) exceeds the max query amount. Only the first %s items will be returned.', $requested_query_amount, $max_query_amount ), + [ 'connection' => static::class ] + ); + } + + $this->query_amount = (int) min( $max_query_amount, $requested_query_amount ); } - return min( $max_query_amount, $requested_amount ); + return $this->query_amount; } /** @@ -568,54 +580,53 @@ protected function getLoader() { * Returns the amount of items requested from the connection. * * @return int + * * @throws \GraphQL\Error\UserError If the `first` or `last` args are used together. */ public function get_amount_requested() { - /** - * Set the default amount + * Filters the default query amount for a connection, if no `first` or `last` GraphQL argument is supplied. + * + * @param int $amount_requested The default query amount for a connection. + * @param self $resolver Instance of the Connection Resolver. */ - $amount_requested = 10; + $amount_requested = apply_filters( 'graphql_connection_default_query_amount', 10, $this ); /** - * If both first & last are used in the input args, throw an exception as that won't - * work properly + * If both first & last are used in the input args, throw an exception. */ if ( ! empty( $this->args['first'] ) && ! empty( $this->args['last'] ) ) { - throw new UserError( esc_html__( 'first and last cannot be used together. For forward pagination, use first & after. For backward pagination, use last & before.', 'wp-graphql' ) ); + throw new UserError( esc_html__( 'The `first` and `last` connection args cannot be used together. For forward pagination, use `first` & `after`. For backward pagination, use `last` & `before`.', 'wp-graphql' ) ); } /** - * If first is set, and is a positive integer, use it for the $amount_requested - * but if it's set to anything that isn't a positive integer, throw an exception + * Get the key to use for the query amount. + * We avoid a ternary here for unit testing. */ - if ( ! empty( $this->args['first'] ) && is_int( $this->args['first'] ) ) { - if ( 0 > $this->args['first'] ) { - throw new UserError( esc_html__( 'first must be a positive integer.', 'wp-graphql' ) ); - } - - $amount_requested = $this->args['first']; + $args_key = ! empty( $this->args['first'] ) && is_int( $this->args['first'] ) ? 'first' : null; + if ( null === $args_key ) { + $args_key = ! empty( $this->args['last'] ) && is_int( $this->args['last'] ) ? 'last' : null; } /** - * If last is set, and is a positive integer, use it for the $amount_requested + * If the key is set, and is a positive integer, use it for the $amount_requested * but if it's set to anything that isn't a positive integer, throw an exception */ - if ( ! empty( $this->args['last'] ) && is_int( $this->args['last'] ) ) { - if ( 0 > $this->args['last'] ) { - throw new UserError( esc_html__( 'last must be a positive integer.', 'wp-graphql' ) ); + if ( null !== $args_key && isset( $this->args[ $args_key ] ) ) { + if ( 0 > $this->args[ $args_key ] ) { + throw new UserError( + sprintf( + // translators: %s: The name of the arg that was invalid + esc_html__( '%s must be a positive integer.', 'wp-graphql' ), + esc_html( $args_key ) + ) + ); } - $amount_requested = $this->args['last']; + $amount_requested = $this->args[ $args_key ]; } - /** - * This filter allows to modify the requested connection page size - * - * @param int $amount the requested amount - * @param \WPGraphQL\Data\Connection\AbstractConnectionResolver $resolver Instance of the connection resolver class - */ - return (int) max( 0, apply_filters( 'graphql_connection_amount_requested', $amount_requested, $this ) ); + return (int) $amount_requested; } /** @@ -831,11 +842,11 @@ public function get_ids_for_nodes() { // If we're going backwards then our overfetched ID is at the front. if ( ! empty( $this->args['last'] ) && count( $this->ids ) > absint( $this->args['last'] ) ) { - return array_slice( $this->ids, count( $this->ids ) - absint( $this->args['last'] ), $this->query_amount, true ); + return array_slice( $this->ids, count( $this->ids ) - absint( $this->args['last'] ), $this->get_query_amount(), true ); } // If we're going forwards, our overfetched ID is at the back. - return array_slice( $this->ids, 0, $this->query_amount, true ); + return array_slice( $this->ids, 0, $this->get_query_amount(), true ); } /** @@ -919,7 +930,7 @@ public function get_before_offset() { */ public function has_next_page() { if ( ! empty( $this->args['first'] ) ) { - return ! empty( $this->ids ) && count( $this->ids ) > $this->query_amount; + return ! empty( $this->ids ) && count( $this->ids ) > $this->get_query_amount(); } $before_offset = $this->get_before_offset(); @@ -941,7 +952,7 @@ public function has_next_page() { */ public function has_previous_page() { if ( ! empty( $this->args['last'] ) ) { - return ! empty( $this->ids ) && count( $this->ids ) > $this->query_amount; + return ! empty( $this->ids ) && count( $this->ids ) > $this->get_query_amount(); } $after_offset = $this->get_after_offset(); diff --git a/src/Data/Connection/CommentConnectionResolver.php b/src/Data/Connection/CommentConnectionResolver.php index 6033c0dd6..40ca2fd34 100644 --- a/src/Data/Connection/CommentConnectionResolver.php +++ b/src/Data/Connection/CommentConnectionResolver.php @@ -29,8 +29,7 @@ public function get_query_args() { /** * Prepare for later use */ - $last = ! empty( $this->args['last'] ) ? $this->args['last'] : null; - $first = ! empty( $this->args['first'] ) ? $this->args['first'] : null; + $last = ! empty( $this->args['last'] ) ? $this->args['last'] : null; $query_args = []; @@ -49,7 +48,7 @@ public function get_query_args() { * * @since 0.0.6 */ - $query_args['number'] = min( max( absint( $first ), absint( $last ), 10 ), $this->get_query_amount() ) + 1; + $query_args['number'] = $this->get_query_amount() + 1; /** * Set the default order diff --git a/src/Data/Connection/EnqueuedScriptsConnectionResolver.php b/src/Data/Connection/EnqueuedScriptsConnectionResolver.php index e310ce220..4e5eb4655 100644 --- a/src/Data/Connection/EnqueuedScriptsConnectionResolver.php +++ b/src/Data/Connection/EnqueuedScriptsConnectionResolver.php @@ -1,39 +1,12 @@ fieldName || 'registeredScripts' === $info->fieldName ) { - return 1000; - } - return $max; - }, - 10, - 5 - ); - - parent::__construct( $source, $args, $context, $info ); - } - /** * {@inheritDoc} */ @@ -76,6 +49,13 @@ public function get_loader_name() { return 'enqueued_script'; } + /** + * {@inheritDoc} + */ + protected function max_query_amount(): int { + return 1000; + } + /** * {@inheritDoc} * diff --git a/src/Data/Connection/EnqueuedStylesheetConnectionResolver.php b/src/Data/Connection/EnqueuedStylesheetConnectionResolver.php index cdbfac502..f13c8fbc3 100644 --- a/src/Data/Connection/EnqueuedStylesheetConnectionResolver.php +++ b/src/Data/Connection/EnqueuedStylesheetConnectionResolver.php @@ -1,9 +1,6 @@ fieldName || 'registeredStylesheets' === $info->fieldName ) { - return 1000; - } - return $max; - }, - 10, - 5 - ); - - parent::__construct( $source, $args, $context, $info ); - } - /** * {@inheritDoc} */ @@ -82,6 +56,13 @@ public function get_loader_name() { return 'enqueued_stylesheet'; } + /** + * {@inheritDoc} + */ + protected function max_query_amount(): int { + return 1000; + } + /** * {@inheritDoc} * diff --git a/src/Data/Connection/PostObjectConnectionResolver.php b/src/Data/Connection/PostObjectConnectionResolver.php index 1963510cc..ab9a40114 100644 --- a/src/Data/Connection/PostObjectConnectionResolver.php +++ b/src/Data/Connection/PostObjectConnectionResolver.php @@ -149,8 +149,7 @@ public function get_query_args() { /** * Prepare for later use */ - $last = ! empty( $this->args['last'] ) ? $this->args['last'] : null; - $first = ! empty( $this->args['first'] ) ? $this->args['first'] : null; + $last = ! empty( $this->args['last'] ) ? $this->args['last'] : null; $query_args = []; /** @@ -176,10 +175,10 @@ public function get_query_args() { /** * Set posts_per_page the highest value of $first and $last, with a (filterable) max of 100 */ - $query_args['posts_per_page'] = $this->one_to_one ? 1 : min( max( absint( $first ), absint( $last ), 10 ), $this->query_amount ) + 1; + $query_args['posts_per_page'] = $this->one_to_one ? 1 : $this->get_query_amount() + 1; // set the graphql cursor args - $query_args['graphql_cursor_compare'] = ( ! empty( $last ) ) ? '>' : '<'; + $query_args['graphql_cursor_compare'] = ! empty( $last ) ? '>' : '<'; $query_args['graphql_after_cursor'] = $this->get_after_offset(); $query_args['graphql_before_cursor'] = $this->get_before_offset(); diff --git a/src/Data/Connection/TermObjectConnectionResolver.php b/src/Data/Connection/TermObjectConnectionResolver.php index 8b8b060a3..a058e2c58 100644 --- a/src/Data/Connection/TermObjectConnectionResolver.php +++ b/src/Data/Connection/TermObjectConnectionResolver.php @@ -59,8 +59,7 @@ public function get_query_args() { /** * Prepare for later use */ - $last = ! empty( $this->args['last'] ) ? $this->args['last'] : null; - $first = ! empty( $this->args['first'] ) ? $this->args['first'] : null; + $last = ! empty( $this->args['last'] ) ? $this->args['last'] : null; /** * Set hide_empty as false by default @@ -70,7 +69,7 @@ public function get_query_args() { /** * Set the number, ensuring it doesn't exceed the amount set as the $max_query_amount */ - $query_args['number'] = min( max( absint( $first ), absint( $last ), 10 ), $this->query_amount ) + 1; + $query_args['number'] = $this->get_query_amount() + 1; /** * Don't calculate the total rows, it's not needed and can be expensive diff --git a/tests/wpunit/PostObjectConnectionQueriesTest.php b/tests/wpunit/PostObjectConnectionQueriesTest.php index b1fbea95d..d7cb27fb6 100644 --- a/tests/wpunit/PostObjectConnectionQueriesTest.php +++ b/tests/wpunit/PostObjectConnectionQueriesTest.php @@ -153,7 +153,7 @@ private function getReturnField( $data, $post, $field = '' ) { public function testMaxQueryAmount() { // Create some additional posts to test a large query. - $this->create_posts( 150 ); + $post_ids = $this->create_posts( 150 ); $query = $this->getQuery(); @@ -186,16 +186,65 @@ static function () { $actual = $this->graphql( compact( 'query', 'variables' ) ); + $this->assertCount( 20, $actual['data']['posts']['edges'] ); + $this->assertTrue( $actual['data']['posts']['pageInfo']['hasNextPage'] ); + $this->assertStringContainsString( 'The number of items requested by the connection (150) exceeds the max query amount.', $actual['extensions']['debug'][0]['message'] ); + + // Cleanup + remove_all_filters( 'graphql_connection_max_query_amount' ); + foreach( $post_ids as $post_id ) { + wp_delete_post( $post_id, true ); + } + } + + public function testDefaultQueryAmount() { + // Create some additional posts to test a large query. + $post_ids = $this->create_posts( 25 ); + + $query = $this->getQuery(); + + $variables = [ + 'first' => null, + ]; + + $actual = $this->graphql( compact( 'query', 'variables' ) ); + $this->assertNotEmpty( $actual ); + + /** + * The default that can be queried by default is 10 items + */ + $this->assertCount( 10, $actual['data']['posts']['edges'], '10 items should be returned by default' ); + + /** + * Test the filter to make sure it's defaulting the results properly + */ add_filter( - 'graphql_connection_max_query_amount', + 'graphql_connection_default_query_amount', static function () { - return 100; + return 20; } ); - $this->assertCount( 20, $actual['data']['posts']['edges'] ); - $this->assertTrue( $actual['data']['posts']['pageInfo']['hasNextPage'] ); - $this->assertStringContainsString( 'The number of items requested by the connection (150) exceeds the max query amount.', $actual['extensions']['debug'][0]['message'] ); + $actual = $this->graphql( compact( 'query', 'variables' ) ); + + $this->assertCount( 20, $actual['data']['posts']['edges'], '20 items should be returned by default' ); + + add_filter( + 'graphql_connection_default_query_amount', + static function () { + return 5; + } + ); + + $actual = $this->graphql( compact( 'query', 'variables' ) ); + + $this->assertCount( 5, $actual['data']['posts']['edges'], '5 items should be returned by default' ); + + // Cleanup + remove_all_filters( 'graphql_connection_max_query_amount' ); + foreach( $post_ids as $post_id ) { + wp_delete_post( $post_id, true ); + } } /** From e549d01b5daf14da435b363b381c0c5424d69a90 Mon Sep 17 00:00:00 2001 From: David Levine Date: Sat, 30 Mar 2024 18:10:44 +0000 Subject: [PATCH 5/5] refactor: improve `loader` handling in `AbstractConnectionResolver` --- .../Connection/AbstractConnectionResolver.php | 95 +++++++++++++++---- .../Connection/CommentConnectionResolver.php | 2 +- .../ContentTypeConnectionResolver.php | 2 +- .../EnqueuedScriptsConnectionResolver.php | 2 +- .../EnqueuedStylesheetConnectionResolver.php | 2 +- .../Connection/PluginConnectionResolver.php | 2 +- .../PostObjectConnectionResolver.php | 2 +- .../Connection/TaxonomyConnectionResolver.php | 2 +- .../TermObjectConnectionResolver.php | 2 +- .../Connection/ThemeConnectionResolver.php | 2 +- .../Connection/UserConnectionResolver.php | 2 +- .../Connection/UserRoleConnectionResolver.php | 4 +- 12 files changed, 89 insertions(+), 30 deletions(-) diff --git a/src/Data/Connection/AbstractConnectionResolver.php b/src/Data/Connection/AbstractConnectionResolver.php index e12b0a419..e30c95c61 100644 --- a/src/Data/Connection/AbstractConnectionResolver.php +++ b/src/Data/Connection/AbstractConnectionResolver.php @@ -59,10 +59,19 @@ abstract class AbstractConnectionResolver { */ protected $should_execute = true; + /** + * The loader name. + * + * Defaults to `loader_name()` and filterable by `graphql_connection_loader_name`. + * + * @var ?string + */ + protected $loader_name; + /** * The loader the resolver is configured to use. * - * @var \WPGraphQL\Data\Loader\AbstractDataLoader + * @var ?\WPGraphQL\Data\Loader\AbstractDataLoader */ protected $loader; @@ -185,24 +194,26 @@ public function __construct( $source, array $args, AppContext $context, ResolveI } /** - * Returns the $args passed to the connection. + * The name of the loader to use for this connection. * - * Useful for modifying the $args before they are passed to $this->get_query_args(). + * Filterable by `graphql_connection_loader_name`. * - * @return array + * @todo This is protected for backwards compatibility, but should be abstract and implemented by the child classes. */ - public function get_args(): array { - return $this->args; + protected function loader_name(): string { + return ''; } /** - * Get_loader_name + * Returns the $args passed to the connection. * - * Return the name of the loader to be used with the connection resolver + * Useful for modifying the $args before they are passed to $this->get_query_args(). * - * @return string + * @return array */ - abstract public function get_loader_name(); + public function get_args(): array { + return $this->args; + } /** * Get_query_args @@ -348,6 +359,51 @@ public function get_info(): ResolveInfo { return $this->info; } + /** + * Returns the loader name. + * + * If $loader_name is not initialized, this plugin will initialize it. + * + * @return string + * + * @throws \Exception + */ + public function get_loader_name() { + // Only initialize the loader_name property once. + if ( ! isset( $this->loader_name ) ) { + $name = $this->loader_name(); + + // This is a b/c check because `loader_name()` is not abstract. + if ( empty( $name ) ) { + throw new \Exception( + sprintf( + // translators: %s is the name of the connection resolver class. + esc_html__( 'Class %s does not implement a valid method `loader_name()`.', 'wp-graphql' ), + esc_html( static::class ) + ) + ); + } + + /** + * Filters the loader name. + * This is the name of the registered DataLoader that will be used to load the data for the connection. + * + * @param string $loader_name The name of the loader. + * @param self $resolver The AbstractConnectionResolver instance. + */ + $name = apply_filters( 'graphql_connection_loader_name', $name, $this ); + + // Bail if the loader name is invalid. + if ( empty( $name ) || ! is_string( $name ) ) { + throw new \Exception( esc_html__( 'The Connection Resolver needs to define a loader name', 'wp-graphql' ) ); + } + + $this->loader_name = $name; + } + + return $this->loader_name; + } + /** * Returns whether the connection should execute. */ @@ -551,16 +607,19 @@ public function one_to_one() { /** * Returns the loader. * + * If $loader is not initialized, this method will initialize it. + * * @return \WPGraphQL\Data\Loader\AbstractDataLoader - * @throws \Exception */ protected function get_loader() { - $name = $this->get_loader_name(); - if ( empty( $name ) || ! is_string( $name ) ) { - throw new Exception( esc_html__( 'The Connection Resolver needs to define a loader name', 'wp-graphql' ) ); + // If the loader isn't set, set it. + if ( ! isset( $this->loader ) ) { + $name = $this->get_loader_name(); + + $this->loader = $this->context->get_loader( $name ); } - return $this->context->get_loader( $name ); + return $this->loader; } /** @@ -634,7 +693,7 @@ public function get_connection() { return new Deferred( function () { if ( ! empty( $this->ids ) ) { - $this->loader->load_many( $this->ids ); + $this->get_loader()->load_many( $this->ids ); } /** @@ -748,7 +807,7 @@ public function execute_and_get_ids() { /** * Buffer the IDs for deferred resolution */ - $this->loader->buffer( $this->ids ); + $this->get_loader()->buffer( $this->ids ); return $this->ids; } @@ -846,7 +905,7 @@ public function get_ids_for_nodes() { * @throws \Exception */ public function get_node_by_id( $id ) { - return $this->loader->load( $id ); + return $this->get_loader()->load( $id ); } /** diff --git a/src/Data/Connection/CommentConnectionResolver.php b/src/Data/Connection/CommentConnectionResolver.php index 6033c0dd6..5d5752f86 100644 --- a/src/Data/Connection/CommentConnectionResolver.php +++ b/src/Data/Connection/CommentConnectionResolver.php @@ -158,7 +158,7 @@ public function get_query() { /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'comment'; } diff --git a/src/Data/Connection/ContentTypeConnectionResolver.php b/src/Data/Connection/ContentTypeConnectionResolver.php index d9c27a532..3abbd80eb 100644 --- a/src/Data/Connection/ContentTypeConnectionResolver.php +++ b/src/Data/Connection/ContentTypeConnectionResolver.php @@ -61,7 +61,7 @@ public function get_query() { /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'post_type'; } diff --git a/src/Data/Connection/EnqueuedScriptsConnectionResolver.php b/src/Data/Connection/EnqueuedScriptsConnectionResolver.php index e310ce220..766d7eb29 100644 --- a/src/Data/Connection/EnqueuedScriptsConnectionResolver.php +++ b/src/Data/Connection/EnqueuedScriptsConnectionResolver.php @@ -72,7 +72,7 @@ public function get_query() { /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'enqueued_script'; } diff --git a/src/Data/Connection/EnqueuedStylesheetConnectionResolver.php b/src/Data/Connection/EnqueuedStylesheetConnectionResolver.php index cdbfac502..5d9513571 100644 --- a/src/Data/Connection/EnqueuedStylesheetConnectionResolver.php +++ b/src/Data/Connection/EnqueuedStylesheetConnectionResolver.php @@ -78,7 +78,7 @@ public function get_query() { /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'enqueued_stylesheet'; } diff --git a/src/Data/Connection/PluginConnectionResolver.php b/src/Data/Connection/PluginConnectionResolver.php index b7e5c8bd0..4430d669b 100644 --- a/src/Data/Connection/PluginConnectionResolver.php +++ b/src/Data/Connection/PluginConnectionResolver.php @@ -222,7 +222,7 @@ static function ( $plugin ) use ( $s ) { /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'plugin'; } diff --git a/src/Data/Connection/PostObjectConnectionResolver.php b/src/Data/Connection/PostObjectConnectionResolver.php index 1963510cc..7700829dd 100644 --- a/src/Data/Connection/PostObjectConnectionResolver.php +++ b/src/Data/Connection/PostObjectConnectionResolver.php @@ -69,7 +69,7 @@ public function __construct( $source, array $args, AppContext $context, ResolveI /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'post'; } diff --git a/src/Data/Connection/TaxonomyConnectionResolver.php b/src/Data/Connection/TaxonomyConnectionResolver.php index 0f94aa019..dd810d6c6 100644 --- a/src/Data/Connection/TaxonomyConnectionResolver.php +++ b/src/Data/Connection/TaxonomyConnectionResolver.php @@ -62,7 +62,7 @@ public function get_query() { /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'taxonomy'; } diff --git a/src/Data/Connection/TermObjectConnectionResolver.php b/src/Data/Connection/TermObjectConnectionResolver.php index 8b8b060a3..93867b82e 100644 --- a/src/Data/Connection/TermObjectConnectionResolver.php +++ b/src/Data/Connection/TermObjectConnectionResolver.php @@ -174,7 +174,7 @@ public function get_ids_from_query() { /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'term'; } diff --git a/src/Data/Connection/ThemeConnectionResolver.php b/src/Data/Connection/ThemeConnectionResolver.php index 2b11389e3..741072455 100644 --- a/src/Data/Connection/ThemeConnectionResolver.php +++ b/src/Data/Connection/ThemeConnectionResolver.php @@ -56,7 +56,7 @@ public function get_query() { /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'theme'; } diff --git a/src/Data/Connection/UserConnectionResolver.php b/src/Data/Connection/UserConnectionResolver.php index e58fcf91d..883c1d729 100644 --- a/src/Data/Connection/UserConnectionResolver.php +++ b/src/Data/Connection/UserConnectionResolver.php @@ -23,7 +23,7 @@ class UserConnectionResolver extends AbstractConnectionResolver { /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'user'; } diff --git a/src/Data/Connection/UserRoleConnectionResolver.php b/src/Data/Connection/UserRoleConnectionResolver.php index cd660dc18..b224f8bf2 100644 --- a/src/Data/Connection/UserRoleConnectionResolver.php +++ b/src/Data/Connection/UserRoleConnectionResolver.php @@ -57,14 +57,14 @@ public function get_query_args() { */ public function get_query() { $wp_roles = wp_roles(); - + return ! empty( $wp_roles->get_names() ) ? array_keys( $wp_roles->get_names() ) : []; } /** * {@inheritDoc} */ - public function get_loader_name() { + protected function loader_name(): string { return 'user_role'; }