Fix error message is not displayed in com_config (#4653)

[tranduyhung]

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');
    }
}

[joomdonation]

+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

[tranduyhung]

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.

[joomdonation]

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)

[tranduyhung]

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.

[joomdonation]

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

[ysavran]

Issue is fixed.

---

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

[roland-d]

@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.}

[tranduyhung]

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

[zero-24]

@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.}

[joomdonation]

Hi

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

[roland-d]

Closing as requested. Thanks @joomdonation