WICKET-3899 testing Form#wantSubmitOnParentFormSubmit in the form #1033
Conversation
- removing traversal signaling dead code - fix form validator when dependent components aren't enabled or form participants - internalMarkFormComponentsValid in postorder - updateNestedFormComponentModels in postorder - internalOnValidateModelObjects in porstorder
pedrosans
force-pushed
the
WICKET-3899-want-submit-test
branch
from
c8ae228 to
a89ec0d
Compare
| @@ -757,7 +741,7 @@ public final boolean isSubmitted() | |||
| @Override | |||
| public final void onRequest() | |||
| { | |||
| onFormSubmitted(null); | |||
| onFormSubmitted(findSubmitter()); | |||
There was a problem hiding this comment.
@martin-g This seems a bit problematic. We have failing Tests after upgrading from 10.2. to 10.4 and the reason seems to be that while the submitter is calculated the same as before, there is no check for visibility anymore (like in Line 810ff) and the code does run straight to Line 839 instead of Line 810 because the submitter is not null anymore when checking in line 801
There was a problem hiding this comment.
This might only influence Tests because in reality the user could not click a button that is not visible
There was a problem hiding this comment.
The best would be to send a PR with a fix and a test!
| @@ -2059,13 +2020,13 @@ protected final void validateFormValidator(final IFormValidator validator) | |||
| } | |||
| // check if the dependent component is visible and is attached to | |||
| // the page | |||
| else if (!isFormComponentVisibleInPage(dependent)) | |||
| else if (!dependent.isVisibleInHierarchy() || !dependent.isEnabledInHierarchy() || !dependent.isFormParticipant()) | |||
There was a problem hiding this comment.
Hi @pedrosans, addition of !dependent.isEnabledInHierarchy() makes our migration to Wicket 10 problematic. We have many FormValidators with multiple fields and some are conditionally enabled/disabled, but their values still participate in validation logic.
Following this new logic we'd have to modify getDependentFormComponents and check for each field being enabled/disabled.
I don't see any particular benefit by adding isEnabledInHierarchy condition.
What was the reason behind this change? Because we'd like this to work without updateing every validator and making sure it follows this isEnabledInHierarchy logic.
There was a problem hiding this comment.
I can open a PR with a fix/tests if this gets a 👍
There was a problem hiding this comment.
Hi @mira-silhavy, the idea is to prevent a formvalidator to pass or not based on "dirty" data. The disabled component it depends upon was not processed during the request validating the formvalidator.
There was a problem hiding this comment.
Was this causing problems any problems and what do you mean by "dirty" data?
If component is disabled it's still visible on page, but it's not editable so I wouldn't call it that is has "dirty" data if that's what you mean. In our use case the component can be enabled/disabled in certain scenarios, but we still read the its value during validation. Now I'll need to exclude it from FormValidator dependent components to make sure the validation actually takes place.
There was a problem hiding this comment.
what do you mean by "dirty" data?
Any value returned by dependent formComponent#getInput() or #getConvertedInput() will have not been validated during the form processing before the formvalidator check. wicket-core for instance has formvalidators accessing dependent form component values without checking if they are valid or not, because it should be safe to assume they are valid inside IFormValidator#validate execution.
There was a problem hiding this comment.
It would be nice to hear from other devs here, imo to allow disabled dependent form components inside a formvalidator shouldn't be the default behavior. But in order to make the form backward compatible with apps being migrated to wicket 9.x and 10.x, a flag can be provided/proposed and tested inside the if statement you are proposing to patch.
There was a problem hiding this comment.
IMHO an IFormValidator should be executed if at least one of the dependent components is enabled. It should not be necessary for all components to be enabled. We've been bitten by this problem as well.
Suppose you have a form where you can enter a certain property in 2 ways. For example, a form where you can upload a certificate with private key, either in PKCS#12 format (1 single file) or in PEM format (2 files). You can toggle these fields via radio buttons, so only one of the 2 formats is visible at a given moment. In this form, there's also a field for the domain name, which needs to match with the provided certificate. The easiest way would be to have an IFormValidator with all these fields as dependent components. However, this does not work as either the PKCS#12 or the PEM fields are not visible, so your validator is never executed.
A problem I do see is that disabled components do not participate in form processing, so their converted input is always null. Your validator needs to be able to deal with this properly. In the example above, this is not an issue, because the validator simply checks the state of the radio buttons before reading either the PKCS#12 or PEM fields.
There was a problem hiding this comment.
@pedrosans @papegaaij shall I open a PR that uses Wicket 9 behavior in this situation? Or with flag?
There was a problem hiding this comment.
@mira-silhavy Yes, that would be great and also make it easier to discuss this change.
There was a problem hiding this comment.
@papegaaij I opened a PR here #1186, currently only removes the isEnabledInHierarchy
visiting