Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check and update for SS4 compatability #23

Merged
merged 5 commits into from
Dec 18, 2017

Conversation

raissanorth
Copy link

In progress.

@raissanorth raissanorth changed the title Check and update for SS4 compatability WIP: Check and update for SS4 compatability Dec 14, 2017
Copy link

@NightJar NightJar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small things :)

* In this case, 'mollom_captcha_requested' session is set to true
* so that Field() knows it's time to display captcha
* @return boolean - true when Mollom confirms that the submission is ham (not spam)
* - false when Mollom confirms that the submission is spam

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But... but this is Akismet, not Mollom!

@tractorcow must have based it on the mollom module - which is now correctly archived, since:

End-of-Life Announcement: As of 2 April 2018, Acquia will no longer support or maintain the Mollom product. Learn More

license.md Outdated
@@ -1,3 +1,5 @@
Revised BSD License

Copyright (c) 2016, Damian Mooyman

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not important, but could be 2017 ;)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd avoided doing this recently because it'll have to be updated again in a couple of weeks anyway =D

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, super unimportant :P

@@ -66,7 +74,7 @@ class AkismetSpamProtector implements SpamProtector

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code sniffer is complaining about the preceeding line (73) in that it begins with an underscore. Which is good, because otherwise I probably wouldn't have noticed that the Readme.md notes that the configuration value is api_key, not _api_key. This will need altering too.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is to say, please remove the underscore ;)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above this comment is closed by the newer changes, effectively retaining the functionality on an instance basis rather than a static one, which is nicer.

@@ -133,80 +144,3 @@ public function testProcessor()
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linter complains about 2 empty lines here, remove one :)

.travis.yml Outdated

# Install composer dependencies
- composer validate
- composer require --no-update silverstripe/recipe-core:1.0.x-dev

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need this to be recipe-cms in order to get tests to pass :)

Copy link

@NightJar NightJar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You sat here and watched me do this.
You know what to do ;)

.gitattributes Outdated
@@ -4,3 +4,4 @@
/.gitignore export-ignore
/.travis.yml export-ignore
/.scrutinizer.yml export-ignore
/codecov.yml

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export-ignore

if (self::$_api_key) {
return self::$_api_key;
if (self::$api_key) {
return self::$api_key;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think now that we've had a look at what the protected static $_api_key was for, that we can just get rid of it entirely.
This simplifies the code in places like this, where this if statement is now entirely unneeded.

@robbieaverill robbieaverill self-assigned this Dec 17, 2017
@@ -126,7 +135,9 @@ public function validate($validator)
'messageType' => 'error',
));
$formName = $this->getForm()->FormName();
Session::set("FormInfo.{$formName}.errors", $errors);

$this->getForm()->sessionMessage($errorMessage, ValidationResult::TYPE_GOOD);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting here, this wasn't working so we marked the test as incomplete so we could fix it on its own after merge

Copy link

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment :)

*
* @var string
*/
protected static $_api_key = null;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add this back, just without the leading underscore?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, true :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this property has a different purpose by the looks of it, so we'll need to retain it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up, I see this is done on a different line as protected $apiKey = '';, which is cool :)

@robbieaverill robbieaverill removed their assignment Dec 17, 2017
@robbieaverill robbieaverill changed the title WIP: Check and update for SS4 compatability Check and update for SS4 compatability Dec 17, 2017
@robbieaverill
Copy link

Ok so we've replaced the public static methods with an injectable service for AkismetSpamProtector, and defined the order of priority for API key definition as:

  1. Set to singleton via public API
  2. Config API
  3. Environment

There's still the issue of the form validation message not showing up during the functional test, which we've marked as incomplete for now.

The intention is to test this module when we check the spam protector module shortly (silverstripe/silverstripe-spamprotection#62).

@tractorcow would you mind having a review of this pull request and see what you think?

*
* @var string
*/
protected static $_api_key = null;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up, I see this is done on a different line as protected $apiKey = '';, which is cool :)

@@ -66,7 +74,7 @@ class AkismetSpamProtector implements SpamProtector

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above this comment is closed by the newer changes, effectively retaining the functionality on an instance basis rather than a static one, which is nicer.

@NightJar NightJar merged commit 835ee2f into silverstripe:master Dec 18, 2017
@NightJar NightJar deleted the pulls/4.0/ss4-compat branch December 18, 2017 04:42
@robbieaverill robbieaverill mentioned this pull request Dec 18, 2017
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants