? Success
Related to # 4653
Referenced as Related to: # 4653

User tests: Successful: Unsuccessful:

avatar tranduyhung
tranduyhung
13 Oct 2014

While building a custom JFormRule to validate the options in my custom component's configuration, I realize that the rule's error message is not displayed after redirecting back to the configuration form.

class JFormRuleFoo extends JFormRule
{
    public function test(SimpleXMLElement $element, $value, $group = null, JRegistry $input = null, JForm $form = null)
{
        // Validation fails, enqueue error message, it must be displayed when we are taken back to the form.
        JFactory::getApplication()->enqueueMessage(JText::_('Validation fails'), 'warning');
        return false;
}

}

But if we try to go to somewhere else

JFactory::getApplication()->enqueueMessage(JText::_('test'), 'warning');
JFactory::getApplication()->redirect('index.php');

Then the message is displayed as usual.

The problem is in https://github.com/joomla/joomla-cms/blob/staging/components/com_config/model/form.php#L313

It should be

foreach ($form->getErrors() as $message)
{
    if ($message instanceof Exception)
    {
        JFactory::getApplication()->enqueueMessage($message->getMessage(), 'error');
    }
    else
    {
        JFactory::getApplication()->enqueueMessage($message, 'error');
    }
}
avatar tranduyhung tranduyhung - open - 13 Oct 2014
avatar jissues-bot jissues-bot - change - 13 Oct 2014
Labels Added: ?
avatar brianteeman brianteeman - change - 14 Oct 2014
Category Libraries
avatar brianteeman brianteeman - change - 14 Oct 2014
The description was changed
Rel_Number 4653
Relation Type Related to
avatar joomdonation
joomdonation - comment - 14 Oct 2014

+1 from me for this PR. Thanks for fixing the issue. Just some comments:

  1. I think JFactory::getApplication()->enqueueMessage(JText::_('Validation fails'), 'warning'); is not the correct way of returning error message for the custom field. From my quick look at JForm class, I think you can return an Exception or add the error message to "message" attribute of of the form field definition in xml file) if you want a custom message. Otherwise, leave JForm to build the default error message for the rule.

  2. From my quick look at the JForm validate method, it seems $form->getErrors() always return an array of Exception objects, so I wonder if we need else in above code ?

  3. It seems validate method of JForm only return true or false (and it store all errors in errors property), so I am not sure we still need this block of code: https://github.com/joomla/joomla-cms/blob/staging/components/com_config/model/form.php#L300

avatar tranduyhung
tranduyhung - comment - 14 Oct 2014
  1. Use Exception is not a good idea, when an exception is thrown, the only way you can go back to the form is hitting "Back" button on the browser. Exception makes users think they just break something terribly. If you use "message" attribute, the bug still happens, because the problem here is the way we display the message. For some cases we need to show many messages based many checks and validations, and you need to give users instructions to solve the errors, enqueue the instruction messages in the rule is still the way to go.

  2. Just checked and that's right. I will remove the else block. But there will be many other places we need to remove the else block too, I see this if-else block very often, for example somewhere in FOF framework if my memory is good, and that made me thought the error could be a single text in some cases.

  3. Haven't had a chance dig into it yet, but because it is not related to this issue, you may want to create a new issue for it if you think something needs to be changed.

avatar joomdonation
joomdonation - comment - 14 Oct 2014

Use Exception is not a good idea, when an exception is thrown, the only way you can go back to the form is hitting "Back" button on the browser. Exception makes users think they just break something terribly. If you use "message" attribute, the bug still happens, because the problem here is the way we display the message. For some cases we need to show many messages based many checks and validations, and you need to give users instructions to solve the errors, enqueue the instruction messages in the rule is still the way to go.

You mis-understand me. It is only a bad idea if you throw the Exception, if you return the Exception, it will work well, the error still being queued and displayed (of course with your PR merged). So instead of writing :

        JFactory::getApplication()->enqueueMessage(JText::_('Validation fails'), 'warning');
        return false;

I believe you can write:

   return new Exception(JText::_('Validation failes'));

For using "message" attribute, if your PR is merged, it should work well, too (I haven't tested, just see it by reading the code of JForm class).

Haven't had a chance dig into it yet, but because it is not related to this issue, you may want to create a new issue for it if you think something needs to be changed.

I am OK to leave that code there (it is safer than removing it)

avatar tranduyhung
tranduyhung - comment - 14 Oct 2014

I see, I already used return new Exception(JText::_('Validation failes')); but it wouldn't work, because the exception's message is only retrieved in the foreach loop that I have changed in my pull request.

avatar joomdonation
joomdonation - comment - 14 Oct 2014

Yeah ! I wrote my comments with the assumption that your PR is merged.

avatar nicksavov nicksavov - change - 16 Oct 2014
Labels Added: ?
avatar brianteeman brianteeman - change - 1 Jan 2015
Labels Removed: ?
avatar ysavran
ysavran - comment - 21 Aug 2015

Issue is fixed.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4654.

avatar ysavran ysavran - test_item - 21 Aug 2015 - Tested successfully
avatar roland-d
roland-d - comment - 21 Aug 2015

@tranduyhung Can you fix the merge conflicts and check if the issue still exist? Thank you.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4654.

avatar tranduyhung
tranduyhung - comment - 21 Aug 2015

I fixed the merge conflict. Thank you @roland-d for your letting me know.
Tested again and the problem was still fixed.

avatar zero-24
zero-24 - comment - 23 Aug 2015

@joomdonation can you retest the last changes? Than we can RTC ;)


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4654.

avatar joomdonation
joomdonation - comment - 24 Aug 2015

Hi

Infact, this PR could be closed. The issue is not valid anymore because it was fixed with this PR #6439

avatar roland-d
roland-d - comment - 24 Aug 2015

Closing as requested. Thanks @joomdonation

avatar roland-d roland-d - change - 24 Aug 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-08-24 06:42:39
Closed_By roland-d
avatar roland-d roland-d - close - 24 Aug 2015

Add a Comment

Login with GitHub to post a comment