recaptcha inside a fieldset doesn't validate and allows any submission

[djay]

No description provided.

[coveralls]

Coverage increased (+0.4%) to 92.404% when pulling d0efb97 on djay/fieldset_recaptcha into 459127c on master.

[djay]

@fredvd I'm not sure but my PR above seems like the wrong solution to me. It seems to me that if most of the fields are IFieldExtender fields then they should be marked with that persistently or at least before the lookups for validators happen.
Or the FieldExtenderValidator should be able to call any other adapters of that particular field type. However I wasn't able to work out how to call the "next most specific adapter".

My solution only works for the specific case of Recaptcha and if any other field type is used in easyform that has its own validator etc, it will also not work.

[fredvd]

@djay

I'm not sure but this seems like the wrong solution to me.

What is "this" referring to, your solution here?

If I have to make a guess it's maybe the changes Maurits and I have done to enable the extended validators working again in fields outside the 'default' Form but in Fieldsets?

I don't have an opinion (out of my league) if these ExtendedFieldValidators are correctly implemented, we only found out that the adapter lookup skipped all fields in fieldsets because of a different interface so that part I fixed. @mauritsvanrees , can you please comment on this?

Related issues #172 and later more generic for current Plone 5.X #177

[djay]

@fredvd @mauritsvanrees I guess what I'm saying is that a nicer solution would be if we could do something like this

@implementer(IValidator)
@adapter(IEasyForm, Interface, IEasyFormForm, IField, Interface)
class FieldExtenderValidator(z3c_validator.SimpleFieldValidator):

    """ z3c.form validator class for easyform fields in the default fieldset"""

    def validate(self, value):
        """ Validate field by TValidator """
        super(FieldExtenderValidator, self).validate(value)

        ...

        # IFieldExtender validators didn't work. See if there is a more generic adapter
        genericcontext = remove_interface(self.context, IEasyForm)
        genericform = remove_interface(self.form, IEasyFormForm)
        adapter = getAdapter((genericcontext, self.request, genericform, self.field, self.widget), IValidator)
        adapter(self.context, self.request, self.form, self.field, self.widget).validate(value)

[djay]

@mauritsvanrees ok. I have a version of the my idea above which passes the tests. What do you think?

@implementer(IValidator)
@adapter(IEasyForm, Interface, IEasyFormForm, IField, Interface)
class FieldExtenderValidator(z3c_validator.SimpleFieldValidator):

    """ z3c.form validator class for easyform fields in the default fieldset"""

    def validate(self, value):
        """ Validate field by TValidator """
        super(FieldExtenderValidator, self).validate(value)
        efield = IFieldExtender(self.field)
        validators = getattr(efield, "validators", [])
        if validators:
            for validator in validators:
                vmethod = queryUtility(IFieldValidator, name=validator)
                if not vmethod:
                    continue
                res = vmethod(value)
                if res:
                    raise Invalid(res)
        TValidator = getattr(efield, "TValidator", None)
        if TValidator:
            try:
                cerr = get_expression(self.context, TValidator, value=value)
            except Exception as e:
                raise Invalid(e)
            if cerr:
                raise Invalid(cerr)

        # IFieldExtender validators didn't work. See if there is a more generic adapter
        @implementer(IForm)
        class Dummy():
            # TODO: this should wrap self.view
            pass
        validator = queryMultiAdapter((self.context, self.request, Dummy(), self.field, self.widget), IValidator)
        if validator is not None:
            validator.validate(value)

and to make it work I still need to register the RecaptureValidator as plone.formwidget.recaptcha doesn't for some reason I don't understand.

        <adapter
            for="collective.easyform.interfaces.IEasyForm
                 *
                 *
                 collective.easyform.interfaces.IReCaptcha
                 plone.formwidget.recaptcha.interfaces.IReCaptchaWidget"
            provides="z3c.form.interfaces.IValidator"
            factory="plone.formwidget.recaptcha.ReCaptchaValidator"
            />

[djay]

@fredvd @mauritsvanrees I've committed the alternative solution. I think it's better but please review if there is something you see wrong with this approach

[mauritsvanrees]

It seems nicer to keep the SimpleFieldValidator as base. You can at least skip the __init__ then, because it is the same.

[djay]

I got rid of it to reduce the confusion as to why super wasn't being called.

[mauritsvanrees]

Seems to work, but I have some comments inline.

[mauritsvanrees]

This could use a comment about what does does and why it works.
Is this to make a fieldset and form behave the same?

[djay]

The original problem is that this validator got in the way of the recaptcha validator so it never got called as the registration was more specifc. Instead of just re-registering the recaptcha validator I figured this was a more general solution as there could be other validator adapters being blocked in the future.

[mauritsvanrees]

I think you still want the super call. The base class seems to do some useful things.
I fear that we could break other use cases when we do not use the base class.

[djay]

The StrictSimpleFieldValidator is by default registered for all fields so still gets called in the multi adapter call. This however allows someone else to prevent that happening if they want. Also it means this code isn't run twice. BTW. The tests fail without StrictSimpleFieldValidator validator being called.
I'll remove StrictSimpleFieldValidator as a subclass and put in a comment to make it more clear

[mauritsvanrees]

BTW, I first thought it did not work at all, but that was because I tried a recaptcha field in the base form and in a fieldset at the same time. Then I always get a validation error, even on master. Apparently you should use only one recaptcha field per form. Makes sense.

[djay]

BTW, I first thought it did not work at all, but that was because I tried a recaptcha field in the base form and in a fieldset at the same time. Then I always get a validation error, even on master. Apparently you should use only one recaptcha field per form. Makes sense.

Sounds like a bug in formwidget.recaptcha. It should handle two fields in the same form even if it makes no sense.

[mauritsvanrees]

LGTM now.
It doesn't get any easier to understand with this, but that is what you get when you use z3c.form as base and wrap something around it, and call it "easy"form... ;-)
Fine to merge, unless someone voices an objection soon.

[djay]

@mauritsvanrees EasyForm is overriding the validators for all fields it uses. Thats fine except that the assumption was made that we could use SimpleFieldValidator.validate (super) to replace all possible registered IValidator adaptors for all fields which isn't correct. EasyForm is trying to add validations not replace them.
Instead I'm doing a kind of "super" call for an adapter. Something I think ZCA should really support natively. A "find me the next most specific adapter" call.