Skip to content
This repository has been archived by the owner on Nov 10, 2021. It is now read-only.

[RHDX-291] Set required cron interval ImageCache External #3502

Conversation

jordanpagewhite
Copy link
Contributor

There is a required cron interval config keyvalue that we apparently had
never set for the ImageCache External module. I tested this against the
1.1 and 1.2 releases locally and it seemed like the field was required
in both releases. I am not sure, at least at this point in my debugging,
why we started to encounter these cron failures at this point, instead
of at an earlier date whenever this required value was introduced to the
contrib module. Setting a value resolves the error that arises during
cron execution though. I will verify this on the PR build once a build
either fails or completes successfully by reviewing the logs and testing
the command(s) against the commandline of the PR environment.

JIRA Issue Link

Verification Process

There is a required cron interval config keyvalue that we apparently had
never set for the ImageCache External module. I tested this against the
1.1 and 1.2 releases locally and it _seemed_ like the field was required
in both releases. I am not sure, at least at this point in my debugging,
why we started to encounter these cron failures at this point, instead
of at an earlier date whenever this required value was introduced to the
contrib module. Setting a value resolves the error that arises during
cron execution though. I will verify this on the PR build once a build
either fails or completes successfully by reviewing the logs and testing
the command(s) against the commandline of the PR environment.
@jordanpagewhite jordanpagewhite self-assigned this Feb 11, 2020
@jordanpagewhite jordanpagewhite changed the title Set required cron interval ImageCache External [RHDX-291] Set required cron interval ImageCache External Feb 11, 2020
@jordanpagewhite
Copy link
Contributor Author

Comparison of cron error:

Screen Shot 2020-02-11 at 8 56 33 PM

to cron logs in this PR environment:

Screen Shot 2020-02-11 at 8 56 01 PM

Copy link
Contributor

@robpblake robpblake left a comment

Choose a reason for hiding this comment

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

@jordanpagewhite The issue seems to be that there is a conflict between whatever the ImageCacheExternal 1.2 module version does in its new cron function and what the SimpleSitemapXML module at version 2.12 does in its cron function.

We have a floating version dependency in composer.lock for the ImageCacheExternal module which has silently upgraded prod from 1.1 to 1.2, hence why we're now seeing this problem. The 1.1 version of the ImageCacheExternal module does not have the cron function AFAICT, so this new configuration option is not valid (or ignored there).

The problem only starts to manifest after the ImageCacheExternal has cached some images into /sites/default/files/external. If this directory is empty, then the problem does not manifest i.e. the ImageCacheExternal cron function to clear the cache has nothing to do. This is why our deployments are still working when they execute cron as part of the deployment: we manually clear the /sites/default/files/external directory to remove cached images. I can replicate the failure on your PR by running drush cron after there are images in the /sites/default/files/external directory.

So changes I would suggest here:

  • Update composer.json to lock ImageCacheExternal to 1.2 and disallow floating versions
  • Set the imagecache_external_cron_flush_frequency value to 0 rather than 1 so that the ImageCacheExternal cron function is disabled (we manually clear that directory)
  • Log an issue with the ImageCacheExternal module for the following warnings that I see in the cron output when I run drush cron:
[notice] Starting execution of imagecache_external_cron(), execution of history_cron() took 4.65ms.
 [warning] array_chunk(): Size parameter expected to be greater than 0 imagecache_external.module:374
 [warning] count(): Parameter must be an array or an object that implements Countable imagecache_external.module:375
 [warning] Invalid argument supplied for foreach() imagecache_external.module:378
 [warning] count(): Parameter must be an array or an object that implements Countable form.inc:767
 [notice] Imagecache caches have been flushed

And then to follow-up, I think we should update the SimpleSitemapXML module to a supported version. I would suggest we log an issue against 2.12, but it's EOL and no longer supported.

cheers,

Rob

@robpblake
Copy link
Contributor

@jordanpagewhite I've logged an issue with the ImageCacheExternal project here: https://www.drupal.org/project/imagecache_external/issues/3113027

We do not want imagecache external to run its cron processes since
there is currently a conflict, it seems, with the simple sitemap cron
processes that results in errors / a failure.
@jordanpagewhite
Copy link
Contributor Author

Thank you @robpblake ! I see now that I tricked myself in missing that locally by manually clearing the image cache. I have pushed up a commit to update the config value to 0, and I will push up another in a few minutes with updated composer file(s). Thanks again

@robpblake
Copy link
Contributor

retest this please

@robpblake
Copy link
Contributor

Verified:

  • ImageCacheExternal modue is installed at version 1.2
  • I can successfully run drush cron from the container even when there are images in the /sites/default/files/external directory
  • All tests pass as expected.

LGTM

@robpblake robpblake self-requested a review February 12, 2020 15:59
Copy link
Contributor

@robpblake robpblake left a comment

Choose a reason for hiding this comment

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

@jordanpagewhite This looks good to me!

cheers,

Rob

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants