Issue/44 friendlier zone lock#74
Conversation
Zoninator#44 Updated the redirect url to use when the zone lock expires.
Zoninator Automattic#44 Changed the `error-zone-lock-max` message to reflect the updated redirect url.
Zoninator Automattic#44 Added an admin notice when a user's zone lock expires and they get redirected to the top level zones page.
sboisvert
left a comment
sboisvert
left a comment
There was a problem hiding this comment.
Looks good. One small question
zoninator.php
Outdated
| $zone = $this->get_zone( intval( $_GET['zone_lock'] ) ); | ||
| ?> | ||
| <div class="notice notice-warning is-dismissible"> | ||
| <p><?php echo sprintf( $this->_get_message('error-zone-lock-redirect'), esc_html( $zone->name ) ); ?></p> |
There was a problem hiding this comment.
What happens here if they pass a zone_id that doesn't exist?
There was a problem hiding this comment.
@sboisvert Ah, yeah, I'll need to only display the notice if $zone is not false.
Do you think there should be a different notice if $_GET['zone_lock'] isn't a valid Zone ID or ignore it? I think that should only be able to happen if:
- The zone is deleted as the user who was editing the zone is being redirected
- Someone purposefully puts an invalid Zone ID in the
zone_lockurl param
There was a problem hiding this comment.
Nod I agree on when it should only happen. I suspect that this will throw a PHP Warning if that's the case since we're using the return of $this->get_zone() without checking if it's actually what we expect as per https://vip.wordpress.com/documentation/code-review-what-we-look-for/#not-checking-return-values :)
There was a problem hiding this comment.
Correct. It does throw a warning there if you pass an invalid Zone ID into $_GET['zone_lock'].
Thoughts on adding a different notice if the Zone ID is invalid?
Zoninator Automattic#44 We needed to check and make sure that `$zone` is not `false` coming back from `$this->get_zone()` otherwise a PHP Warning is thrown.
Zoninator Automattic#44 Fixed a whitespace issue in new code.
3 participants
Zoninator #44
error-zone-lock-maxmessage to reflect the updated redirect url.