Problem/Motivation

#2454447: Split Utility\String class to support PHP 7 (String is a reserved word) deprecated all the functions in Utility/String.
Following changes took place :

  1. String::checkPlain() moved to SafeMarkup::checkPlain()
  2. String::format() moved to SafeMarkup::format()
  3. String::placeholder() moved to SafeMarkup::placeholder()

Since String class is deprecated but Core still uses above three String:: * functions so this issue aims to make use of SafeMarkup::* in all such places.

Proposed resolution

Move all usages of above mentioned String::* to SafeMarkup::*

Remaining tasks

  • Patch
  • Review

User interface changes

None

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

prateekMehta’s picture

Issue summary: View changes
dawehner’s picture

Maybe its worth trying out https://github.com/grom358/d8codetools for this huge change.

AjitS’s picture

Assigned: Unassigned » AjitS

Working on it now. Would it make sense to replace these functions in separate tickets? One could be done in this ticket and the 2 in follow up tickets?

alexpott’s picture

Please don't remove the methods in this issue. Also we need to attach this issue to the CR.

prateekMehta’s picture

Status: Active » Needs review
FileSize
611.87 KB

Moving all usages of String::* to SafeMarkup::*
replacing Drupal\Component\Utility\String Drupal\Component\Utility\SafeMarkup or
Adding Drupal\Component\Utility\SafeMarkup wherever required.

Status: Needs review » Needs work

The last submitted patch, 5: use_utility_safemarkup-2457887-5.patch, failed testing.

prateekMehta’s picture

Status: Needs work » Needs review
FileSize
611.5 KB

Rerolling , and not removing methods as suggested in #4.

Status: Needs review » Needs work

The last submitted patch, 7: use_utility_safemarkup-2457887-7.patch, failed testing.

prateekMehta’s picture

Status: Needs work » Needs review
FileSize
450 bytes

Removing Syntax Errors.

prateekMehta’s picture

Sorry wrong patch.

Status: Needs review » Needs work

The last submitted patch, 10: use_utility_safemarkup-2457887-10.patch, failed testing.

The last submitted patch, 9: use_utility_safemarkup-2457887-9.patch, failed testing.

prateekMehta’s picture

Status: Needs work » Needs review
FileSize
611.12 KB

Making changes to .inc , .module , .twig etc files.

Status: Needs review » Needs work

The last submitted patch, 13: use_utility_safemarkup-2457887-13.patch, failed testing.

prateekMehta’s picture

Status: Needs work » Needs review
FileSize
612.65 KB

Missed at aq few more places, sorry for negligence.

Status: Needs review » Needs work

The last submitted patch, 15: use_utility_safemarkup-2457887-15.patch, failed testing.

prateekMehta’s picture

Status: Needs work » Needs review
FileSize
613.18 KB

Adding Utility/SafeMarkup where required.

Status: Needs review » Needs work

The last submitted patch, 17: use_utility_safemarkup-2457887-17.patch, failed testing.

AjitS’s picture

Assigned: AjitS » Unassigned

@prateekMehta Alex has mentioned above that the methods should not be removed in this issue. That is the reason I did not work on it even after assigning to myself. I think we should wait for some inputs here before resuming the work.

@alexpott : Where should the methods removed then? And should there be a new CR for this? AFAIK the CR is created after the issue is fixed (after code commit)? Or is there a CR already present for this?

prateekMehta’s picture

Status: Needs work » Needs review
FileSize
613.14 KB

@AjitS i think what he is referring to is https://www.drupal.org/node/2458387 , ie not removing the class itself.
May be that can act as a follow up issue. @alexpott correct me if am wrong?
Nevertheless re rolling, we can use it later if its required.

dawehner’s picture

@alexpott : Where should the methods removed then? And should there be a new CR for this? AFAIK the CR is created after the issue is fixed (after code commit)? Or is there a CR already present for this?

#2458387: Remove Utility\String class will remove them
Note: We don't need a new change record, because the old issue already introduced one, see https://www.drupal.org/node/2457593

alexpott’s picture

Status: Needs review » Needs work

Usages like the following still exist. And please post interdiffs - see https://drupal.org/documentation/git/interdiff.

$roles = array_map(array('\Drupal\Component\Utility\String', 'checkPlain'), user_role_names(TRUE));
//...
      '#options' => array_map(array('\Drupal\Component\Utility\String', 'checkPlain'), user_role_names(TRUE)),
//...
      return $this->caseTransform(StringUtility::checkPlain($value), $this->options['case']);
prateekMehta’s picture

Status: Needs work » Needs review
FileSize
615.66 KB
3.24 KB

Making changes as mentioned in #22 ,

root_brute’s picture

Status: Needs review » Reviewed & tested by the community

Looks good :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/src/Plugin/views/argument/FieldList.php
    @@ -7,7 +7,7 @@
    -use Drupal\Component\Utility\SafeMarkup as UtilityString;
    +use Drupal\Component\Utility\SafeMarkup as UtilitySafeMarkup;
    
    +++ b/core/modules/views/src/Plugin/views/argument/ListString.php
    @@ -7,7 +7,7 @@
    -use Drupal\Component\Utility\SafeMarkup as UtilityString;
    +use Drupal\Component\Utility\SafeMarkup as UtilitySafeMarkup;
    

    I don't think these use statements are needed at all.

  2. +++ b/core/modules/options/src/Plugin/views/argument/StringListField.php
    @@ -13,7 +13,7 @@
    -use Drupal\Component\Utility\String as StringUtility;
    +use Drupal\Component\Utility\SafeMarkup as SafeMarkupUtility;
    
    @@ -84,7 +84,7 @@ public function summaryName($data) {
    -      return $this->caseTransform(StringUtility::checkPlain($value), $this->options['case']);
    +      return $this->caseTransform(SafeMarkupUtility::checkPlain($value), $this->options['case']);
    

    No need for the alias.

prateekMehta’s picture

yes. but they were already there so i didn't remove them.

alexpott’s picture

@prateekMehta then why did you change them?

prateekMehta’s picture

Removing alias suggested in #25. Hope nothing else needs to be done.

prateekMehta’s picture

Status: Needs work » Needs review
FileSize
9.9 KB

Interdiff 23 - 27, probably the correct way to upload an interdiff.

prateekMehta’s picture

@alexpott Using Utility/SafeMarkup as UtilitySafeMarkup , looked more logical than using it as UtilityString. I have removed aliases anyway.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -172,7 +172,7 @@
-    return UtilitySafeMarkup::checkPlain((string) $this->operator) . ' ' . UtilitySafeMarkup::checkPlain((string) $this->value);
+    return SafeMarkup::checkPlain((string) $this->operator) . ' ' . UtilitySafeMarkup::checkPlain((string) $this->value);

Still have a UtilitySafeMarkup in this line. Hope fully the tests would fail if this had been set to needs review.

sidharrell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 28: use_utility_safemarkup-2457887-27.patch, failed testing.

prateekMehta’s picture

Status: Needs work » Needs review
FileSize
615.36 KB

Removing it.

Status: Needs review » Needs work

The last submitted patch, 34: use_utility_safemarkup-2457887-34.patch, failed testing.

prateekMehta’s picture

Status: Needs work » Needs review
FileSize
651 bytes
615.42 KB

re-rolling and posting an interdiff.

stefan.r’s picture

I had accidently worked on this in another issue and just did an interdiff with my own patch. They were essentially the same, just 4 differences:

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -80,6 +80,7 @@ public function translate($string, array $args = array(), array $options = array
        *   A translated string.
        *
        * @see self::translate()
    +   * @see \Drupal\Component\Utility\SafeMarkup
        * @see t()
        * @see \Drupal\Component\Utility\SafeMarkup::format()
    

    Do we need to link to both the class and to one of the methods?

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -104,7 +105,7 @@ public function formatPlural($count, $singular, $plural, array $args = array(),
        *   not need to include @count in this array; this replacement is done
    
    +++ b/core/modules/comment/src/Tests/CommentBlockTest.php
    @@ -6,7 +6,7 @@
     namespace Drupal\comment\Tests;
    -use Drupal\Component\Utility\String;
    +use Drupal\Component\Utility\SafeMarkup;
     use Drupal\user\RoleInterface;
    

    Could use a newline after the namespace statement.

  3. +++ b/core/modules/node/src/Tests/Views/BulkFormAccessTest.php
    @@ -6,7 +6,7 @@
     namespace Drupal\node\Tests\Views;
    -use Drupal\Component\Utility\String;
    +use Drupal\Component\Utility\SafeMarkup;
     use Drupal\user\Entity\User;
    

    Could use a newline after the namespace statement.

  4. +++ b/core/modules/views/src/Plugin/views/ViewsHandlerInterface.php
    @@ -7,6 +7,7 @@
     
    +use Drupal\Component\Utility\SafeMarkup;
    

    Looks like this may not be needed?

Berdir’s picture

Changing parent, so all these issues are visible in the php7 meta issue.

Mile23’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs reroll.

$ git apply 2457887-37.patch 
error: patch failed: core/modules/field_ui/src/Tests/ManageFieldsTest.php:7
error: core/modules/field_ui/src/Tests/ManageFieldsTest.php: patch does not apply
error: core/modules/taxonomy/src/Controller/TermAutocompleteController.php: No such file or directory
error: core/modules/taxonomy/src/Plugin/Field/FieldFormatter/LinkFormatter.php: No such file or directory
error: core/modules/taxonomy/src/Plugin/Field/FieldFormatter/PlainFormatter.php: No such file or directory
error: patch failed: core/modules/taxonomy/src/Tests/TermTest.php:8
error: core/modules/taxonomy/src/Tests/TermTest.php: patch does not apply
stefan.r’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
611.47 KB
4.3 KB

reroll

stefan.r’s picture

FileSize
610.97 KB
1.73 KB

And this addresses comments in #38.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

All the String::placeholder(), String::checkPlain(), and String::format() are gone.

Can't find any results for array_map()-style calling, either.

Looking good. :-)

Berdir’s picture

Priority: Normal » Critical

This is part of the critical PHP7 meta issue. We must do this to be able to remove the String class.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

OK, might as well break everything sooner than later! Added this issue to https://www.drupal.org/node/2457593 so it's mentioned in a change record. All other feedback looks like it was addressed.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 70f8ac6 on 8.0.x
    Issue #2457887 by prateekMehta, stefan.r, rpayanm, alexpott: Use Utility...
Mile23’s picture

This issue is critical while this other issue is postponed to 8.1.x: #2228741: Replace calls to format_string() with Drupal\Component\Render\FormattableMarkup

Makes no sense.

stefan.r’s picture

String is a reserved word in PHP7 so this was critical because it's required for PHP7 support :)

Mile23’s picture

An easy level of follow-through to finish the job of deprecation, is what I'm saying.

Status: Fixed » Closed (fixed)

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

DuaelFr’s picture

I had some issues trying to reroll an old patch because no change record has been made for this API change.
I just created the change record. It's my first one so if someone could review it quickly I'd be more confident publishing it.
Thanks.

Berdir’s picture

There is a change record: https://www.drupal.org/node/2457593

It doesn't have the class in the title, but searching for it finds it.

DuaelFr’s picture

Damn... you're right, I'm sorry.
Can you delete mine? It seems to require more rights that I have :'(

Berdir’s picture

I can't, but I can unpublish it again.

benjy’s picture

I deleted it for you.