This issue is part of #2454513: [meta] Make Drupal 8 work with SQLite.

The test case Drupal\taxonomy\Tests\TermTest is currently failing on SQLite.

CommentFileSizeAuthor
#11 2454733-followup-11.patch1.56 KBamateescu
#10 2454733-followup.patch613 bytesamateescu
#4 2454733.patch2.72 KBamateescu

Comments

amateescu’s picture

amateescu commented
Parent issue: #2454731: SQLite: Fix search\Tests\SearchRankingTest » #2454513: [meta] Make Drupal 8 work with SQLite
bzrudi71’s picture

bzrudi71 commented
Related issues: +#1518506: Normalize how case sensitivity is handled across database engines

@amateescu, please see #1518506: Normalize how case sensitivity is handled across database engines before spending to much time trying to fix this. I spend an hour to fix this for PG before I found this issue ;-)

amateescu’s picture

amateescu commented

Thanks for the pointer, I was already following that issue :)

amateescu’s picture

amateescu commented
Title: SQLite: Fix taxonomy\Tests\TermTest » Add a user-space case-insensitive collation to the SQLite driver
Status: Active » Needs review
StatusFileSize
new 2454733.patch2.72 KB

This was not very straightforward because the PDO sqliteCreateCollation() method is not documented, yet it exists, it works just like it's non-PDO version (SQLite3::createCollation()) and we can use it :)

Using PHP's built-in strcasecmp() for the comparison function would probably be faster, but it doesn't support multi-byte strings so we have to implement our own mb-safe variant.

Test run before:

Test summary
------------

Drupal\taxonomy\Tests\TermTest           330 passes   2 fails       73 messages

Test run duration: 1 min 45 sec

After:

Test summary
------------

Drupal\taxonomy\Tests\TermTest           332 passes                 73 messages

Test run duration: 1 min 44 sec
chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

Wow. This doesn't murder performance?? I would've thought it does. But then again I guess SQLite won't be used with large datasets? This is truly a wow issue. The method was added to PHP 5.3 and 5.4 in https://bugs.php.net/bug.php?id=55226 so yes it's good to use.

However, strcmp? That's a bit odd although I guess we can rely on that Unicode is created sanely , ie that A < B even if "A" and "B" are characters in a non Latin ABC.

Also, I have added this function to the PHP manual so that the next guy will have an easier job.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed ed6b882 on 8.0.x 
    Issue #2454733 by amateescu: Add a user-space case-insensitive collation...
chx’s picture

chx commented

The documentation is now live at http://php.net/manual/en/pdo.sqlitecreatecollation.php . Thanks amateescu for showing it exists.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/lib/Drupal/Component/Utility/Unicode.php
@@ -542,6 +542,22 @@ public static function truncate($string, $max_length, $wordsafe = FALSE, $add_el
+  public static function strCaseCmp($str1 , $str2) {
+    return strcmp(mb_strtoupper($str1, 'utf-8'), mb_strtoupper($str2, 'utf-8'));
+  }

Shouldn't this use static::strtoupper(), because this just added a hardcoded dependency on the mbstring extension?

amateescu’s picture

amateescu commented
Status: Fixed » Needs review
StatusFileSize
new 2454733-followup.patch613 bytes

I thought about that at some point but I think I got distracted by another issue and I forgot to correct it :/

amateescu’s picture

amateescu commented
StatusFileSize
new 2454733-followup-11.patch1.56 KB

While we're there, let's also update its name to match that of the PHP function, like the other methods in the Unicode class. @amateescu--

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

Still good. I was so shocked by the existence of the method and the testing time not exploding that I didn't catch this.

alexpott’s picture

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

Committed 2492b80 and pushed to 8.0.x. Thanks!

  • alexpott committed 2492b80 on 8.0.x 
    Issue #2454733 followup by amateescu: Add a user-space case-insensitive...

Status: Fixed » Closed (fixed)

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