Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
String will be a reserved word in PHP 7. We could easily allow support for PHP 7 by simply renaming core/lib/Drupal/Component/Utility/String
Proposed resolution
Split class methods into:
1) move decodeEntities()
into \Drupal\Component\Utility\Html
class
2) other methods to \Drupal\Component\Utility\SafeMarkup
class
Remaining tasks
Decide on change record.
See https://github.com/grom358/d8codetools for a way to easily rename a function across all files.
User interface changes
no
API changes
Deprecate all methods in \Drupal\Component\Utility\String
String::decodeEntities() |
Html::decodeEntities() |
String::checkPlain() |
SafeMarkup::checkPlain() |
String::format() |
SafeMarkup::format() |
String::placeholder() |
SafeMarkup::placeholder() |
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff.txt | 1.24 KB | rteijeiro |
#36 | 2454447-string-36.patch | 25.69 KB | rteijeiro |
#23 | 2454447-23.patch | 672.64 KB | dawehner |
Comments
Comment #1
jibranHow about DrupalString?
Comment #2
jibranand for DataType StingData.
Comment #3
andypostMaybe just
Strings
Comment #4
andypostComment #5
BerdirThe plugin classes are no issue, that shouldn't break anything except if someone subclassed theme.
But Utility\String is going to be painful, String::checkPlain() has 255 calls in core alone.
Comment #6
BerdirOh. Maybe we can use http://php.net/manual/en/function.class-alias.php to keep BC for String?
Comment #7
benjy CreditAttribution: benjy at CodeDrop commentedLets make this issue about Utility\String which is going to be a big enough patch anyway, i'll open some new issues for the views string classes.
Comment #8
benjy CreditAttribution: benjy at CodeDrop commentedComment #9
benjy CreditAttribution: benjy at CodeDrop commentedNew issue for the views String classes #2455415: Rename Views plugin classes to support PHP 7
Comment #10
dawehnerWhat about naming it simply StringHelper, given that the documentation of the class says, that this is the purpose of it:
Encodes special characters in a plain-text string for display as HTML.
Comment #11
dawehnerI'm curious how big such a patch can become.
Note: even if we decide on a better name this is much easier with that patch applied, given that StringHelper is unique enough.
Comment #13
dawehnerFix stuff.
Comment #14
tim.plunkettLooks good!
Comment #15
benjy CreditAttribution: benjy at CodeDrop commented+1 for StringHelper.
Comment #16
benjy CreditAttribution: benjy at CodeDrop commentedDo we need a change record here?
Comment #17
webchickOof. :( That's much more verbose/weird. I guess it has parallels with UrlHelper, which is also verbose/weird, but the difference is that's not called almost 1,000 times in core.
Any chance we can cheat and just call it "Strings"? :P
Comment #18
benjy CreditAttribution: benjy at CodeDrop commentedI think there is a lot of personal preference here but I personally feel that StringHelper makes more sense than "Strings" even if it's a touch longer.
Comment #19
BerdirI think String was always a pretty strange class/name.
What do checkPlain(), decodeEntities() and format() have in common? Exactly, nothing. They operate on strings, but the same is true for Html (which would be a much better place for decodeEntities()), Xss (which would be a much better place for checkPlain()), Unicode, UrlHelper and so on.
Except those didn't exist when we added this, I guess it was one of the first: #1938972: Start moving string functions into a utility class (Xss.php was a month later..)
So the simple truth I guess is that StringHelper is as bad as String is right now, but we won't be able to come up with a good name, because there can't be a good name given that those methods have nothing in common.
Unless we actually decide to fix this problem by moving those methods to different classes*, I guess it doesn't matter that much, StringHelper works for me, and as mentioned, is consistent with UrlHelper.
* Not sure how viable that is, and also not sure where to move format() to, if we would go with this.
PS: Another reminder about the BC layer, do we want to do this or not?
Comment #20
Wim Leers100% agreed with every word in #19. That's exactly what I would've written :)
Comment #21
Fabianx CreditAttribution: Fabianx commentedI would be all for merging this into the SafeMarkup class as that is all related to it, and would help performance quite a bit as then all the filters could avoid function calls to SafeMarkup::isSafe(), but use an isset on the static protected class member.
Especially because we generally have been going into the direction of making all escaping auto-escaping in e.g. String::format.
We probably would need to leave a BC layer, but that should be fine ...
And yes move decodeEntities to Html::
Comment #22
andypostInitially I though on Strings, but +1 on #21
Also suppose the title is better
Comment #23
dawehnerIts tricky, given that in case we want to move String::format into something else a lot of code would need some form of additional treatment if you update.
Fixed the test file in the meantime
Comment #24
benjy CreditAttribution: benjy at CodeDrop commentedIf we're going to split the methods out, I guess now would be the time. We don't want to make another API break in the future so I guess we need to make a decision on that.
For what it's worth, I'd be in favour of moving the methods into more relevant classes if that's an option that's likely to get committed.
Comment #25
catchBumping to critical, https://wiki.php.net/rfc/php7timeline looks like PHP 7 will be out before 8.1.0 even if not before 8.0.0.
I'd personally be OK with an API change here, including splitting the class to make more sense - it's not going to be any more of an API change to split the class up really, and in this case we copied some related functions from common.inc to a class. On the other hand we want to have the new API available asap since this will be widespread even if it's easy to make, so would be good not to get stuck on naming for too long.
Leaving the old class in for ~two betas also seems good - that gives plenty of warning before anything breaks.
Permanent dynamically defined BC-layer, that sounds like more trouble than it's worth.
Comment #26
andypostSuppose the split should be like that.
Also it makes sense to split the issue on parts for example on per module basis
Comment #27
andypostAnd split the tests.
Comment #28
andypostremove core usage and clean-up
Comment #29
dawehnerI'm certainly in favour of making things sane, so here is a suggestion:
Comment #30
ianthomas_ukThere's a script at https://github.com/grom358/d8codetools which makes it easy to move/rename a function across the codebase, once we've decided where we want them to go.
Comment #31
Berdir@dawehner: That means we would have two API change. I don't think that makes sense and that we can qualify that.
I think we have two options:
a) Rename to StringHelper.
b) Go with the patch in #28. Maybe already convert the method calls here, maybe not.
I'd prefer b), with a follow-up to remove String in 1-2month.
Comment #32
dawehnerAlright, so let's do that. I'm curious whether we should do the conversion of the method calls per message, so its really easy to review?
Comment #33
Fabianx CreditAttribution: Fabianx commented+1 to #28, could improve performance quite a lot.
Comment #34
aspilicious CreditAttribution: aspilicious commentedMe found a typo
Comment #35
andypostfixed
Comment #36
rteijeiro CreditAttribution: rteijeiro commentedWow, you were faster than me @andypost. Anyway I fixed a few nitpicks.
Comment #37
aspilicious CreditAttribution: aspilicious commentedOk everyone agrees, lets get this in asap so everyone can update their code once again ;)
Comment #38
aspilicious CreditAttribution: aspilicious commentedWhoopsie, we need a change record first...
Comment #39
andypostComment #40
dawehnerCreated one
Comment #41
dawehnerBack to RTBC.
Comment #42
Fabianx CreditAttribution: Fabianx commentedRTBC + 1
Comment #43
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 40d6efe and pushed to 8.0.x. Thanks!
Comment #45
benjy CreditAttribution: benjy at CodeDrop commentedShould these have been done here? Follow-up?
Comment #46
benjy CreditAttribution: benjy at CodeDrop commentedAhh Berdir pointed me to the two follow-ups, cheers.
Comment #47
andypost