feat: Root Entry points for GuestCommenter #2577

moonmeister · 2022-10-13T01:29:07Z

What does this implement/fix? Explain your changes.

Makes comment authors who are not Users (aka GuestCommenters), available to be queried from the RootQuery.

GuestCommenter type has replaced CommentAuthor type in hopes of clarifying the difference between User and non-User comment authors. This also helps pave the way for commenter(s) entry points that can get Users or GuestCommenters.

Does this close any currently open issues?

closes #1757

Any other comments?

As discussed with @jasonbahl we'll need to provide a filter to revert the name change on this Type for those affected by this breaking change with the release.

I also changed how the ID filed got generated on this type, meaning it's prefixed with guest_commenter now instead of comment_author. This means if anyone has hard coded IDs they will change. Are we okay with this? Does this need to be reverted?

I have not written any tests, I can work on them if they're needed.

Where has this been tested?

Operating System: Linux

WordPress Version: 6.0.2

codeclimate · 2022-10-13T01:29:34Z

src/Type/ObjectType/RootQuery.php

+									break;
+							}
+
+							return ! empty( $id ) ? DataSource::resolve_guest_commenter( $id, $context ) : null;


Avoid too many return statements within this method.

codeclimate · 2022-10-13T01:29:35Z

src/Connection/GuestCommenters.php

+	 * @return array
+	 */
+	public static function get_connection_args() {
+		return [


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

coveralls · 2022-10-13T01:43:38Z

Coverage decreased (-0.5%) to 81.676% when pulling a6cf76c on feat/1757 into b1e1b3a on develop.

justlevine · 2022-10-13T11:24:15Z

As discussed with @jasonbahl we'll need to provide a filter to revert the name change on this Type for those affected by this breaking change with the release.

I also changed how the ID filed got generated on this type, meaning it's prefixed with guest_commenter now instead of comment_author. This means if anyone has hard coded IDs they will change. Are we okay with this? Does this need to be reverted?

@moonmeister it's not just the dataloader keys that have me concerned, but also the breaking PHP class names/files and the type name changes themselves. Are all/any of these essential just to expose commenters to the RootQuery?

I assume we want this feature merged before a major release, but without #2543 this will break pretty much every single site using comments (and likely a few plugins) overnight.

Even if users could fix their site after-the-fact with a filter (a lot of what's included cant be reverted with existing filters), in the past we've only relied on such a 'workaround' when the breaking changes were absolutely necessary. I worry this is a very bad precedent just to (albeit justifiably) improve the semantic naming of a GraphQL type that's been in use since 2019, unless it's otherwise essential to shipping these changes.

Thoughts?

jasonbahl · 2022-10-13T13:10:55Z

@moonmeister @justlevine I think there is a backward compatible way to do this. (sorry, @moonmeister. I should have been able to clearly think of this yesterday 🤦🏻‍♂️).

I think we can introduce a new GuestCommenter type to the Schema, but leave the CommentAuthor type for now.

I believe the only field that resolves to CommentAuthor currently is the comment.author.node field.

If we deprecate that field (but leave it), and introduce a new field, something like comment.authoredBy.node which then resolves to either User or GuestCommenter then we can do this in a back compatible way.

Existing users querying for comment.author can continue to resolve to either User or CommentAuthor and we can encourage folks to start querying for comment.authoredBy and expecting a User or GuestCommenter type.

Then, sometime in the future when we believe most users have likely switched to using the new comment.authoredBy field, we could remove the comment.author field and the CommentAuthor type, but still provide a snippet that makes it possible to bring it back for users that really need it.

moonmeister · 2022-10-13T13:52:54Z

All makes sense. The type chances are not necessary. I can change it to Jason's suggestion or just not worry about it and we can make the changes later during a major release.

justlevine · 2022-10-13T14:11:25Z

I like @jasonbahl 's suggestion too, that way we can continue to iterate on the new type without getting held up by back-compat.

Speaking in broad terms, I suggest we use the new/future names for the new PHP classes/types, but keep the existing dataloader key. As far as the existing PHP classes, instead of renaming them entirely, I think they should extend the new classes with a deprecation warning, so users can prepare for the eventual breaking change at their leisure.

codeclimate · 2022-10-13T15:45:28Z

src/Type/ObjectType/GuestCommenter.php

+	 *
+	 * @return void
+	 */
+	public static function register_type() {


Method register_type has 71 lines of code (exceeds 40 allowed). Consider refactoring.

codeclimate · 2022-10-13T15:45:29Z

src/Model/GuestCommenter.php

+					return current_user_can( 'moderate_comments' ) && ! empty( $this->data->comment_author_email ) ? $this->data->comment_author_email : null;
+				},
+				'url'        => function () {
+					return ! empty( $this->data->comment_author_url ) ? $this->data->comment_author_url : '';


Avoid too many return statements within this method.

codeclimate · 2022-10-13T15:45:29Z

src/Type/ObjectType/GuestCommenter.php

@@ -0,0 +1,104 @@
+<?php


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

codeclimate · 2022-10-13T15:45:29Z

src/Model/GuestCommenter.php

@@ -0,0 +1,70 @@
+<?php


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

…hed via node entry

codeclimate · 2022-10-13T17:40:16Z

src/Type/ObjectType/Comment.php

+						'connectionInterfaces' => [ 'CommentWriterConnection' ],
+						'description'          => __( 'The author of the comment', 'wp-graphql' ),
+						'oneToOne'             => true,
+						'resolve'              => function ( $comment, $args, AppContext $context, ResolveInfo $info ) {


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

codeclimate · 2022-10-13T17:40:16Z

src/Type/InterfaceType/CommentAuthorInterface.php

@@ -0,0 +1,73 @@
+<?php


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

codeclimate · 2022-10-13T17:40:16Z

src/Type/InterfaceType/CommentAuthorInterface.php

+	 *
+	 * @return void
+	 */
+	public static function register_type( TypeRegistry $type_registry ) {


Method register_type has 45 lines of code (exceeds 40 allowed). Consider refactoring.

moonmeister · 2022-10-13T17:51:10Z

Okay. SO new Types create, new interfaces and new authoredBy field has been created with those new types. All old types/interfaces/fields remain. though author has been marked as depreciated. That isn't working correctly due to connections not implementing deprecationReason correctly...Jason is working on a quick fix for that.

justlevine · 2022-10-13T18:32:00Z

@moonmeister would you mind fixing the phpcs / phpstan issues?

I have not written any tests, I can work on them if they're needed.

Looks like we're going to need them, especially around the id type resolution. I'm happy to lend a hand if you want 😎

codeclimate · 2022-10-13T18:51:36Z

src/Type/ObjectType/RootQuery.php

+									break;
+							}
+
+							return ! empty( $id ) ? DataSource::resolve_guest_commenter( $id ) : null;


Avoid too many return statements within this method.

moonmeister · 2022-10-13T19:20:53Z

@justlevine Help with Tests would be great. I'm working on that last phpcs comment warning. Needed to revisit the filters anyway.

codeclimate · 2022-10-13T19:53:47Z

Code Climate has analyzed commit a6cf76c and detected 14 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	4
Duplication	10

View more on Code Climate.

stale · 2023-02-02T18:49:39Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

moonmeister · 2023-02-02T19:01:33Z

Where are we at on this? Is it waiting for tests or something else?

justlevine · 2023-10-04T12:13:15Z

@moonmeister I just tried reviewing /rebasing this locally in order to start working on tests, but I'm not entirely sure which of these changes are intentional vs cosmetic, or what is actually is supposed to be happening since the force-push vs what was originally described in the PR description. Seems there's also still some breaking changes like the removal of DataSource::resolve_comment_author().

If you can provide some direction I'd love to give it another attempt 🤞

moonmeister

@justlevine Here are some comments. Hopefully, this is helpful. my memory ain't so good. No comments meant it seemed important. Maybe not correct...but 🤷🏻

moonmeister · 2023-10-04T12:23:08Z

bin/run-docker.sh

@@ -82,7 +82,7 @@ case "$subcommand" in
                WP_VERSION=${WP_VERSION} PHP_VERSION=${PHP_VERSION} docker compose up app
                    ;;
                t )
-                docker-compose run --rm \
+                docker compose run --rm \


Docker changed how compose is executed. I'm not sure if I was the only one having the issue, but if y'all aren't having issues this probably doesn't need to carry over.

moonmeister · 2023-10-04T12:23:58Z

src/AppContext.php

 			'nav_menu_item'       => new PostObjectLoader( $this ),
+			'plugin'              => new PluginLoader( $this ),


This change was cosmetic to keep with the alphabetical list.

moonmeister · 2023-10-04T12:27:05Z

src/Mutation/CommentDelete.php

@@ -105,8 +105,8 @@ public static function mutate_and_get_payload() {
 			} else {
 				// Get the current user id
 				$current_user_id = absint( get_current_user_id() );
-				// If the current user ID is the same as the comment author's ID, then the
-				// current user is the comment author and can delete the comment
+				// If the current user ID is the same as the commenter's ID, then the


Cosmetic. I think I was trying to keep consistent language through out the code base.

moonmeister · 2023-10-04T12:28:44Z

src/Type/InterfaceType/CommenterInterface.php

Looks cosmetic for consistent language

moonmeister · 2023-10-04T12:29:09Z

src/Type/ObjectType/Comment.php

@@ -26,10 +26,11 @@ public static function register_type() {
 				'model'       => CommentModel::class,
 				'interfaces'  => [ 'Node', 'DatabaseIdentifier' ],
 				'connections' => [
-					'author' => [
+					'author'     => [


moonmeister · 2023-10-04T12:29:19Z

src/Type/ObjectType/Comment.php

 						'toType'               => 'Commenter',
 						'connectionInterfaces' => [ 'CommenterConnection' ],
 						'description'          => __( 'The author of the comment', 'wp-graphql' ),
+						'deprecationReason'    => __( 'Deprecated in favor of `authoredBy`', 'wp-graphql' ), 


cosmetic for consistent language

moonmeister · 2023-10-04T12:30:48Z

src/Type/ObjectType/RootQuery.php

@@ -630,13 +630,46 @@ function ( $post ) use ( $allowed_post_types ) {

 						},
 					],
-					'viewer'      => [
+					'viewer'         => [


Tabs are all cosmetic

moonmeister · 2023-10-04T12:32:20Z

src/Type/WPConnectionType.php

looks cosmetic.

codeclimate bot reviewed Oct 13, 2022

View reviewed changes

feat: new GuestCommenter and entry points without changing CommentAuthor

aca653e

justlevine added Type: Enhancement New feature or request Compat: Breaking Change This is a breaking change to existing functionality ObjectType: User Component: Query labels Oct 13, 2022

moonmeister force-pushed the feat/1757 branch from 8020c21 to aca653e Compare October 13, 2022 15:44

codeclimate bot reviewed Oct 13, 2022

View reviewed changes

moonmeister added 3 commits October 13, 2022 16:11

fix: CommentAuthor type loading

3428adb

fix: unique gobal ID for GuestCommenter so they can be correctly fetc…

fe89f33

…hed via node entry

implement new authorBy and depreciate author

5fc4f15

codeclimate bot reviewed Oct 13, 2022

View reviewed changes

moonmeister removed the Compat: Breaking Change This is a breaking change to existing functionality label Oct 13, 2022

moonmeister added 2 commits October 13, 2022 18:41

fix: phpstan errors

b8a280a

fix: formatting and stan issues

aac0bf2

codeclimate bot reviewed Oct 13, 2022

View reviewed changes

feat: review filters, and confirm they are working as expected

a6cf76c

jasonbahl added the Needs: Tests Tests should be added to ensure this works as expected label Nov 4, 2022

stale bot added the Stale? May need to be revalidated due to prolonged inactivity label Feb 2, 2023

moonmeister removed the Stale? May need to be revalidated due to prolonged inactivity label Feb 2, 2023

moonmeister added the not stale Short-circuits stalebot. USE SPARINGLY label Feb 2, 2023

justlevine removed the not stale Short-circuits stalebot. USE SPARINGLY label Mar 3, 2023

moonmeister commented Oct 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Root Entry points for GuestCommenter #2577

feat: Root Entry points for GuestCommenter #2577

moonmeister commented Oct 13, 2022

codeclimate bot Oct 13, 2022

codeclimate bot Oct 13, 2022

coveralls commented Oct 13, 2022 •

edited

justlevine commented Oct 13, 2022 •

edited

jasonbahl commented Oct 13, 2022 •

edited

moonmeister commented Oct 13, 2022

justlevine commented Oct 13, 2022

codeclimate bot Oct 13, 2022

codeclimate bot Oct 13, 2022

codeclimate bot Oct 13, 2022

codeclimate bot Oct 13, 2022

codeclimate bot Oct 13, 2022

codeclimate bot Oct 13, 2022

codeclimate bot Oct 13, 2022

moonmeister commented Oct 13, 2022

justlevine commented Oct 13, 2022

codeclimate bot Oct 13, 2022

moonmeister commented Oct 13, 2022

codeclimate bot commented Oct 13, 2022

stale bot commented Feb 2, 2023

moonmeister commented Feb 2, 2023

justlevine commented Oct 4, 2023

moonmeister left a comment

moonmeister Oct 4, 2023

moonmeister Oct 4, 2023

moonmeister Oct 4, 2023

moonmeister Oct 4, 2023

moonmeister Oct 4, 2023

moonmeister Oct 4, 2023

moonmeister Oct 4, 2023

moonmeister Oct 4, 2023

		'nav_menu_item' => new PostObjectLoader( $this ),
		'plugin' => new PluginLoader( $this ),

feat: Root Entry points for GuestCommenter #2577

Are you sure you want to change the base?

feat: Root Entry points for GuestCommenter #2577

Conversation

moonmeister commented Oct 13, 2022

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Where has this been tested?

codeclimate bot Oct 13, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2022

Choose a reason for hiding this comment

coveralls commented Oct 13, 2022 • edited

justlevine commented Oct 13, 2022 • edited

jasonbahl commented Oct 13, 2022 • edited

moonmeister commented Oct 13, 2022

justlevine commented Oct 13, 2022

codeclimate bot Oct 13, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2022

Choose a reason for hiding this comment

codeclimate bot Oct 13, 2022

Choose a reason for hiding this comment

moonmeister commented Oct 13, 2022

justlevine commented Oct 13, 2022

codeclimate bot Oct 13, 2022

Choose a reason for hiding this comment

moonmeister commented Oct 13, 2022

codeclimate bot commented Oct 13, 2022

stale bot commented Feb 2, 2023

moonmeister commented Feb 2, 2023

justlevine commented Oct 4, 2023

moonmeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 13, 2022 •

edited

justlevine commented Oct 13, 2022 •

edited

jasonbahl commented Oct 13, 2022 •

edited