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

CommentFileSizeAuthor
#21 2551335.21.patch33.48 KBwim leers
#17 16-17-interdiff.txt953 bytesalexpott
#17 2551335.17.patch33.45 KBalexpott
#16 2551335.15.patch33.46 KBalexpott
#16 10-15-interdiff.txt1.55 KBalexpott
#10 2551335.10.patch33.26 KBalexpott
#10 2551335.test-only.patch2.55 KBalexpott
#5 2551335.5.patch30.71 KBalexpott
#5 4-5-interdiff.txt12.17 KBalexpott
#4 2551335.3.patch19.25 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: [plan] Remove as much of the SafeMarkup class's methods as possible » Remove unnecessary SafeMarkup usage in tests and clear the static cache before every run
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review

Here's a patch...

This works when you do:

cd core;
phpunit --testsuite unit
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2551335.3.patch19.25 KB

No for the patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 4-5-interdiff.txt12.17 KB
new 2551335.5.patch30.71 KB

Found some more in several module's unit tests.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -48,6 +48,12 @@ protected function setUp() {
+
+    // Reset the static list of SafeStrings to prevent bleeding between tests.
+    $reflected_class = new \ReflectionClass('\Drupal\Component\Utility\SafeMarkup');
+    $reflected_property = $reflected_class->getProperty('safeStrings');
+    $reflected_property->setAccessible(true);
+    $reflected_property->setValue([]);

Ideally 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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@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.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Fair point. Let's do that later.

dawehner’s picture

dawehner
Primary language German
commented
Status: Reviewed & tested by the community » Needs review

Alex is working on some additional test coverage.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2551335.test-only.patch2.55 KB
new 2551335.10.patch33.26 KB

Test-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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

It'll be interesting to see what our test runner does with an exception thrown uncaught from phpunit.

The last submitted patch, 10: 2551335.test-only.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Sweet! the test breaks HEAD and the patch is green.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
  1. +++ b/core/phpunit.xml.dist
@@ -34,7 +34,9 @@
+    <listener class="\Drupal\Tests\Listeners\SafeMarkupSideEffects">

    PHPUnit++ 

  2. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
@@ -320,3 +321,27 @@ public function __toString() {
+ * SafeMarkup::set() is a global static that effects all tests.

    Nit: s/effects/affects/ 

  3. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
@@ -320,3 +321,27 @@ public function __toString() {
+class SafeMarkupTestSafeString implements SafeStringInterface {

    +1 

  4. +++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
@@ -69,11 +69,7 @@ public function providerPlaceholders() {
+      return SafeString::create('<drupal-render-placeholder callback="Drupal\Tests\Core\Render\PlaceholdersTest::callback" arguments="' . '0=' . $args[0] . '" token="' . $token . '"></drupal-render-placeholder>');

    I don't think this even needs to be a safe string. 

  5. +++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
@@ -151,7 +147,7 @@ public function providerPlaceholders() {
-          $generate_placeholder_markup() => [
+          (string) $generate_placeholder_markup() => [

@@ -318,8 +314,8 @@ public function providerPlaceholders() {
-    $expected_placeholder_render_array = $x['#attached']['placeholders'][$generate_placeholder_markup()];
-    unset($x['#attached']['placeholders'][$generate_placeholder_markup()]['#cache']);
+    $expected_placeholder_render_array = $x['#attached']['placeholders'][(string) $generate_placeholder_markup()];
+    unset($x['#attached']['placeholders'][(string) $generate_placeholder_markup()]['#cache']);

@@ -333,6 +329,7 @@ public function providerPlaceholders() {
+    $placeholder_markup = (string) $placeholder_markup;

    These suggest that too.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I especially like how the added listener will prevent similarly fragile tests from being added! :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 10-15-interdiff.txt1.55 KB
new 2551335.15.patch33.46 KB

Thanks @Wim Leers.
1. :)
2. Fixed
3. :)
4. & 5: Added a comment.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2551335.17.patch33.45 KB
new 16-17-interdiff.txt953 bytes

Very small doc improvement.

The last submitted patch, 16: 2551335.15.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2551335.17.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2551335.21.patch33.48 KB

Automatically rebased, thank you git.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2551335.21.patch, failed testing.

Status: Needs work » Needs review

dawehner queued 21: 2551335.21.patch for re-testing.

stefan.r’s picture

stefan.r commented
Status: Needs review » Reviewed & tested by the community

Still green so back to RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 3d44e9a on 8.0.x 
    Issue #2551335 by alexpott, Wim Leers: Remove unnecessary SafeMarkup...

Status: Fixed » Closed (fixed)

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