Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
The layout_builder test module no_transitions_css has an invalid hook_page_attachments definition.
/**
* Implements hook_page_attachments().
*/
function settings_tray_test_css_page_attachments(array &$attachments) {
// Unconditionally attach an asset to the page.
$attachments['#attached']['library'][] = 'settings_tray_test_css/drupal.css_fix';
}
The test which uses it has a "@todo" to remove it after #2901792: Disable all animations in Javascript testing. However, its library never runs. It doesn't even load the settings_tray_test to accidentally load the library.
I discovered while working on a PHPStan extension to perform some analysis on Drupal core.
Note: Using configuration file /Users/mglaman/Drupal/sites/commerce2x/phpstan.neon.
PHP Fatal error: Cannot redeclare settings_tray_test_css_page_attachments() (previously declared in /Users/mglaman/Drupal/sites/commerce2x/web/core/modules/layout_builder/tests/modules/no_transitions_css/no_transitions_css.module:13) in /Users/mglaman/Drupal/sites/commerce2x/web/core/modules/settings_tray/tests/modules/settings_tray_test_css/settings_tray_test_css.module on line 16
Fatal error: Cannot redeclare settings_tray_test_css_page_attachments() (previously declared in /Users/mglaman/Drupal/sites/commerce2x/web/core/modules/layout_builder/tests/modules/no_transitions_css/no_transitions_css.module:13) in /Users/mglaman/Drupal/sites/commerce2x/web/core/modules/settings_tray/tests/modules/settings_tray_test_css/settings_tray_test_css.module on line 16
Proposed resolution
Fix the hook name
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3020142-no_transitions_css-7.patch | 3.37 KB | tim.plunkett |
| #2 | 3020142-2.patch | 841 bytes | mglaman |
Comments
Comment #2
mglamanPatch. PHPStan did not explode once this was applied.
Comment #3
borisson_Yep! This makes sense. I think it means that the test using this module is probably not good enough. We can look at that in a followup though. This already make the current (broken) situation better.
Comment #4
alexpottGiven that the only thing this module does is supposed to implement this hook I think it must not be needed. Rather than fixing this and thereby changing the tests I think we should discuss completely removing this module. I think CSS transitions were more problematic in the PhantomJS days but the issue we need to look at is #2901792: Disable all animations in Javascript testing which seems to be based disabling all transitions.
Anyhow, no_css_transitions is enabled in
\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTestBasewhich is used by\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTestand\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockPrivateFilesTestand I've not seen random fails in these tests.Comment #5
tedbowI would say it could be removed. If we haven't got random failures in the test in all this time with the CSS not be active.
Comment #6
mglamanWorks for me to remove the test module. Wanted to make a patch fixing it first. Maybe a follow up can be made to remove the settings tray one after.
Comment #7
tim.plunkettAgreed.
Comment #8
borisson_Removing dead code is always a win, I think.
Comment #9
alexpottCommitted and pushed c00be4c49c to 8.7.x and 1f4544c992 to 8.6.x. Thanks!
Backported to 8.6.x as the test module is similarly useless there :)
Comment #13
tim.plunkettRetroactively tagging