Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Account for undocumented exception in Akismet::getIsSpam #31

Merged
merged 1 commit into from
Apr 20, 2020
Merged

Account for undocumented exception in Akismet::getIsSpam #31

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 39 additions & 11 deletions src/AkismetField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

namespace SilverStripe\Akismet;

use Exception;
use Psr\Log\LoggerInterface;
use SilverStripe\Akismet\Service\AkismetService;
use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Forms\CheckboxField;
use SilverStripe\Forms\Validator;
use SilverStripe\ORM\ValidationResult;
Expand All @@ -20,6 +23,13 @@
*/
class AkismetField extends FormField
{
/**
* If the Akismet network response fails, it is neither true or false
* This is the value assigned on a 400
*
* @var boolean
*/
private static $is_spam_when_response_fails = false;
/**
* @var array
*/
Expand All @@ -30,7 +40,7 @@ class AkismetField extends FormField
* @var boolean
*/
protected $isSpam = null;

/**
* Get the nested confirmation checkbox field
*
Expand All @@ -43,7 +53,7 @@ protected function confirmationField()
if (empty($requireConfirmation)) {
return null;
}

// If confirmation is required then return a checkbox
return CheckboxField::create(
$this->getName(),
Expand All @@ -56,23 +66,23 @@ protected function confirmationField()
->setMessage($this->getMessage(), $this->getMessageType())
->setForm($this->getForm());
}

public function Field($properties = array())
{
$checkbox = $this->confirmationField();
if ($checkbox) {
return $checkbox->Field($properties);
}
}

public function FieldHolder($properties = array())
{
$checkbox = $this->confirmationField();
if ($checkbox) {
return $checkbox->FieldHolder($properties);
}
}

/**
* @return array
*/
Expand All @@ -81,7 +91,7 @@ public function getSpamMappedData()
if (empty($this->fieldMapping)) {
return null;
}

$result = array();
$data = $this->form->getData();

Expand All @@ -91,7 +101,7 @@ public function getSpamMappedData()

return $result;
}

/**
* This function first gets values from mapped fields and then checks these values against
* akismet and then notifies callback object with the spam checking result.
Expand All @@ -102,7 +112,7 @@ public function getSpamMappedData()
*/
public function validate($validator)
{

// Check that, if necessary, the user has given permission to check for spam
$requireConfirmation = Config::inst()->get(AkismetSpamProtector::class, 'require_confirmation');
if ($requireConfirmation && !$this->Value()) {
Expand All @@ -116,7 +126,7 @@ public function validate($validator)
);
return false;
}

// Check result
$isSpam = $this->getIsSpam();
if (!$isSpam) {
Expand Down Expand Up @@ -184,10 +194,28 @@ public function getIsSpam()
// Check result
/** @var AkismetService $api */
$api = AkismetSpamProtector::singleton()->getService();
$this->isSpam = $api && $api->isSpam($content, $author, $email, $url);
$this->isSpam = false;
try {
$this->isSpam = $api && $api->isSpam($content, $author, $email, $url);
} catch (Exception $e) {
//if the network response fails, it still needs to be true/false
$this->isSpam = (bool) $this->config()->is_spam_when_response_fails;
Copy link

Choose a reason for hiding this comment

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

Do we want to do something with the exception, even if it's logging?

Copy link
Author

Choose a reason for hiding this comment

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

To be honest, I'm not too concerned with logging the error, I have other tools to capture that information. The current module doesn't handle exceptions at all, and I'd prefer to keep that as a separate feature if it's required.

This is causing production issues for several of our CWP clients and I'd appreciate a review and merge to fix this supported module. Thanks.

Copy link

Choose a reason for hiding this comment

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

Can we just add a Injector::inst()->get(LoggerInterface::class)->error($e->getMessage()); rather than just swallowing it? If you're marking authentic comments as spam, I think you want some awareness of it.

Copy link
Author

Choose a reason for hiding this comment

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

The exception message only says "invalid" when caught. Is there anything else I should include with the message? I'm hesitant to include the actual values of the four checked fields, as it means logging some level of personal user information..

Copy link
Author

Choose a reason for hiding this comment

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

I'll append a string from the en.yml file containing a more specific reason.

Copy link

@unclecheese unclecheese Apr 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If that's the case, then let's just provide some context in the error.

Injector::inst()->get(LoggerInterface::class)->error(sprintf(
  'Akismet marked content as spam due to network error. Response from service: %s',
  $e->getMessage()
));

No need to i18n an error message.

Copy link
Author

Choose a reason for hiding this comment

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

There's an existing i18n I've used instead. Squashed and pushed

$errorMessage = sprintf(
"%s: %s",
$e->getMessage(),
_t(
__CLASS__ . '.SPAM',
"Your submission has been rejected because it was treated as spam."
)
);
Injector::inst()
->get(LoggerInterface::class)
->error($errorMessage);
}

return $this->isSpam;
}

/**
* Get the fields to map spam protection too
*
Expand Down