Feat: Refactor get_allowed_post_types() and get_allowed_taxonomies() by justlevine · Pull Request #2353 · wp-graphql/wp-graphql

justlevine · 2022-04-26T22:18:32Z

What does this implement/fix? Explain your changes

This PR refactors WPGraphQL::get_allowed_post_types() and WPGraphQL::get_allowed_taxonomies() to correctly store data in the WPGraphQL::$allowed_post_types and WPGraphQL::$allowed_taxonomies class properties.

Additionally, it stores the WP_Post_Type and WP_Taxonomy objects, allowing the user to retrieve either.

This will ensure that the list of possible post types / taxonomies is the same throughout the schema, and will also let us significantly clean up the codebase by removing now-extraneous rehydration calls ( e.g. get_post_type( \WPGraphQL::allowed_post_type( )[ $n ] ) ) in a future PR.

Details

The array of WP_Post_Types and WP_Taxonomies are stored to their respective parameters after any ...entities_allowed_{type} filters have been applied, but before any args passed to the methods.
WPGraphQL::get_allowed_post_types() now takes an $output argument as the first parameter that lets a user decide if they need the names or the objects, and relocates the$args` to the second parameter. This is backwards compatible and if an array is passed as the first arg, it will automatically reassign it.
WPGraphQL::get_allowed_taxonomies() accepts those same two parameters. There's no need to worry about back-compat, since it currently doesnt accept any.
Numerous calls to get_post_types() and get_taxonomies() have been replaced with the respective get_allowed...() methods. This fixes a handful of bugs where the list of "allowed" types was different throughout the plugin, which would break the schema (e.g. querying contentTypes after deregistering a core type ).
The WPGraphQL::$allowed_post_types and WPGraphQL::$allowed_taxonomies properties are now protected, to enforce the static getter pattern.

Does this close any currently open issues?

Any relevant logs, error output, GraphiQL screenshots, etc?

Any other comments?

As mentioned, this PR intentionally doesnt refactor the usage of these methods (e.g rehydrating the post object by first getting the names and then looping through them and calling get_post_type() ). That's a separate improvement deserving of its own PR.
I also didnt change many of the calls in NodeByUri, since that's already a finicky piece of code, and ideally we'd first get chore: refactor NodeResolver #2342 and better test how using a cached list of post/tax types might affect filters on the stored query vars.
Relatedly: the deprecation notice is currently commented out until we're no longer _doing_it_wrong() ourselves.
Storing an array of objects in WPGraphQL::$allowed_post_types and WPGraphQL::$allowed_taxonomies is non-breaking since theyre still array types, and they weren't actually holding anything beforehand. That said, while changing those properties to protected is ideal and wouldn't affect any users, ~~since it's technically a breaking change, I'll leave it to @jasonbahl to decide.~~ (edit: added by Jason).
The rationale behind the change in argument order for WPGraphQL::get_allowed_post_types() is to conform with PHP best practices, since it's significantly more likely to toggle between names/objects outputs than it is to pass args. Technically we don't even need to deprecate passing just the $args as shorthand for WPGraphQL::get_allowed_post_types( 'names', $args ) , and can just remove the commented out _doing_it_wrong. Again, I'll let @jasonbahl decide 😎.

Where has this been tested?

Operating System: Ubuntu 20.04 (wsl2 + devilbox + php 8.0.15)

WordPress Version: 5.9.3

qlty-cloud-legacy · 2022-04-26T22:19:02Z

src/WPGraphQL.php

-
-				$tax_object = get_taxonomy( $taxonomy );
+		// Initialize array of allowed post type objects.
+		if ( empty( self::$allowed_taxonomies ) ) {


Similar blocks of code found in 2 locations. Consider refactoring.

qlty-cloud-legacy · 2022-04-26T22:19:02Z

src/WPGraphQL.php

-				/** @var string $post_type */
-				$post_type_object = get_post_type_object( $post_type );
+		// Initialize array of allowed post type objects.
+		if ( empty( self::$allowed_post_types ) ) {


Similar blocks of code found in 2 locations. Consider refactoring.

qlty-cloud-legacy · 2022-04-26T22:19:02Z

src/WPGraphQL.php

-				'show_in_graphql' => true,
-			]
-		);
+	public static function get_allowed_taxonomies( $output = 'names', $args = [] ) {


Function get_allowed_taxonomies has a Cognitive Complexity of 13 (exceeds 5 allowed). Consider refactoring.

qlty-cloud-legacy · 2022-04-26T22:19:02Z

src/WPGraphQL.php

-		 * Get all post_types
-		 */
-		$post_types = get_post_types( array_merge( [ 'show_in_graphql' => true ], $args ) );
+	public static function get_allowed_post_types( $output = 'names', $args = [] ) {


Function get_allowed_post_types has a Cognitive Complexity of 14 (exceeds 5 allowed). Consider refactoring.

coveralls · 2022-04-26T22:27:06Z

Coverage increased (+0.08%) to 79.34% when pulling 5dadda1 on justlevine:feat/refactor-allowed-post-types-and-taxonomies into 7290bb9 on wp-graphql:develop.

jasonbahl · 2022-04-26T22:37:25Z

significantly reducing the number of database calls from repeatedly calling get_post_types() and get_taxonomies() respectively

@justlevine where were you seeing significant calls to the database from these functions? Maybe when using something like Custom Post Type UI or something like that? Curious what was causing the extraneous calls to the DB

justlevine · 2022-04-26T23:04:37Z

significantly reducing the number of database calls from repeatedly calling get_post_types() and get_taxonomies() respectively

@justlevine where were you seeing significant calls to the database from these functions? Maybe when using something like Custom Post Type UI or something like that? Curious what was causing the extraneous calls to the DB.

Honestly not sure. Now that you mention it I'm remembering that the wp functions are supposed to hit the globals first, but they were still leaking through on complex queries. Maybe it's an issue with my local object caching...

The original reason I wrote this was to eventually be able to clean up all the messy rehydration loops caused by only storing the names, and then I noticed both the drop in hits and how enforcing the pattern preemptively fixes bugs like with contentTypes queries or null type errors when deregistering core types. Might have jumped to conclusions prematurely.

I'm way out of my league on writing performance benchmarks, but maybe that makes sense before replacing some of existing get_post_types and get_taxonomies calls. Is the script you used to benchmark 1.6.0 public?

jasonbahl · 2022-04-26T23:31:09Z

Post Types and Taxonomies in core are registered in code, so there's no database queries at all for them.

I think the database queries must be from plugins that register Custom Post Types / Taxonomies via a GUI, like CPT UI. . .which is very common, so I'm happy to support those cases. It seems like those plugins should also be more responsible with how they filter get_post_types, etc, if they are indeed causing new queries to occur on each request.

I was curious to see if we could "prove" the impact this has. I think this is a beneficial refactor, regardless though.

Is the script you used to benchmark 1.6.0 public?

~~You can find it here: #1873 (comment)~~

err, hmm. Maybe it's not there 👀

...ok, now that I think about it more, I don't think the benchmarks were a formal script. I believe it was me loading the Schema and recording the time in a spreadsheet 🤔 😆

justlevine · 2022-04-26T23:55:48Z

I think the database queries must be from plugins that register Custom Post Types / Taxonomies via a GUI, like CPT UI. . .which is very common, so I'm happy to support those cases. It seems like those plugins should also be more responsible with how they filter get_post_types, etc, if they are indeed causing new queries to occur on each request.

Rank Math was the culprit. I was using a class_exists() instead of is_plugin_active(), so even though it was disabled their code was still being loaded. Fixed that and those calls went away 😊. For safety I ran install-test-env on a new docker instance, and didnt notice any notable performance differences with/without the pr.

...ok, now that I think about it more, I don't think the benchmarks were a formal script. I believe it was me loading the Schema and recording the time in a spreadsheet 🤔 😆

Haha nvm then. I'ma see if theres anything I can do about those codeclimate smells, and let you know when this is ready for review.

justlevine · 2022-04-27T00:58:41Z

I've got nothing. post types and taxonomies are so similar, but any abstractions I tried made it even harder to read ($objects, $allowlist, graphql_{$entity_type}_entities_allowed_{$wp_type}.

I'm open to suggestions or just leaving it as-is.

jasonbahl · 2022-04-27T22:11:35Z

@justlevine I updated the properties to be protected. I'm cool with that change.

closes #1679 possibly others.

Can you share how this closes that issue?

I just ran the query from that issue, and I'm seeing the same results described in the issue:

justlevine · 2022-04-28T02:15:50Z

It's working fine here, lemme investigate.

justlevine · 2022-04-28T04:06:27Z

Oops. Forgot to disable my other plugins just now, and like with the db calls it was leaking through from my dev env. Will abstract and post the temporary workaround there.

Since clearly I overpromised in the description, I'm going to go back and review everything from this pr.

qlty-cloud-legacy · 2022-04-28T07:05:31Z

Code Climate has analyzed commit 5dadda1 and detected 6 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	2
Duplication	4

View more on Code Climate.

justlevine · 2022-04-28T07:14:29Z

Okay the PR description has been updated to describe what this actually does and why 😅

justlevine added 2 commits April 26, 2022 21:03

feat: refactor get_allowed_{ post_types|taxonomies }()

3774a6b

chore: restore filter tests

6bcb72c

qlty-cloud-legacy bot reviewed Apr 26, 2022

View reviewed changes

justlevine added status: in review Awaiting review before merging or closing and removed scope: performance Enhancing speed and efficiency labels Apr 27, 2022

- make $allowed_taxonomies and $allowed_post_types protected properties

fe0a034

tests: clear WPGraphQL properties with clear_schema()

5dadda1

jasonbahl merged commit 4bc1cfa into wp-graphql:develop Apr 28, 2022

justlevine deleted the feat/refactor-allowed-post-types-and-taxonomies branch April 28, 2022 18:10

justlevine mentioned this pull request Apr 29, 2022

chore: refactor codebase using WPGraphQL::get_allowed_{type}( 'objects ') #2356

Merged

jasonbahl mentioned this pull request May 2, 2022

Release/v1.8.1 #2359

Merged

justlevine mentioned this pull request May 27, 2022

NodeByUri will not query WooCommerce products since v1.8.1 #2394

Closed

Conversation

justlevine commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes

Details

Does this close any currently open issues?

Any relevant logs, error output, GraphiQL screenshots, etc?

Any other comments?

Where has this been tested?

Uh oh!

qlty-cloud-legacy bot Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonbahl commented Apr 26, 2022

Uh oh!

justlevine commented Apr 26, 2022

Uh oh!

jasonbahl commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justlevine commented Apr 26, 2022

Uh oh!

justlevine commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonbahl commented Apr 27, 2022

Uh oh!

justlevine commented Apr 28, 2022

Uh oh!

justlevine commented Apr 28, 2022

Uh oh!

qlty-cloud-legacy bot commented Apr 28, 2022

Uh oh!

justlevine commented Apr 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

justlevine commented Apr 26, 2022 •

edited

Loading

coveralls commented Apr 26, 2022 •

edited

Loading

jasonbahl commented Apr 26, 2022 •

edited

Loading

justlevine commented Apr 27, 2022 •

edited

Loading