-
Notifications
You must be signed in to change notification settings - Fork 50
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
Issue/44 friendlier zone lock #74
base: develop
Are you sure you want to change the base?
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens here if they pass a zone_id that doesn't exist?
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.
@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_lock
url param
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Zoninator #44
error-zone-lock-max
message to reflect the updated redirect url.