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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

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

Status: Active » Needs review

Here's a patch...

This works when you do:

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

No for the patch.

alexpott’s picture

Found some more in several module's unit tests.

dawehner’s picture

+++ 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

@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

Status: Needs review » Reviewed & tested by the community

Fair point. Let's do that later.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review

Alex is working on some additional test coverage.

alexpott’s picture

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

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

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

Wim Leers’s picture

  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

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

alexpott’s picture

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

alexpott’s picture

Very small doc improvement.

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

Wim Leers’s picture

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

Status: Needs work » Reviewed & tested by the community
FileSize
33.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

Status: Needs review » Reviewed & tested by the community

Still green so back to RTBC.

catch’s picture

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.