This patch starts to move out some of our common string functions into a utility class.

My main motivation for this change is make our classes more unit testable. Currently, if a class calls format_string or check_plain, which is very common, it cannot be unit tested with PHPUnit because bootstrap.inc is not included and functions cannot be autoloaded.

I started with check_plain, format_string, and drupal_placeholder, because usage of check_plain and format_string are common in tests and many conversions to PHPUnit are blocked by them.

I moved the tests relating to these functions from XssUnitTest into a dedicted PHPUnit test, and I also added test coverage for String::placeholder (didn't see any before).

CommentFileSizeAuthor
#12 drupal-1938972-12.patch15.37 KBParisLiakos
#10 drupal-1938972-10.patch15.39 KBdawehner
#10 interdiff.txt2.4 KBdawehner
#5 string-1938972-5.patch15.32 KBtsphethean
#4 1938972.patch15.2 KBrobloach
string-util.patch14.03 KBmsonnabaum

Comments

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

This may overlap/collide with #1938670: Convert unicode.inc to \Drupal\Component\Utility\Unicode.

robloach’s picture

robloach
he/him
commented
Issue tags: +PHPUnit Blocker
robloach’s picture

robloach
he/him
commented
robloach’s picture

robloach
he/him
commented
StatusFileSize
new 1938972.patch15.2 KB

Added some docblocks and made use of @dataProvider for checkPlain and format, since we're on PHPUnit now for String.

tsphethean’s picture

tsphethean commented
StatusFileSize
new string-1938972-5.patch15.32 KB

Just a couple of minor docblock changes that my IDE was flagging up - otherwise looking good to me.

robloach’s picture

robloach
he/him
commented
Status: Needs review » Reviewed & tested by the community
dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Performance

Could this additional method calls be a performance problem, hopefully not?

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

We should only use the procedural wrappers during a transitioning phase. We can convert all calls in follow-up issues and remove the functions when we're done with that. Alternatively we can do the conversion in this issue.

ParisLiakos’s picture

ParisLiakos commented

lets replace them in followups, will be much easier

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new interdiff.txt2.4 KB
new drupal-1938972-10.patch15.39 KB

Just a silly doc-style update.

msonnabaum’s picture

msonnabaum commented

The method call is not a significant performance hit at all. Also, it's easily avoidable by calling the class directly for those paranoid about it.

I agree that any talk of removing the procedural wrappers should be done in another thread. I'm not sure I support it, but I dont want that discussion to block these issues.

ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new drupal-1938972-12.patch15.37 KB

chasing HEAD..
had to adjust the use statements after the Timer class went in

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed d9fd65f and pushed to 8.x. Thanks!

Minor fix done during commit... :)

+++ b/core/tests/Drupal/Tests/Component/Utility/StringTest.phpundefined
@@ -0,0 +1,109 @@
+ * Contains \Drupal\Tests\Component\Utillity\StringTest.

Fixed spelling of Utility

Status: Fixed » Closed (fixed)

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

dawehner’s picture

dawehner
Primary language German
commented

Just linking the conversion of filter_xss: #1998466: Convert filter_xss_admin and similar function to an Xss component