Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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 :
- String::checkPlain() moved to SafeMarkup::checkPlain()
- String::format() moved to SafeMarkup::format()
- 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
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff.txt | 1.73 KB | stefan.r |
#42 | 2457887-42.patch | 610.97 KB | stefan.r |
#41 | interdiff.txt | 4.3 KB | stefan.r |
#41 | 2457887-41.patch | 611.47 KB | stefan.r |
#37 | 2457887-37.patch | 616.68 KB | rpayanm |
Comments
Comment #1
prateekMehta CreditAttribution: prateekMehta commentedComment #2
dawehnerMaybe its worth trying out https://github.com/grom358/d8codetools for this huge change.
Comment #3
AjitSWorking 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?
Comment #4
alexpottPlease don't remove the methods in this issue. Also we need to attach this issue to the CR.
Comment #5
prateekMehta CreditAttribution: prateekMehta commentedMoving all usages of String::* to SafeMarkup::*
replacing Drupal\Component\Utility\String Drupal\Component\Utility\SafeMarkup or
Adding Drupal\Component\Utility\SafeMarkup wherever required.
Comment #7
prateekMehta CreditAttribution: prateekMehta commentedRerolling , and not removing methods as suggested in #4.
Comment #9
prateekMehta CreditAttribution: prateekMehta commentedRemoving Syntax Errors.
Comment #10
prateekMehta CreditAttribution: prateekMehta commentedSorry wrong patch.
Comment #13
prateekMehta CreditAttribution: prateekMehta commentedMaking changes to .inc , .module , .twig etc files.
Comment #15
prateekMehta CreditAttribution: prateekMehta commentedMissed at aq few more places, sorry for negligence.
Comment #17
prateekMehta CreditAttribution: prateekMehta commentedAdding Utility/SafeMarkup where required.
Comment #19
AjitS@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?
Comment #20
prateekMehta CreditAttribution: prateekMehta commented@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.
Comment #21
dawehner#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
Comment #22
alexpottUsages like the following still exist. And please post interdiffs - see https://drupal.org/documentation/git/interdiff.
Comment #23
prateekMehta CreditAttribution: prateekMehta commentedMaking changes as mentioned in #22 ,
Comment #24
root_brute CreditAttribution: root_brute commentedLooks good :)
Comment #25
alexpottI don't think these use statements are needed at all.
No need for the alias.
Comment #26
prateekMehta CreditAttribution: prateekMehta commentedyes. but they were already there so i didn't remove them.
Comment #27
alexpott@prateekMehta then why did you change them?
Comment #28
prateekMehta CreditAttribution: prateekMehta commentedRemoving alias suggested in #25. Hope nothing else needs to be done.
Comment #29
prateekMehta CreditAttribution: prateekMehta commentedInterdiff 23 - 27, probably the correct way to upload an interdiff.
Comment #30
prateekMehta CreditAttribution: prateekMehta commented@alexpott Using Utility/SafeMarkup as UtilitySafeMarkup , looked more logical than using it as UtilityString. I have removed aliases anyway.
Comment #31
alexpottStill have a UtilitySafeMarkup in this line. Hope fully the tests would fail if this had been set to needs review.
Comment #32
sidharrell CreditAttribution: sidharrell commentedComment #34
prateekMehta CreditAttribution: prateekMehta commentedRemoving it.
Comment #36
prateekMehta CreditAttribution: prateekMehta commentedre-rolling and posting an interdiff.
Comment #37
rpayanmAnother occurrence on docblocks.
Comment #38
stefan.r CreditAttribution: stefan.r commentedI 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:
Do we need to link to both the class and to one of the methods?
Could use a newline after the namespace statement.
Could use a newline after the namespace statement.
Looks like this may not be needed?
Comment #39
BerdirChanging parent, so all these issues are visible in the php7 meta issue.
Comment #40
Mile23Needs reroll.
Comment #41
stefan.r CreditAttribution: stefan.r commentedreroll
Comment #42
stefan.r CreditAttribution: stefan.r commentedAnd this addresses comments in #38.
Comment #43
Mile23All the
String::placeholder()
,String::checkPlain()
, andString::format()
are gone.Can't find any results for array_map()-style calling, either.
Looking good. :-)
Comment #44
BerdirThis is part of the critical PHP7 meta issue. We must do this to be able to remove the String class.
Comment #45
webchickOK, 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!
Comment #47
Mile23This 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.
Comment #48
stefan.r CreditAttribution: stefan.r commentedString is a reserved word in PHP7 so this was critical because it's required for PHP7 support :)
Comment #49
Mile23An easy level of follow-through to finish the job of deprecation, is what I'm saying.
Comment #51
DuaelFrI 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.
Comment #52
BerdirThere 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.
Comment #53
DuaelFrDamn... you're right, I'm sorry.
Can you delete mine? It seems to require more rights that I have :'(
Comment #54
BerdirI can't, but I can unpublish it again.
Comment #55
benjy CreditAttribution: benjy at CodeDrop commentedI deleted it for you.