Skip to content

Allow form rules to throw exceptions. #636

Closed
wants to merge 1 commit into from

6 participants

@realityking
Joomla! member

No description provided.

@jonnsl
jonnsl commented Dec 12, 2011

👍

@joomla-jenkins

Unit testing complete. There were 0 failures and 0 errors from 1886 tests and 11024 assertions.
Checkstyle analysis reported 208 warnings and 0 errors.

@eddieajau

I think we should go a bit further with this. validateRule should have @throws added to the docblock and allow for logic exceptions, such as changing:
return new JException(JText::sprintf('JLIB_FORM_VALIDATE_FIELD_RULE_MISSING', $type), -2, E_ERROR);
to
throw new LogicException(JText::sprintf('JLIB_FORM_VALIDATE_FIELD_RULE_MISSING', $type));

JRule should throw a LogicException on an invalid rule, and be marked to throw RuntimeExceptions otherwise for failed validation and change the return false to throwing a RuntimeException. Similarly change the derived rule classes to throw and exception rather than returning false.

What do you think?

@elinw
elinw commented Dec 14, 2011

That makes a lot of sense to me. I've written a bunch of rules and mixing the two together has always felt wrong to me.

@realityking
Joomla! member

I agree. But wouldn't the example Andrew gave better be a RuntimeException?

@eddieajau

Actually, that example is better as an InvalidArgumentException. For failing rules, I'd use a RuntimeException.

@realityking
Joomla! member

Ok I think I've done it as Andrew suggested. Note I haven't tested the code yet, but I'd be grateful if someone would check if my approach is sound.

@joomla-jenkins

Build triggered by changes to the head.

Test log missing. Tests failed to execute.
Checkstyle analysis reported 200 warnings and 0 errors.

@jonnsl jonnsl referenced this pull request Jan 5, 2012
Closed

Formvalidation Improvements #210

@realityking
Joomla! member

I realized I handled the legacy JExceptions in the wrong place but this should be good. I haven't tested this in the wild yet so please don't merge quite yet. I'd be glad thou if someone looked over the code.

Backward compatibility should be maintained with this unless one is directly using the (protected method) JForm::validateField() or is directly using JFormRuleEquals (which is hardly possible) so I think we're pretty safe.

@realityking realityking Allow form rules to throw exceptions.
Change JFormRuleEquals to throw exceptions.
cd293c7
@joomla-jenkins

Build triggered by changes to the head.

Unit testing complete. There were 0 failures and 1 errors from 1994 tests and 11158 assertions.
Checkstyle analysis reported 166 warnings and 0 errors.

@eddieajau

Closing. I'm pretty sure we settled on returning exceptions, not throwing them.

@eddieajau eddieajau closed this Apr 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.