Repository navigation
Update/simple notifier to check - #171
amerk-sitecrafting wants to merge 7 commits into
Conversation
Deploying conifer-plugin with
|
| Latest commit: |
7543849
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f5ef0db0.conifer-plugin.pages.dev |
| Branch Preview URL: | https://update-simple-notifier-to-ch.conifer-plugin.pages.dev |
rhendrickson-sc
left a comment
rhendrickson-sc
left a comment
There was a problem hiding this comment.
It looks good! I just left some questions on it, but nothing that's a blocker.
| throw new \InvalidArgumentException(self::DEFAULT_EXCEPTION_MESSAGE); | ||
| } | ||
|
|
||
| if (gettype($to) === 'string') { |
There was a problem hiding this comment.
Question, why use gettype instead of is_string?
|
|
||
| // If we have reached this point, we have an array of some sort, so we need to validate all of the emails in the array. | ||
| // We will still check the type for sanity | ||
| if (gettype($toCopy) === 'array') { |
There was a problem hiding this comment.
Same question as above, why use gettype instead of is_array?
There was a problem hiding this comment.
You could also type $to as string|array to make it more consistent with the other updates in this file.
rhendrickson-sc
left a comment
rhendrickson-sc
left a comment
There was a problem hiding this comment.
Looks good! I'm just curious though if we want to make a v2 branch and merge this into that instead, so we can prep for a v2 without "dirty-ing" the main branch with v2 code before we officially launch it.
Way ahead of you! I have not updated the target for this branch (which would require remaking the entire PR), but I do have an |
|
This PR is approved and merged into the v2 branch, so I am closing this PR out |
2 participants
Ticket: #
247
249
Issue
Currently Conifer performs an "unsafe" redirect when viewing the
edit-comments.phppage in the admin center.Conifer's SimpleNotifier constructor does not validate that
$tois either a single email, comma separated list of emails, or array of emails.Solution
Update
wp_redirect->wp_safe_redirectAdd validation to the Simple Notifier constructor, and throw an \InvalidArgumentException if validation fails
Impact
If any site is extending the SimpleNotifier and attempt to pass in an invalid array, bad stuff will happen. But, more specifically it will throw an exception, which needs to be handled in that code.
Usage Changes
None
Testing
All tests pass after this update. The tests will also run in the pipeline.