Skip to content

Commit 56fbf54

Browse files
jasonbahljasonbahl
authored
Merge pull request #3104 from justlevine/dev/cr2-backport/refactor-should_execute
refactor: make `AbstractConnectionResolver::should_execute()` non-abstract and add `::pre_should_execute()`
2 parents a0d1103 + 8014e2c commit 56fbf54

10 files changed

+108
-108
lines changed

‎src/Data/Connection/AbstractConnectionResolver.php‎

Lines changed: 89 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,13 @@ abstract class AbstractConnectionResolver {
6464
/**
6565
* Whether the connection resolver should execute.
6666
*
67-
* @var bool
67+
* If `false`, the connection resolve will short-circuit and return an empty array.
68+
*
69+
* Filterable by `graphql_connection_pre_should_execute` and `graphql_connection_should_execute`.
70+
*
71+
* @var ?bool
6872
*/
69-
protected $should_execute = true;
73+
protected $should_execute;
7074

7175
/**
7276
* The loader name.
@@ -172,8 +176,8 @@ public function __construct( $source, array $args, AppContext $context, ResolveI
172176
*/
173177
$this->args = $args;
174178

175-
// Bail if the Post->ID is empty, as that indicates a private post.
176-
if ( $source instanceof Post && empty( $source->ID ) ) {
179+
// Pre-check if the connection should execute so we can skip expensive logic if we already know it shouldn't execute.
180+
if ( ! $this->get_pre_should_execute( $this->source, $this->unfiltered_args, $this->context, $this->info ) ) {
177181
$this->should_execute = false;
178182
}
179183

@@ -257,22 +261,29 @@ abstract public function get_query_args();
257261
abstract public function get_query();
258262

259263
/**
260-
* Should_execute
261-
*
262-
* Determine whether or not the query should execute.
263-
*
264-
* Return true to execute, return false to prevent execution.
265-
*
266-
* Various criteria can be used to determine whether a Connection Query should
267-
* be executed.
264+
* Used to determine whether the connection query should be executed. This is useful for short-circuiting the connection resolver before executing the query.
268265
*
269-
* For example, if a user is requesting revisions of a Post, and the user doesn't have
270-
* permission to edit the post, they don't have permission to view the revisions, and therefore
271-
* we can prevent the query to fetch revisions from executing in the first place.
266+
* When `pre_should_excecute()` returns false, that's a sign the Resolver shouldn't execute the query. Otherwise, the more expensive logic logic in `should_execute()` will run later in the lifecycle.
272267
*
273-
* @return bool
268+
* @param mixed $source Source passed down from the resolve tree
269+
* @param array<string,mixed> $args Array of arguments input in the field as part of the GraphQL query.
270+
* @param \WPGraphQL\AppContext $context The app context that gets passed down the resolve tree.
271+
* @param \GraphQL\Type\Definition\ResolveInfo $info Info about fields passed down the resolve tree.
274272
*/
275-
abstract public function should_execute();
273+
protected function pre_should_execute( $source, array $args, AppContext $context, ResolveInfo $info ): bool {
274+
$should_execute = true;
275+
276+
/**
277+
* If the source is a Post and the ID is empty (i.e. if the user doesn't have permissions to view the source), we should not execute the query.
278+
*
279+
* @todo This can probably be abstracted to check if _any_ source is private, and not just `PostObject` models.
280+
*/
281+
if ( $source instanceof Post && empty( $source->ID ) ) {
282+
$should_execute = false;
283+
}
284+
285+
return $should_execute;
286+
}
276287

277288
/**
278289
* The maximum number of items that should be returned by the query.
@@ -322,6 +333,25 @@ public function get_ids_from_query() {
322333
);
323334
}
324335

336+
/**
337+
* Determine whether or not the query should execute.
338+
*
339+
* Return true to exeucte, return false to prevent execution.
340+
*
341+
* Various criteria can be used to determine whether a Connection Query should be executed.
342+
*
343+
* For example, if a user is requesting revisions of a Post, and the user doesn't have permission to edit the post, they don't have permission to view the revisions, and therefore we can prevent the query to fetch revisions from executing in the first place.
344+
*
345+
* Runs only if `pre_should_execute()` returns true.
346+
*
347+
* @todo This is public for b/c but it should be protected.
348+
*
349+
* @return bool
350+
*/
351+
public function should_execute() {
352+
return true;
353+
}
354+
325355
/**
326356
* Returns the offset for a given cursor.
327357
*
@@ -428,12 +458,6 @@ public function get_loader_name() {
428458
return $this->loader_name;
429459
}
430460

431-
/**
432-
* Returns whether the connection should execute.
433-
*/
434-
public function get_should_execute(): bool {
435-
return $this->should_execute;
436-
}
437461

438462
/**
439463
* Returns the $args passed to the connection, before any modifications.
@@ -496,6 +520,19 @@ public function get_query_amount() {
496520
return $this->query_amount;
497521
}
498522

523+
/**
524+
* Returns whether the connection should execute.
525+
*
526+
* If conditions are met that should prevent the execution, we can bail from resolving early, before the query is executed.
527+
*/
528+
public function get_should_execute(): bool {
529+
// If `pre_should_execute()` or other logic has yet to run, we should run the full `should_execute()` logic.
530+
if ( ! isset( $this->should_execute ) ) {
531+
$this->should_execute = $this->should_execute();
532+
}
533+
534+
return $this->should_execute;
535+
}
499536

500537
/**
501538
* Returns an array of IDs for the connection.
@@ -649,6 +686,33 @@ public function one_to_one() {
649686
return $this;
650687
}
651688

689+
/**
690+
* Gets whether or not the query should execute, BEFORE any data is fetched or altered, filtered by 'graphql_connection_pre_should_execute'.
691+
*
692+
* @param mixed $source The source that's passed down the GraphQL queries.
693+
* @param array<string,mixed> $args The inputArgs on the field.
694+
* @param \WPGraphQL\AppContext $context The AppContext passed down the GraphQL tree.
695+
* @param \GraphQL\Type\Definition\ResolveInfo $info The ResolveInfo passed down the GraphQL tree.
696+
*/
697+
protected function get_pre_should_execute( $source, array $args, AppContext $context, ResolveInfo $info ): bool {
698+
$should_execute = $this->pre_should_execute( $source, $args, $context, $info );
699+
700+
/**
701+
* Filters whether or not the query should execute, BEFORE any data is fetched or altered.
702+
*
703+
* This is evaluated based solely on the values passed to the constructor, before any data is fetched or altered, and is useful for shortcircuiting the Connection Resolver before any heavy logic is executed.
704+
*
705+
* For more in-depth checks, use the `graphql_connection_should_execute` filter instead.
706+
*
707+
* @param bool $should_execute Whether or not the query should execute.
708+
* @param mixed $source The source that's passed down the GraphQL queries.
709+
* @param array $args The inputArgs on the field.
710+
* @param \WPGraphQL\AppContext $context The AppContext passed down the GraphQL tree.
711+
* @param \GraphQL\Type\Definition\ResolveInfo $info The ResolveInfo passed down the GraphQL tree.
712+
*/
713+
return apply_filters( 'graphql_connection_pre_should_execute', $should_execute, $source, $args, $context, $info );
714+
}
715+
652716
/**
653717
* Returns the loader.
654718
*
@@ -792,12 +856,9 @@ function () {
792856
* @throws \Exception
793857
*/
794858
public function execute_and_get_ids() {
795-
796859
/**
797-
* If should_execute is explicitly set to false already, we can
798-
* prevent execution quickly. If it's not, we need to
799-
* call the should_execute() method to execute any situational logic
800-
* to determine if the connection query should execute or not
860+
* If should_execute is explicitly set to false already, we can prevent execution quickly.
861+
* If it's not, we need to call the should_execute() method to execute any situational logic to determine if the connection query should execute.
801862
*/
802863
$should_execute = false === $this->should_execute ? false : $this->should_execute();
803864

‎src/Data/Connection/CommentConnectionResolver.php‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -311,11 +311,4 @@ public function sanitize_input_fields( array $args ) {
311311
public function is_valid_offset( $offset ) {
312312
return ! empty( get_comment( $offset ) );
313313
}
314-
315-
/**
316-
* {@inheritDoc}
317-
*/
318-
public function should_execute() {
319-
return true;
320-
}
321314
}

‎src/Data/Connection/ContentTypeConnectionResolver.php‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -73,11 +73,4 @@ protected function loader_name(): string {
7373
public function is_valid_offset( $offset ) {
7474
return (bool) get_post_type_object( $offset );
7575
}
76-
77-
/**
78-
* {@inheritDoc}
79-
*/
80-
public function should_execute() {
81-
return true;
82-
}
8376
}

‎src/Data/Connection/EnqueuedScriptsConnectionResolver.php‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,11 +72,4 @@ public function is_valid_offset( $offset ) {
7272
global $wp_scripts;
7373
return isset( $wp_scripts->registered[ $offset ] );
7474
}
75-
76-
/**
77-
* {@inheritDoc}
78-
*/
79-
public function should_execute() {
80-
return true;
81-
}
8275
}

‎src/Data/Connection/EnqueuedStylesheetConnectionResolver.php‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -79,11 +79,4 @@ public function is_valid_offset( $offset ) {
7979
global $wp_styles;
8080
return isset( $wp_styles->registered[ $offset ] );
8181
}
82-
83-
/**
84-
* {@inheritDoc}
85-
*/
86-
public function should_execute() {
87-
return true;
88-
}
8982
}

‎src/Data/Connection/PostObjectConnectionResolver.php‎

Lines changed: 19 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -113,33 +113,30 @@ public function get_ids_from_query() {
113113
* {@inheritDoc}
114114
*/
115115
public function should_execute() {
116-
if ( false === $this->should_execute ) {
117-
return false;
118-
}
119-
120116
/**
121-
* For revisions, we only want to execute the connection query if the user
122-
* has access to edit the parent post.
117+
* If the post_type is not revision we can just return the parent::should_execute().
123118
*
124-
* If the user doesn't have permission to edit the parent post, then we shouldn't
125-
* even execute the connection
126-
*/
127-
if ( isset( $this->post_type ) && 'revision' === $this->post_type ) {
128-
if ( $this->source instanceof Post ) {
129-
$parent_post_type_obj = get_post_type_object( $this->source->post_type );
130-
if ( ! isset( $parent_post_type_obj->cap->edit_post ) || ! current_user_can( $parent_post_type_obj->cap->edit_post, $this->source->ID ) ) {
131-
$this->should_execute = false;
132-
}
133-
/**
134-
* If the connection is from the RootQuery, check if the user
135-
* has the 'edit_posts' capability
136-
*/
137-
} elseif ( ! current_user_can( 'edit_posts' ) ) {
138-
$this->should_execute = false;
119+
* @todo This works because AbstractConnectionResolver::pre_should_execute does a permission check on the `Post` model )
120+
*/
121+
if ( ! isset( $this->post_type ) || 'revision' !== $this->post_type ) {
122+
return parent::should_execute();
123+
}
124+
125+
// If the connection is from the RootQuery (i.e. it doesn't have a `Post` source), check if the user has the 'edit_posts' capability.
126+
if ( ! $this->source instanceof Post && current_user_can( 'edit_posts' ) ) {
127+
return true;
128+
}
129+
130+
// For revisions, we only want to execute the connection query if the user has access to edit the parent post.
131+
if ( $this->source instanceof Post ) {
132+
$parent_post_type_obj = get_post_type_object( $this->source->post_type );
133+
134+
if ( isset( $parent_post_type_obj->cap->edit_post ) && current_user_can( $parent_post_type_obj->cap->edit_post, $this->source->ID ) ) {
135+
return true;
139136
}
140137
}
141138

142-
return $this->should_execute;
139+
return false;
143140
}
144141

145142
/**

‎src/Data/Connection/TaxonomyConnectionResolver.php‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,4 @@ protected function loader_name(): string {
7474
public function is_valid_offset( $offset ) {
7575
return (bool) get_taxonomy( $offset );
7676
}
77-
78-
/**
79-
* {@inheritDoc}
80-
*/
81-
public function should_execute() {
82-
return true;
83-
}
8477
}

‎src/Data/Connection/TermObjectConnectionResolver.php‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -294,13 +294,4 @@ static function ( $id ) {
294294
public function is_valid_offset( $offset ) {
295295
return get_term( absint( $offset ) ) instanceof \WP_Term;
296296
}
297-
298-
/**
299-
* {@inheritDoc}
300-
*
301-
* Default is true, meaning any time a TermObjectConnection resolver is asked for, it will execute.
302-
*/
303-
public function should_execute() {
304-
return true;
305-
}
306297
}

‎src/Data/Connection/ThemeConnectionResolver.php‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -67,11 +67,4 @@ public function is_valid_offset( $offset ) {
6767
$theme = wp_get_theme( $offset );
6868
return $theme->exists();
6969
}
70-
71-
/**
72-
* {@inheritDoc}
73-
*/
74-
public function should_execute() {
75-
return true;
76-
}
7770
}

‎src/Data/Connection/UserConnectionResolver.php‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -281,11 +281,4 @@ protected function sanitize_input_fields( array $args ) {
281281
public function is_valid_offset( $offset ) {
282282
return (bool) get_user_by( 'ID', absint( $offset ) );
283283
}
284-
285-
/**
286-
* {@inheritDoc}
287-
*/
288-
public function should_execute() {
289-
return true;
290-
}
291284
}

0 commit comments

Comments
 (0)