Problem/Motivation

The rollback of #2575105: Use cache collector for state revealed that our testing of adding shutdown functions in shutdown functions is incomplete. Whilst \Drupal\Tests\system\Functional\System\ShutdownFunctionsTest tests this it is vulnerable to changes to core that add or remove shutdown functions. Therefore we should add a KernelTest that eliminates this and asserts that we are working with a clean slate.

Proposed resolution

Add a kernel test to test adding shutdown functions in shutdown functions. I don't think we should remove \Drupal\Tests\system\Functional\System\ShutdownFunctionsTest because that has extra test coverage around different web server set ups where shutdown functions are triggered after the server has sent the response to the browser.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#3 3008140-2.patch1.96 KBalexpott
#3 3008140-3.revert.patch13.06 KBalexpott
#2 3008140-2.patch1.96 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
Related issues: +#2885309: [PHP 7.2] each() function is deprecated, +#2575105: Use cache collector for state
FileSize
3008140-2.patch1.96 KB

Here's a test that would have failed when we made the breaking change - which was #2885309: [PHP 7.2] each() function is deprecated - the way in which we converted to a foreach was wrong. See https://3v4l.org/le78R - it broke on PHP 5 (but not on PHP 7 where a bugfix has been implemented). The fix for this has been committed in #2575105: Use cache collector for state

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3008140-3.revert.patch13.06 KB
3008140-2.patch1.96 KB

Here's a patch that reverts both commits on #2575105: Use cache collector for state - ie. 7fcabc7ad6 and 8861a546b8 so we're back to where we were before the revert commit to show the new test would fall on PHP5 whilst all other tests including \Drupal\Tests\system\Functional\System\ShutdownFunctionsTest should pass.

Re-uploading #2 too so the latest patch is the one that should be reviewed.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Test makes sense, still applies and still passes.

  • catch committed 3eaacac on 9.1.x 
    Issue #3008140 by alexpott: Improve shutdown function testing
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 3eaacac and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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