symbiote / silverstripe-seed

Symbiote Seed. A set of modules that form a solid foundation for SilverStripe website builds.
BSD 3-Clause "New" or "Revised" License
8 stars 7 forks source link

add (spamprotection): honeypot spamprotection field #23

Closed Neumes closed 7 years ago

Neumes commented 8 years ago

add (spamprotection): setting for automatic override of forms or using patched updateForm extension hook add (spamprotection): automatic honeypot field inclusion with disable option for userforms

nglasl commented 8 years ago

While I do like this, I'm wondering whether BasisSpamProtectionForm is something we should actually have. The problem is more that existing forms will not extend this, and most people are likely to just use the base form without thinking about whether they actually want spam protection. So even with the config in place, we're likely to end up in a situation where maybe 25% of forms don't have spam protection, and the question is going to come up as.. why? This feels to me like something we're going to need to address in core, one way or another. Thoughts?

silbinarywolf commented 8 years ago

"excludes" solution seems odd to me.

I imagined it more like "private static disable_honeypot_field = false;", and then a user would just disable it on a form to form case in YML:

MyCoolForm:
  disable_honeypot_field: true

or on their form itself:

class MyCoolForm extends Form {
    private static $disable_honeypot_field = true;
}

As for the form, "BasisSpamProtectionForm", it might be worth making this called "BasisForm" so that we can put a lot of reusable goodies into it. Also I personally feel applying the extension directly with private static $extensions = array('BasisFormProtectionExtension') is nicer so there's less back and forth between YML and PHP.

Neumes commented 8 years ago

Oh yeah @SilbinaryWolf, so the reason I did the excludes like that is because it may be for a specific form which isn't necessarily an extension of Form itself, so it can be excluded by its name in configuration, rather than requiring it to be set against the extended Form object (but we can definitely take your suggested route if you think the use case is a bit skewed). Along the lines of this :

BasisFormProtectionExtension:
  excludes:
    - <FormName>

@nglasl The form is there mainly due to the fact of how there is currently no support for an extension hook against Form itself (like what User Defined Forms has with updateForm) so it's somewhat there in as a crutch which I was really hoping we could remove as soon as possible. I could most definitely provide a bit of documentation around it if required. Going to PR this against core and hopefully we can get somewhere with it.

Definitely agree with possibly just changing it to BasisForm and putting everything nice in it @SilbinaryWolf

nglasl commented 7 years ago

@Neumes, could you please update this when you have some time? I assume all the changes discussed have not gone back in just yet.

silbinarywolf commented 7 years ago

Work is being done on this PR: https://github.com/silverstripe-australia/silverstripe-ba-sis/pull/28