Notepad dashboard widget method changed to POST#469
Notepad dashboard widget method changed to POST#469rinatkhaziev merged 2 commits intoAutomattic:masterfrom TheCrowned:fix/466-notepad-widget-post-form
Conversation
sboisvert
left a comment
sboisvert
left a comment
There was a problem hiding this comment.
In this case when I first saw the change from GET to POST I wondered why that would work (since REQUEST is frowned upon) https://vip.wordpress.com/documentation/code-review-what-we-look-for/#using-_request
In the PR we should mention the answer to any expected questions or things that feel strange. Changing a form without changing the way superglobals are accessed would fall under that. We try to make it as easy and clear to review the why of the change (and this includes "why" it works)
| wp_insert_post( $new_note ); | ||
| } | ||
|
|
||
| wp_safe_redirect( wp_get_referer() ); |
There was a problem hiding this comment.
Is this change expected to be part of this PR?
There was a problem hiding this comment.
Yes. When data was sent over GET, that line had the use of hiding the querystring. The user was immediately redirected to the page it created the note from, so that he would not see the GET parameters. However, wp_get_referer returns false after a POST request, so I had to be taken away, together with the following exit.
| $current_id = ( ! empty( $posts[0]->ID ) ) ? $posts[0]->ID : 0; | ||
| $current_post = ( ! empty( $posts[0] ) ) ? $posts[0] : false; | ||
|
|
||
There was a problem hiding this comment.
The whitespace should be part of a separate commit as per https://vip.wordpress.com/documentation/creating-good-changesets/
There was a problem hiding this comment.
All right - should I create a new commit restoring that line to what it was before? :)
There was a problem hiding this comment.
All right - should I create a new commit restoring that line to what it was before? :)
Yup!
| } | ||
|
|
||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
This change, while also fine, should probably not be part of this commit :)
There was a problem hiding this comment.
All right - should I create a new commit restoring that line to what it was before? :)
(Here, I think that the issue is that my editor adds a newline at the end of the file after saving - will see how to get rid of it)
There was a problem hiding this comment.
All right - should I create a new commit restoring that line to what it was before? :)
That would be optimal :)
|
About (not) using $_REQUEST, should I create a new commit in which $_POST is used instead? I did not change them because I supposed that, since they were like that before, I should have left them alone :) Nod (for the future) about explaining why the changes work and answering to questions you may have when reviewing! |
This should be a separate PR. Leaving it as is in this PR is the right call. Just having information on "why" it works is good. |
|
I have removed the whitespace changes :) I will create another branch + PR to change $_REQUEST usage in $_POST usage in this file. |
|
cc @rinatkhaziev to decide in what release to stick this in |
3 participants
Form method changed to POST and function
handle_notepad_update()updated accordingly.Fixes #466.