Split Utility\String class to support PHP 7 (String is a reserved word)

How about DrupalString?

and for DataType StingData.

Maybe just Strings

The 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.

Oh. Maybe we can use http://php.net/manual/en/function.class-alias.php to keep BC for String?

What 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.

I'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.

Fix stuff.

Looks good!

-    $href = '<' . String::checkPlain($attributes['href']) . '>;';
+    $href = '<' . StringHelper::checkPlain($attributes['href']) . '>;';

Oof. :( 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

I 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?

100% agreed with every word in #19. That's exactly what I would've written :)

I 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::

Initially I though on Strings, but +1 on #21
Also suppose the title is better

Its 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

Bumping 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.

Suppose the split should be like that.
Also it makes sense to split the issue on parts for example on per module basis

And split the tests.

remove core usage and clean-up

I'm certainly in favour of making things sane, so here is a suggestion:

• Have one issue to fix the critical issue: Rename String => StringHelper
• Have a dedicated discussion how to make things sane, and keep StringHelper as BC layer
• Convert the instances from StringHelper onto the same API

There'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.

@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.

b) Go with the patch in #28. Maybe already convert the method calls here, maybe not.

Alright, 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?

+1 to #28, could improve performance quite a lot.

+++ b/core/lib/Drupal/Component/Utility/String.php
@@ -27,45 +24,32 @@ class String {
+   * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
+   *   Use \qDrupal\Component\Utility\Html::decodeEntities() instead.

Me found a typo

fixed

Wow, you were faster than me @andypost. Anyway I fixed a few nitpicks.

Ok everyone agrees, lets get this in asap so everyone can update their code once again ;)

Whoopsie, we need a change record first...

Created one

Back to RTBC.

RTBC + 1

This 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!