[jira] Commented: (WICKET-839) FormComponent#checkRequired should implement only the input check

["Eelco Hillenius (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/WICKET-839?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12519321 ] 

Eelco Hillenius commented on WICKET-839:
----------------------------------------

> left.setrequired() and right.setrequired() shouldve been called in the constructor. 

No. They are nested in another component (the formcomponentpanel), which may or may not be required. If that component is not required, those checks should not be executed. If it is, how are you gonna do those checks? You can only do that in the way I showed. You can't override setRequired as it is final, and that would have been an ugly hack to start with.

> left.checkrequried(), right.checkrequired(), formcomponentpanel.checkrequired() 

No, the whole point is that FormComponentPanel in this case should decide whether to execute the required check on is kids directly. So left.checkRequired and right.checkRequired should not be called by default (unless the panel already decided that they required in any case) but instead the panel should decide.

> so in this case formcomponentpanel.checkrequired() should simply return true - in fact i think that should just be the default implementation of checkrequried for fcp, no need to make it abstract.

No. I got here because of a bug report on the list about DateField not doing a proper required check. This was because such a component does not receive direct input, but instead should delegate that check to nested components that do (the date text field in this case). But it should only do that when it's required flag is set. 

Now this implementation is this:

	public boolean checkRequired()
	{
		return dateField.checkRequired();
	}

without this change it would be:

	public boolean checkRequired()
	{
		if (isRequired())
		{
			dateField.setRequired(true);
			return dateField.checkRequired();
		}
		return true;
	}

or we would have to open up setRequired so that it would set the required bit on the text field. Both not very good solutions (the latter is not good as it is more limited, and people will probably often forget about it).

Finally, I want to force people to implement checkRequired as it is something we just cannot provide a meaningful default implementation for. It is important they think about the implications of the required flag on their custom form and provide an appropriate check.

Finally finally, I was just surprised to see the isRequired check in this method rather then in validateRequired where it is much better placed. It makes sense to have a method that only does the check whether it has input.

> FormComponent#checkRequired should implement only the input check
> -----------------------------------------------------------------
>
>                 Key: WICKET-839
>                 URL: https://issues.apache.org/jira/browse/WICKET-839
>             Project: Wicket
>          Issue Type: Bug
>    Affects Versions: 1.3.0-beta2
>            Reporter: Eelco Hillenius
>            Assignee: Eelco Hillenius
>             Fix For: 1.3.0-beta3
>
>
> Currently, checkRequired starts with calling isRequired(), so that method is actually doing two things where it should do one. The check should be done before the method is called in validateRequired instead
> Currently the multiply example in wicket-examples/forminput would have to have the check implemented like this:
> 	public boolean checkRequired()
> 	{
> 		return isRequired() ? left.setRequired(true).checkRequired() &&
> 				right.setRequired(true).checkRequired() : true;
> 	}
> which is a pretty ugly hack, while after the change, it can be coded like this:
> 	public boolean checkRequired()
> 	{
> 		return left.checkRequired() && right.checkRequired();
> 	}

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.