Problem/Motivation

Currently, an exception raised during cron causes the entire cron run to die. This is extremely problematic, as it essentially renders cron nonfunctional if any module with a cron hook encounters an unexpected condition.

Proposed resolution

Catch exceptions on a per-module basis and log the exception for troubleshooting. Patches in #15 implement this change.

Remaining tasks

The patch is RTBC. It includes tests and has been updated based on committer feedback. Since the patch adds an additional test module, there are different versions for D7 and D8 with different .info files. Both versions of the patch are in #15.

The fact that the original 7.x patch ran successfully against 8.x despite the 7.x .info file has raised a couple questions about testbot and module dependencies. For these questions, refer to:

User interface changes

None.

API changes

None.

Original report by Crell

I'm not 100% confident of how to replicate this, but I think this is the process:

Enable node and comment modules. Create content and comments.

Disable comment.module.

Uninstall comment.module.

Run cron via the UI.

Cron dies with:

FieldException: Attempt to purge a field comment_body that still has instances. in field_purge_field() (line 1141 of /shared/vash/sandboxes/garfield/solrd7/www/modules/field/field.crud.inc).

I'm not entirely sure of the cause here, but I suspect it has to do with how Field API, by design, never deletes data when a field is deleted but waits to clean it up later on cron.

I'm filing this as major, but since it locks cron completely it may qualify as critical. :-( I leave that to the Field API gurus to decide.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Nephele’s picture

The FieldException error is the same as #566048: Attempt to purge a field that still has instances -- but this issue has a slightly different cause and a more extreme effect on the user, so I'm not sure that this can be closed as duplicate.

yched’s picture

Sylvain Lecoy’s picture

I have the cron not working as well, I disabled the comment module, but re-enable it isn't sufficient to get cron back.

I don't know where the error come from, I'll try to debug the live server to provide more information.

Sylvain Lecoy’s picture

Still buggy there and can't figure out where is the problem.

Without the cron logs will flood the database, hope the bug will get fixed very quick :p

chx’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
672 bytes

Someone please write a test that has two modules, one that throws an exception in a cron run and another that just runs and verify the second ran.

chx’s picture

Title: Left over field data + uninstalling module = cron go boom » Handle exceptions thrown in cron
ksenzee’s picture

Version: 7.x-dev » 8.x-dev
Component: field system » cron system
Issue tags: -Needs tests
FileSize
3.16 KB

Here's a version of the patch from #5 that logs the exception as well as catching it. Includes tests.

catch’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Patch and tests look good.

catch’s picture

#7: 978944-7.cron-exceptions.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

"Common Test 2" module? :) Can we please rename this to something more obvious (common_test_cron_helper even common_support would be better, if we anticipate it holding more functionality as the .info file would imply) so that we don't start a new anti-pattern in core? People coming in to debug this test later will have no idea why that's there.

Additionally, some minor nits:

+++ b/modules/simpletest/tests/common_test.module
@@ -225,3 +225,7 @@ function common_test_js_and_css_querystring() {
+function common_test_cron() {

Let's add some PHPDoc here to explain what this is doing.

+++ b/modules/simpletest/tests/common_test.module
@@ -225,3 +225,7 @@ function common_test_js_and_css_querystring() {
+  throw new Exception('Uncaught exception');

Should that be wrapped in t()?

+++ b/modules/simpletest/tests/common_test_2.module
@@ -0,0 +1,9 @@
+function common_test_2_cron() {

PHPdoc.

+++ b/modules/system/system.test
@@ -680,6 +684,19 @@ class CronRunTestCase extends DrupalWebTestCase {
+   * Make sure exceptions thrown on hook_cron don't affect other modules.

hook_cron()

Powered by Dreditor.

ksenzee’s picture

What do you mean, anti-pattern? I was looking forward to system2.module. /me ducks

I probably won't have time to work on this for a week or so - anyone who wants to jump in is welcome.

Aron Novak’s picture

Status: Needs work » Needs review
FileSize
3.67 KB

The beauty fixes.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks like that deals with all of webchick's review. Back to rtbc.

Lars Toomre’s picture

Status: Reviewed & tested by the community » Needs work

A small nit... The info file refers to core 7.x, yet the tests are being run for Drupal 8.x and all tests show green. Shouldn't the core directive in the new info file point to 8.x?

This example leads me to raise a potentially bigger issue. I am under the impression that Drupal 6 modules will not work with D7 and likewise Drupal 7 modules with D8. If my understanding is correct, why is the patch in #12 showing all green? Is the simpletest suite not checking for module version numbers? I would have expected the patch in #12 to have failed for the version check. Do we need to open a new issue for this type of expected failure?

Based on the above, I also believe we need distinct patches here for D7 and D8 before this is RTBC.

Aron Novak’s picture

Status: Needs work » Needs review
FileSize
3.67 KB
3.67 KB

Right, here it is.
Btw. I think the test bot uses the version specs from the issue itself. And it is set to 8.x, it makes sense that the tests ran against it. I don't think the testbot should start to analyze the patch for a version number.

catch’s picture

Not at pc but there's an existing issue for D6 modules satisfy D7 dependencies, will be the same for d7/d8, so no need to worry about that here.

Lars Toomre’s picture

Initially, I was glad to see that both of the revised D7 and D8 patches pass with all green. However, I am puzzled why if one clicks on the view details links that both display 'Drupal core - 8.x'. I am now thinking that the D7 patch again was tested against the D8 code base. I may be wrong, but am very puzzled.

Thanks also @catch for the information about module dependencies and differing Drupal versions. I think that the issue that you were referring to is #1002258: D6 modules satisfy D7 module dependencies.. Please feel to correct me if I am wrong.

Reading through that dependencies issue and these patches once more, I realized that we may have missed another item. There is now a dependency between common_test.module and common_test_cron_helper.module in order for modules/system/system.test to execute properly. I realize that it is a bit pedantic, but does not one of these test modules need to have a dependency on the other specified in its info file?

I do not believe a test file specifies anywhere such dependencies and hence never would be caught in whatever way #1002258: D6 modules satisfy D7 module dependencies. ultimately is resolved. Again, feel free to correct me if I have missed something.

As a side note, I personally am quite bothered by the more significant issue of modules with different Drupal versions passing the Simpletest suite of tests for a particular Drupal version. If we are testing for Drupal X, I would expect all modules enabled for that test suite also must be of version X.

catch’s picture

Yes they're both running against the d8 code base but the only way they could fail would be if that other bug was fixed, which we can work on there.

Lars Toomre’s picture

I added a comment #1002258-21: D6 modules satisfy D7 module dependencies. about using modules with different Drupal major versions in tests for version Y. Hopefully, when that issue is resolved, patches like that in #12 will no longer pass with all greens.

Despite this test failure, I believe that the patches in #15 are largely RTBC assuming that the D7 tests also return green. Sorry for any extraneous noise I may have raised.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Nah it's a good catch, if we'd committed the previous patch the other issue would have had weird test failures. Moving back to rtbc.

Lars Toomre’s picture

Side inquiry, how does one ensure that the first patch in #15 is run against the D7 test suite? For completeness, that patch should be run through all D7 simpletests.

catch’s picture

When the issue is moved back to D7, it'll run against that branch. If you suffix the patch with D7 then it also won't run while the issue is against 8.x.

Lars Toomre’s picture

Oh boy, I did not realize that changing the version selector determined the Drupal version test suite number to process. When #22336: Move all core Drupal files under a /core folder to improve usability and upgrades is committed around November 1st, the patches in #15 would need to reflect the new path location for common.inc as well.

At that time to fix D7 bugs, we are going to have many more comments with patches like #15 where one patch is for D7 and the other is for D8. Although logically the same, they will differ because of the new file structure. It would be great to be able to somehow specify the version test suite to run based on the file name.

Lars Toomre’s picture

I opened issue #1248458: Allow Drupal version test suite specification to address the optional specification of test suite versions. If that suggestion were implemented, the first patch in #15 could be automatically tested against D7 and the second against D8.

As I mentioned in #23, a comment with two patches for differing Drupal versions will become more prevalent after #22336: Move all core Drupal files under a /core folder to improve usability and upgrades lands. Ideally, two green test results (like in #15) will mean that patches for different Drupal versions passed their respective test suites.

The current results in #15 are misleading even though I believe both patches are RTBC. (The first patch in #15 should have failed because a D7 module is being used in the D8 test suite.)

xjm’s picture

Tagging.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Summary added.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great! Nice to see this one put away.

Committed and pushed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.