New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: createComment
Mutation not checking for name and email
#1849
base: develop
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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. |
We can add a verbose description. Including a descriptive part that can switch based on current setting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(just to make the status change more visible)
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the conditional logic so it only applies to unauthenticated users. Also added tests for this and expanded the existing ones to cover unauthenticated cases.
Theres one case failing somewhere upstream that I've been unable to track 🤔 Could use some help.
|
||
$variables['email'] = null; | ||
|
||
$actual = $this->graphql( compact( 'query', 'variables' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this implement/fix? Explain your changes.
Changed the.commentOn
type 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. (commentOn
was 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