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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented

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

RobLoach’s picture

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

RobLoach
he/him
CreditAttribution: RobLoach commented
RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
FileSize
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 CreditAttribution: tsphethean commented
FileSize
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
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner 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
CreditAttribution: Xano 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 CreditAttribution: ParisLiakos commented

lets replace them in followups, will be much easier

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt2.4 KB
drupal-1938972-10.patch15.39 KB

Just a silly doc-style update.

msonnabaum’s picture

msonnabaum CreditAttribution: 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 CreditAttribution: ParisLiakos commented
FileSize
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 🇪🇺🌍
CreditAttribution: alexpott 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
CreditAttribution: dawehner commented

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