Problem/Motivation
Our client discovered there where no firewall reports anymore.
When looking in the database the table 'cleantalk_sfw' was almost empty, only 2 records.
While in an older version there where more then 17000 records.
Steps to reproduce
Trigger the cron. This module doesn't use drupals cron.
Cron can be triggered by visiting a page.
Each requests is listened to by src/EventSubscriber/BootSubscriber.php
From there the cron class is called: lib/Cleantalk/Common/Cron/Cron.php
The construct contains the following lines:
if ( ! $this->setCronLastStart() ) {
return;
}
Code before these lines will be reached.
Code after these lines will never be reached.
And with that cron functionality will not be triggered like updating the 'cleantalk_sfw' table.
Reason is that $this-setCronLastStart() returns finally the return value of drupals:
\Drupal::state()->set($setting_name, $setting_value);
Which is always null.
So negating the null means that you always enter the early return.
Debugging suggestion
While debugging it might come in handy to comment out
$task_data['next_call'] <= time() in the Cron::checkTasks() method so you don't have to wait for the timewindow .
Proposed resolution
My proposal for resolution is removing the if statement with the early return.
So:
- if ( ! $this->setCronLastStart() ) {
- return;
- }
+ $this->setCronLastStart();
But that ignores the check that the developer initially had in mind.
I couldn't figure out what the original intention was partly due to the large comment where most of the code comes from:
https://git.drupalcode.org/project/cleantalk/-/commit/ea3bc750c1199290c8...
So there is no history with a similar setup where that check was working.
Additional resolution, prevent scalar as array
After applying the above resolution the code ran further but crashed on the warning:
Cannot use a scalar value as an array in Cleantalk\Common\Cron\Cron->checkTasks() (line 243 of web/modules/contrib/cleantalk/lib/Cleantalk/Common/Cron/Cron.php)
There the $task_data is assumed to be an array while it isn't always.
Reason in our case might be due to old settings from the cron, stored in the drupal state wit name: 'cleantalk_cron'
But to prevent crashing the site from a previous state value I suggest to make sure the $task_data is an array.
+ if (!is_array($task_data)) {
+ continue;
+ }
Additional resolution, remove old state
After fixing the cron and the php I discovered that the the old state for 'cleantalk_cron' wasn't compatible anymore with the current cleantalk functionality.
Luckily the module provides in a default set to store when that state isn't in the database.
So last thing I had to do was: drush state:delete cleantalk_cron
A neater solution ofcourse would be to do that in an update hook.
After fixing the cron, the php error and removing that state I was able to trigger the module by visiting any page. Which resulted in filling the cleantalk_sfw table as supposed.
Remaining tasks
I will provide a simple .patch.
Keep in mind that it doesn't replace any logic for the removed if statement.
If there was any intentionally logic at all in that if statement other than triggering an early return based on for example a false of a return value.
Additional an update hook might be a nice solution to prevent an old 'cleantalk_cron' state that doesn't match the current version of the module.
User interface changes
no user interface changes
API changes
no api changes
Data model changes
no data model changes