Problem/Motivation

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

Proposed resolution

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

Script to do the majority of the work

# Replace the FQDN occurrences in comments.
find ./core -type f -not -path "./core/node_modules/*" -not -path "./core/assets/*" -not -path "./core/misc" -exec sed -i -r 's/\\Drupal\\Component\\Utility\\SafeMarkup::format(\(\))?/\\Drupal\\Component\\Render\\FormattableMarkup/g;' {} \;

# Replace the occurrences in code.
find ./core -type f -not -path "./core/node_modules/*" -not -path "./core/assets/*" -not -path "./core/misc" -exec sed -i -r 's/SafeMarkup::format\(/new FormattableMarkup\(/g;' {} \;

# Revert unnecessary changes
git co core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php

# Replace use statements in affected files.
git ls-files -m | xargs sed -i -r 's/use Drupal\\Component\\Utility\\SafeMarkup/use Drupal\\Component\\Render\\FormattableMarkup/g;'

# Apply the smaller post script issue patch from https://www.drupal.org/project/drupal/issues/2958429

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
#14 2958429-3-14.patch274.63 KBalexpott
#12 2958429-2-12.patch274.61 KBalexpott
#12 2958429-12.post-script.patch10.44 KBalexpott
#10 2958429-10.patch274.55 KBalexpott
#6 2958429-6.patch274.75 KBalexpott
#6 2958429-6.post-script.patch10.63 KBalexpott
#6 4-6-interdiff.txt380 bytesalexpott
#4 2958429-4.patch274.8 KBalexpott
#4 2958429-4.post-script.patch10.14 KBalexpott
#4 2-4-interdiff.txt1.64 KBalexpott
#2 2958429-2.patch274.98 KBalexpott
#2 2958429-2.post-script.patch7.85 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2958429-2.post-script.patch7.85 KB
new 2958429-2.patch274.98 KB

Here's patch built with a script - details added to the issue summary and a small post script patch to fix up a few things and properly deprecate the method.

Status: Needs review » Needs work

The last submitted patch, 2: 2958429-2.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2-4-interdiff.txt1.64 KB
new 2958429-4.post-script.patch10.14 KB
new 2958429-4.patch274.8 KB

Fixed #2

Status: Needs review » Needs work

The last submitted patch, 4: 2958429-4.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 4-6-interdiff.txt380 bytes
new 2958429-6.post-script.patch10.63 KB
new 2958429-6.patch274.75 KB

Missed one.

andypost’s picture

andypost
he/him
Primary language Russian
commented

@alexpott Impressive!
Now grep shows only few mentions

core/lib/Drupal/Core/Template/TwigNodeVisitor.php:46:        // Use our own escape filter that is SafeMarkup aware.
core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php:41:   * after SafeMarkup change.
core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php:120:    // Assert that HTML is escaped in "all languages" in UI after SafeMarkup
core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php:38:   * after SafeMarkup change.
core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php:117:    // Assert that HTML is escaped in "all languages" in UI after SafeMarkup
core/tests/Drupal/Tests/Core/Plugin/Context/ContextDefinitionTest.php:113:   * @uses \Drupal\Component\Utility\SafeMarkup

That could be changed in follow-up
Other then that RTBC++

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#2958407: Properly deprecate SafeMarkup::isSafe, SafeMarkup::checkPlain and the SafeMarkup class

@andypost thanks for the review. I think we should do the generic SafeMarkup mention in #2958407: Properly deprecate SafeMarkup::isSafe, SafeMarkup::checkPlain and the SafeMarkup class as that is about more than one method and can be scoped to include the class.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Good idea! let's get this one in first cos it is most disruptive

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2958429-10.patch274.55 KB

rerolled as #2958407: Properly deprecate SafeMarkup::isSafe, SafeMarkup::checkPlain and the SafeMarkup class landed

First, rewinding head to replay your work on top of it...
Generating patches: 100% (6/6), done.
Applying: Script
Using index info to reconstruct a base tree...
M	core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
M	core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
M	core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
M	core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
Auto-merging core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
Auto-merging core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
Auto-merging core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
Applying: Revert something
Using index info to reconstruct a base tree...
M	core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
Applying: Part 1 of post script
Using index info to reconstruct a base tree...
M	core/lib/Drupal/Component/Utility/SafeMarkup.php
M	core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
Auto-merging core/lib/Drupal/Component/Utility/SafeMarkup.php
Applying: More post-script
Applying: Fix stuff
Applying: One got away

Thanks git!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2958429-10.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2958429-12.post-script.patch10.44 KB
new 2958429-2-12.patch274.61 KB

Rerolled.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2958429-2-12.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2958429-3-14.patch274.63 KB

Rerolled using the script.

catch’s picture

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

Committed 45bc055 and pushed to 8.6.x. Thanks!

  • catch committed 45bc055 on 8.6.x 
    Issue #2958429 by alexpott: Properly deprecate SafeMarkup::format()

Status: Fixed » Closed (fixed)

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