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.
Follow-up to #2549943: [plan] Remove as much of the SafeMarkup class's methods as possible
Problem/Motivation
SafeMarkup is a static that bleeds between PHPUnit tests we should reset it in UnitTestCase::setUp() - but we can't at the moment because tests rely on being able to mark things safe in data providers which are all run before all tests when you run all PHPUnit tests from the command line.
This problem caused a revert of #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender
Proposed resolution
- Remove all unnecessary SafeMarkup usage in unit tests.
- Reset the static in
UnitTestCase::setUp()
Remaining tasks
Review and commit.
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#21 | 2551335.21.patch | 33.48 KB | Wim Leers |
#17 | 16-17-interdiff.txt | 953 bytes | alexpott |
#17 | 2551335.17.patch | 33.45 KB | alexpott |
#16 | 2551335.15.patch | 33.46 KB | alexpott |
#16 | 10-15-interdiff.txt | 1.55 KB | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottHere's a patch...
This works when you do:
Comment #4
alexpottNo for the patch.
Comment #5
alexpottFound some more in several module's unit tests.
Comment #6
dawehnerIdeally we would kinda assert that we don't have new entries coming in. If we move this onto a method dedicated tests could opt out of that checking.
Comment #7
alexpott@dawehner yes good idea. I think it is too early for that since there is still some left. The other SafeMarkup method removal issues should completely tidy that up. I'll open a followup.
Comment #8
dawehnerFair point. Let's do that later.
Comment #9
dawehnerAlex is working on some additional test coverage.
Comment #10
alexpottTest-only patch is the interdiff. Created a general listener folder instead of specific standards one cause it seems pointless.
By adding the SafeMarkupSideEffects listener we can ensure that no test pollutes the SafeMarkup list in the data providers.
Comment #11
alexpottIt'll be interesting to see what our test runner does with an exception thrown uncaught from phpunit.
Comment #13
alexpottSweet! the test breaks HEAD and the patch is green.
Comment #14
Wim LeersPHPUnit++
Nit: s/effects/affects/
+1
I don't think this even needs to be a safe string.
These suggest that too.
Comment #15
Wim LeersI especially like how the added listener will prevent similarly fragile tests from being added! :)
Comment #16
alexpottThanks @Wim Leers.
1. :)
2. Fixed
3. :)
4. & 5: Added a comment.
Comment #17
alexpottVery small doc improvement.
Comment #19
Wim LeersComment #21
Wim LeersAutomatically rebased, thank you git.
Comment #24
stefan.r CreditAttribution: stefan.r commentedStill green so back to RTBC.
Comment #25
catchCommitted/pushed to 8.0.x, thanks!