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:
- #1002258: D6 modules satisfy D7 module dependencies.
- #1248458: Allow Drupal version test suite specification
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.
Comment | File | Size | Author |
---|---|---|---|
#15 | 978944_cron_exceptions_D7-15.patch | 3.67 KB | Aron Novak |
#15 | 978944_cron_exceptions_D8-15.patch | 3.67 KB | Aron Novak |
#12 | 978944-8.cron-exceptions.patch | 3.67 KB | Aron Novak |
#7 | 978944-7.cron-exceptions.patch | 3.16 KB | ksenzee |
#5 | 978944.patch | 672 bytes | chx |
Comments
Comment #1
Nephele CreditAttribution: Nephele commentedThe 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.
Comment #2
yched CreditAttribution: yched commentedSubscribe.
#566048: Attempt to purge a field that still has instances is fixed by #915906: Deleting node type with only instance of a field leaves the field in a strange zombie state, but it's less clear whether it will fix this one as well. Handling modules being disabled / uninstalled is a long-time PITA.
We should see clearer when #915906: Deleting node type with only instance of a field leaves the field in a strange zombie state and #974712: Deleting comment_body field on sole content type breaks subsequent content types land.
Comment #3
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI 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.
Comment #4
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedStill 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
Comment #5
chx CreditAttribution: chx commentedSomeone 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.
Comment #6
chx CreditAttribution: chx commentedComment #7
ksenzeeHere's a version of the patch from #5 that logs the exception as well as catching it. Includes tests.
Comment #8
catchPatch and tests look good.
Comment #9
catch#7: 978944-7.cron-exceptions.patch queued for re-testing.
Comment #10
webchick"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:
Let's add some PHPDoc here to explain what this is doing.
Should that be wrapped in t()?
PHPdoc.
hook_cron()
Powered by Dreditor.
Comment #11
ksenzeeWhat 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.
Comment #12
Aron NovakThe beauty fixes.
Comment #13
catchLooks like that deals with all of webchick's review. Back to rtbc.
Comment #14
Lars Toomre CreditAttribution: Lars Toomre commentedA 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.
Comment #15
Aron NovakRight, 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.
Comment #16
catchNot 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.
Comment #17
Lars Toomre CreditAttribution: Lars Toomre commentedInitially, 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.
Comment #18
catchYes 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.
Comment #19
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #20
catchNah 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.
Comment #21
Lars Toomre CreditAttribution: Lars Toomre commentedSide 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.
Comment #22
catchWhen 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.
Comment #23
Lars Toomre CreditAttribution: Lars Toomre commentedOh 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.
Comment #24
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.)
Comment #25
xjmTagging.
Comment #25.0
xjmUpdated issue summary.
Comment #25.1
xjmUpdated issue summary.
Comment #26
xjmSummary added.
Comment #27
webchickGreat! Nice to see this one put away.
Committed and pushed to 8.x and 7.x. Thanks!
Comment #28.0
(not verified) CreditAttribution: commentedUpdated issue summary.