Problem/Motivation

The SafeMarkup class has been deprecated since 8.0.0 but we didn't have deprecation testing then or a policy to add @trigger_error()

Handling \Drupal\Component\Utility\SafeMarkup::format() in a separate issue because it is still in use - see #2958429: Properly deprecate SafeMarkup::format()

Proposed resolution

Add @trigger_error() and improve the tests to test this.

Remaining tasks

User interface changes

None

API changes

None - this is already documented at https://www.drupal.org/node/2549395

Data model changes

None

CommentFileSizeAuthor
#4 2958407-4.patch8.08 KBalexpott
#4 2-4-interdiff.txt4.52 KBalexpott
#2 2958407-2.patch3.55 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 Thunder commented
FileSize
2958407-2.patch3.55 KB

Here's a patch

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Title: Properly deprecate SafeMarkup::isSafe and SafeMarkup::checkPlain » Properly deprecate SafeMarkup::isSafe, SafeMarkup::checkPlain and the SafeMarkup class
Issue summary: View changes
Status: Needs review » Needs work
Related issues: +#2958429: Properly deprecate SafeMarkup::format()

Going to handle instances of SafeMarkup mention without ::format here as well.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Needs work » Needs review
FileSize
2-4-interdiff.txt4.52 KB
2958407-4.patch8.08 KB

Here's a patch that addresses #2958429-7: Properly deprecate SafeMarkup::format() - thanks @andypost.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented 
+++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
@@ -125,10 +128,6 @@ public function testFormat($string, array $args, $expected, $message, $expected_
-    foreach ($args as $arg) {
-      $this->assertSame($arg instanceof SafeMarkupTestMarkup, SafeMarkup::isSafe($arg));

why this hunk removed? I can't find similar test case for formattable markup

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

@andypost that assertion is a relic of the time when we had a list of safe strings. Fortunately we don’t have that anymore and the equivalent assertion would be nonsense - it’d be assertSame($arg instanceof SafeMarkupTestMarkup, $arg instanceof MarkupInterface).

The point of the test was to ensure $arg didn’t become safe due to the list.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

Aha yes, we always escape arguments now

  • catch committed fdf37cf on 8.6.x 
    Issue #2958407 by alexpott: Properly deprecate SafeMarkup::isSafe,...
catch’s picture

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

Committed fdf37cf and pushed to 8.6.x. Thanks!

Status: Fixed » Closed (fixed)

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