-
Notifications
You must be signed in to change notification settings - Fork 429
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
Two very minor changes #361
Conversation
jrfnl
commented
Apr 27, 2015
- Don't pass parameter to function which doesn't have parameters
- Only print (and escape) the message when there actually is one
@@ -439,7 +439,7 @@ public function install_plugins_page() { | |||
<?php $plugin_table->prepare_items(); ?> | |||
|
|||
<?php | |||
if ( isset( $this->message ) ) { | |||
if ( isset( $this->message ) && is_string( $this->message ) && '' !== $this->message ) { |
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.
Per the other discussion, I think the last part here should use empty()
.
Alternatively, as things stand, the second part is redundant, since the last part is already checking the type is a string.
So, either:
if ( isset( $this->message ) && is_string( $this->message ) && ! empty( $this->message ) ) {
or
if ( isset( $this->message ) && '' !== ( $this->message ) ) {
should be sufficient.
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.
I'll change it to your first option - the second is definitely incorrect as that would pass new stdClass
, (int) 5
etc
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.
#wceu public is a bit too diverse for my hard-core PHP defensive coding talk (The big "Why equal doesn't equal" Quiz), but maybe we should do with a select group in a hallway track or something....
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.
Oh and I've update the PR for empty
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.
I'll change it to your first option - the second is definitely incorrect as that would pass new stdClass, (int) 5 etc
Yes, you're right.
#wceu public is a bit too diverse for my hard-core PHP defensive coding talk (The big "Why equal doesn't equal" Quiz), but maybe we should do with a select group in a hallway track or something....
I've seen a video of it :-)
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.
Interesting... I always ask them to cut the actual quiz out if they video it - which one did you see ? There are actually some five different versions of the quiz (different sets of questions).