Account for undocumented exception in Akismet::getIsSpam #31
Conversation
| $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; |
There was a problem hiding this comment.
Do we want to do something with the exception, even if it's logging?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
I'll append a string from the en.yml file containing a more specific reason.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There's an existing i18n I've used instead. Squashed and pushed
| $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; |
There was a problem hiding this comment.
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.
elliot-sawyer
force-pushed
the
fix/network-error
branch
from
a240cd3 to
94eb1f6
Compare
|
Just fix the linting error and we should be good to go. |
Returns isSpam = false on network error, but can be config'ed to true Log the reason for the failure
elliot-sawyer
force-pushed
the
fix/network-error
branch
from
94eb1f6 to
bc0ecc1
Compare
|
All sorted, thanks @unclecheese ! |
https://github.com/tijsverkoyen/Akismet/blob/6f7987fe181fd0fddbc9832f2ddeaf088425c83f/Akismet.php#L367
Context
If the network fails to contact Akismet, it will throw an undocumented exception with a 400 response. This leaves the method in a state where it returns neither true nor false. If this occurrs, the AkismetField will return isSpam = false. This can be optionally configured to true instead by a developer.
This issue also occurs if any one of the mapping fields is used and marked as spam. The underlying module throws an exception in this case, and the module fails to account for it.