[#3941] - [#34001] Don't queue empty messages in JApplication message queue
- Closed
- 27 Jul 2014
- Medium
- Build: staging
- # 3941
- Diff
- itbra:patch-19
- Foreign ID 34001
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Take for instance JModelAdmin::save(). This method triggers two events while processing the store - event_before_save and event_after_save. The former event handlers' response is catched and evaluated. However, it is expected that the error that might have occured was thrown by the table object passed in. In fact the error should be expected from the dispatcher, since errors are either thrown directly by the plugin or set via $this->subject->setError by the event handler where subject is the JEventDispatcher object. So the error setting block for this example should rather be
// Trigger the onContentBeforeSave event.
$result = $dispatcher->trigger($this->event_before_save, array($this->option . '.' . $this->name, $table, $isNew));
if (in_array(false, $result, true))
{
$this->setError($dispatcher->getError());
return false;
}
This however might be one place where an empty entry is added to the queue. So this block should additionally check for the error's existance.
if (in_array(false, $result, true))
{
if ($dispatcher->getError())
{
$this->setError($dispatcher->getError());
}
}
I'm sure there are many more places where this kind of empty errors/messages are produced and added to the messageQueue. To circumvent this issue i added a check to drop empty entries as it makes no sense to render message containers with no content.
Please let me know, what you think about it!
Associated Bug tracker item
I actually prefer to use !$msg if we are sure the variable is set, which is the case here. If the variable may not be set empty($msg) makes more sense because it has a built-in isset check and doesn't throw a warning.
Both ways will return true if the variable is either null, 0, false or an empty string.
@Bakual
but please consider the linked artikel ... especially the evaluation of 0 vs '0'. !$msg is even not reliable.
Lets face the problem again. The related method expects two parameters - $msg and $type - both of type string. While $type falls back to value 'message' when no value was passed in $msg won't fall back to a default value but rather cause an error to be thrown because of the missing parameter. Thus, PHP already checked that a value for $msg was passed in.
Since this PR is about catching empty strings (length = 0) we should not care about senseless values. If somebody wants to throw '0', we shouldn't care.
What remains to check is that the passed in string is longer than 0 characters. And the most reliable way for that is strlen(). I would even go a little futher and check this way:
if (!$msg || !strlen($msg)) // catch $msg == null || $msg == ''
Sorry but I don't agree. We should do something like:
$msg = trim($msg);
if (empty($msg))
{
return;
}
That way we will remove empty spaces at the beginning/end of the $msg which would be useful also to render it properly.
@Bakual is not the first time you suggest to use !$msg instead of empty($msg) and that could lead to undesired errors as you know if the var is not set. We need to search good global practices and that's to always use empty($msg). I think we should spread that.
@phproberto The real problem here with empty($msg) and !$msg isn't spaces. It's if you pass in a string containing only a zero:
$msg = '0';
if (empty($msg))
{
// This code will show
}
if (!strlen($msg))
{
// This code will not show
}
Because PHP is a loosely typed language it will convert that '0' string to an integer and see it as a non-value. But it would actually be wrong behavior.
One could argue that we never see an error message containing only that zero in a string. But we don't know for certain and strlen would actually threat it correct.
As for empty() vs !. Imho the useage depends if the variable is set within the control of the class/method or if it's out of control. If it's certain that it is set, we don't need empty. If it may happen that the variable isn't set, then empty is the way to go.
$msg = 'value';
if (!empty($msg))
{
// This just produces more complex code without any added value
}
if ($msg)
{
// This is perfectly fine
}
if ($other)
{
// This is not good code because we didn't define $other ourself
}
if (!empty($other))
{
// This is perfect here
}
+1 from me too.
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-07-27 17:45:08 |
Any reason for using strlen instead of empty ?