fix: createComment Mutation not checking for name and email
#1849
Conversation
src/Mutation/CommentCreate.php
Outdated
| 'description' => __( 'The ID of the post object the comment belongs to.', 'wp-graphql' ), | ||
| ], | ||
| 'author' => [ | ||
| 'type' => 'String', | ||
| 'type' => boolval( get_option( 'require_name_email' ) ) ? [ 'non_null' => 'String' ] : 'String', |
There was a problem hiding this comment.
If an option can make this nullable, we should probably leave it nullable in the Schema.
Otherwise setting/unsetting this option in the admin would be a breaking change for clients.
There was a problem hiding this comment.
That doesn't make sense to me, but I may be missing something...
When this option is enabled on the server any mutations not passing this data will start breaking either way, right?. The only difference is whether it's a schema error or a server error (not sure if those are the correct terms 🤷 ). If this remains, then when this option is enabled the schema will correctly indicate to devs what the server requires, which seems extremely helpful.
If this option is disabled, It can still accept name/email but it isn't required...i don't see what would break...am I missing something?
There was a problem hiding this comment.
@moonmeister the difference as I see it is that a schema change can break the frontend (possibly even preventing it from compiling) whereas if it fails validation on the server, an existing frontend will just handle it like any other error response.
There was a problem hiding this comment.
Personally if someone flips that switch and causes something to break I'd rather my build break than finding out in production.
There was a problem hiding this comment.
Okay, We don't want to change this. Keep in nullable
|
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. |
justlevine
added
the
Compat: Breaking Change
|
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. |
stale
bot
added
the
Stale?
moonmeister
added
not stale
|
We can add a verbose description. Including a descriptive part that can switch based on current setting. |
justlevine
added
Type: Enhancement
moonmeister
changed the title
createComment Mutation not checking for name and email
|
@justlevine Got this updated. Looks like I might need to format. Otherwise, we should probably add tests. I can try, just point me in the right direction. |
Co-authored-by: Dovid Levine <justlevine@gmail.com>
|
Code Climate has analyzed commit befd0dc and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
|
||
| $variables['email'] = null; | ||
|
|
||
| $actual = $this->graphql( compact( 'query', 'variables' ) ); |
There was a problem hiding this comment.
This query is throwing the following GraphQL error, but i've been unable to trace what is requesting that array key, or why it would be a fatal error instead of a PHP warning:
(removing comment from the query entirely passes, so I'm guessing it's something on the CommentModel ).
What does this implement/fix? Explain your changes.
Changed the.commentOntype to by ID as it seems it should be. Given this is potentially a breaking change I can revert it if neededModified the Schema to correctly reflect what fields are required. (commentOnwas already required in the code but was throwing a error when not provided. the Schema now reflects that.and reflect that change in the schema as appropriate.(previously choosing the option to require name/email didn't affect the ability to submit a comment without name/email).Does this close any currently open issues?
1 partially addresses #997Any other comments?
I'll try my hand at tests here soon. I'll also see if I should modify related mutations to match.
Where has this been tested?
Operating System: …
Linux
WordPress Version: …
6.3