fix: if no spam protector set, fail sliently

[wilr]

Tidies up the repo to match other modules (cc @GuySartorelli )

[GuySartorelli]

This should probably be multiple commits, if not multiple PRs. The changes to composer.json and the code/ (now src/ in this PR) directory are definitely not related to the commit message. Can you please split those out to their own MNT prefixed commit?

[GuySartorelli]

I don't think we should change this. As mentioned in #103, when we upgraded this module for CMS 5 compatibility and dropped support for the silverstripe/akismet module we made a conscious decision to leave this exception here - developers should be made aware early on that there's a missing dependency, and may miss a console error.

[wilr]

@GuySartorelli this is for an environment where no recaptcha key is set, that should not kill the entire web page.

[GuySartorelli]

This module doesn't know anything about recaptcha - a separate module would need to be used (e.g. undefinedoffset/silverstripe-nocaptcha) and set as the spam protector following these instructions in the README.

That separate module is then in charge of how it handles missing configuration e.g. missing recaptcha keys.

If you're calling enableSpamProtection(), then there absolutely should be a protector set - otherwise there has been a developer error. I can't see any way this shouldn't throw an error for the developer to resolve.

[wilr]

@GuySartorelli I disagree completely (website going down and showing an error to users is absolutely last resort) but I've put my decision behind a config flag for now so it can be toggled off at least.

[GuySartorelli]

Sorry, I'm just struggling to understand how you'd get into a situation where this happens on a production website without a developer having encountered the exception first and remedied the configuration... can you please help me understand the scenario in which what would happen?

[GuySartorelli]

This PR is making the following change:

-            throw new LogicException('No spam protector has been set. Null is not valid value.');
+            if ($this->config()->get('throw_exception_on_missing_protector')) {
+                throw new LogicException('No spam protector has been set. Null is not valid value.');
+            } else {
+                Injector::inst()->get(LoggerInterface::class)->warning(
+                    'No spam protector has been set. Null is not valid value.'
+                );
+            }

The change is regarding this exception.... what warning are you referring to?

Edit: Sorry, I now realise you mean the console warning in the PR change has been replaced with adding a PHP warning. In some setups that's better than a console error, but in some setups it will be worse, if those aren't being sent to anyone or to some central "hey you need to look at this" service.

[emteknetnz]

Any thoughts from the rest of https://github.com/orgs/silverstripe/teams/core-team?

Throw hard exceptions if it's something is misconfigured so people get immediate feedback is the general philosophy we follow. Silent errors are generally bad, means things can be broken for a long time before anyone realises.

@wilr Sorry to hear about you about woes with your migration, though sounds like what happened there is a lack of testing as part of the migration. I don't see that as a good reason to change our general philosophy

[wilr]

Ok given the change as now does not change behaviour I don't see any problem merging?

[GuySartorelli]

The problem as I see it is that this just doesn't seem like a good change. I don't think we should make it easier to ignore misconfigured environments - and there are already workarounds if you do want to, for some reason.

[wilr]

I don't believe any of the work arounds are very user friendly. My original comment is still, having no spam protection is better than a broken website.

[michalkleiner]

I'm not gonna pick sides here, but 2-3 points from me:

• the PR mixes concerns and should be split to address the structure/code style from functional changes
• we should find a solution where a site doesn't fail fatally when there's a misconfigured spam protector (recaptcha or other), whether it's having a default fake/mock protector or some other way
• I don't see adding the config here as the right way how to address that.

[wilr]

@michalkleiner Sure. I've spilt it out now to a new PR as requested (#106)

[wilr]

we should find a solution where a site doesn't fail fatally when there's a misconfigured spam protector (recaptcha or other), whether it's having a default fake/mock protector or some other way

Agreed @michalkleiner so I've created a workaround for this project in a NullSpamProtector but it sounds from Guy's comments they would prefer the exception for users to be the prefered output (which I still disagree with) but it's easily solved at the application level.

<?php

use SilverStripe\Forms\HiddenField;

class NullSpamProtector implements SpamProtector
{
    public function getFormField($name = null, $title = null, $value = null)
    {
        return HiddenField::create($name, $title, $value);
    }


    public function setFieldMapping($fieldMapping)
    {
        return null;
    }
}

Revelant YAML for anyone who needs it

---
name: spam
---
SilverStripe\SpamProtection\Extension\FormSpamProtectionExtension:
  default_spam_protector: NullSpamProtector
---
name: recaptcha
envvarset: SS_NOCAPTCHA_SITE_KEY
---
SilverStripe\SpamProtection\Extension\FormSpamProtectionExtension:
  default_spam_protector: UndefinedOffset\NoCaptcha\Forms\NocaptchaProtector