-
Notifications
You must be signed in to change notification settings - Fork 8
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: remove the exclusion of NonceVerification in phpcs and fix the issues #67
Conversation
Thanks for the pull request, @MaferMazu! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
f7c9558
to
ae1bd3c
Compare
ae1bd3c
to
42d04ff
Compare
42d04ff
to
bd30514
Compare
This is ready for review, @feanil and @felipemontoya. This PR solves the first issue from Wordpress.org related to the PHPCS and WordPress Standards, and the error logs they sent us are in the attached file. If you have any questions or feedback, please don't hesitate to contact me. |
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.
The PR seems very reasonable to me. Adding nonces for security of the form submissions looks good.
I don't have an environment to test this so we depend on the testing that you are doing locally or in the remote staging. How did that go?
@felipemontoya, I tested locally and in our stage. You can test it by entering the stage with these credentials and checking if the forms correctly store the info you change (If the nonce doesn't work, they don't store the info), and check if you are receiving the nonce variable (In inspect searching the nonce variable related to that form).
|
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.
Sorry to take so long. I tested the code in the staging server and it's working correctly.
We can go ahead with merging this to move forward with the wordpress plugin index publication
@MaferMazu I came back to this and notice my review is not approving. Maybe we need a green check from @feanil to be able to merge. |
@felipemontoya thanks, just approved but also maybe it makes sense for you to be a CC on this repo? do you want to post for some rights expansion? |
@MaferMazu 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR:
save_post_shop_order
towoocommerce_update_order
.Testing instructions
The expected result is that the forms are saved correctly (if the nonce_verification fails, the save or update won't store the new information).
Credentials for the stage to test: https://share.1password.com/s#Pu505pb8w5wyFnHuHhvHBg4K1B5VIHk69kK0O-mCy9s
Additional information
save_post_shop_order
towoocommerce_update_order
because the first one was preventing me from saving the order extra information.More references related:
Checklist for Merge