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

Drupal\system\Tests\Entity\EntityQueryTest is currently failing on SQLite due to case sensitivity differences with MySQL.

SQLite does not have a native ILIKE operator (which could be used as a base for case-insensitive LIKE queries), so we need to overload the non-standard GLOB operator for case-sensitive matching. Another option would have been to overload another non-standard operator, MATCH, but that one does not support the NOT keyword prefix.

CommentFileSizeAuthor
#16 interdiff.txt840 bytesamateescu
#16 2475187-16.patch2.55 KBamateescu
#8 interdiff.txt1.86 KBamateescu
#8 2475187-8.patch2.37 KBamateescu
#3 2475187-3.patch3.11 KBamateescu
#1 2475187.patch3.09 KBamateescu

Comments

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2475187.patch3.09 KB

This was not trivial to figure out, basically we have to implement SQL LIKE in regex in order to make everything work properly :)

Without the patch:

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

Drupal\system\Tests\Entity\EntityQueryTest                   131 passes             1 exceptions             

Test run duration: 12 sec

...

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General
error: 1 near ":db_condition_placeholder_0": syntax error: SELECT
base_table.revision_id AS revision_id, base_table.id AS id
FROM 
{entity_test_mulrev} base_table
INNER JOIN {entity_test_mulrev__field_cs} entity_test_mulrev__field_cs ON
entity_test_mulrev__field_cs.entity_id = base_table.id
WHERE  (entity_test_mulrev__field_cs.field_cs_value LIKE BINARY
:db_condition_placeholder_0) ; Array
(
    [:db_condition_placeholder_0] => o4wtt5ha%
)

With the patch applied:

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

Drupal\system\Tests\Entity\EntityQueryTest                   141 passes                                      

Test run duration: 12 sec

Status: Needs review » Needs work

The last submitted patch, 1: 2475187.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2475187-3.patch3.11 KB

Rerolled on latest HEAD.

damien tournoud’s picture

damien tournoud commented

Unfortunately, this is going to force SQLite to skip the indexes... Do we have a lot of prefix-based LIKE queries?

amateescu’s picture

amateescu commented

You mean NOT LIKE queries? Probably not so many. But the LIKE overloading from the patch is more about having proper case-insensitivity handling for non-ASCII characters.

If we prefer to not care about that, we can just keep the GLOB overloading for case-sensitive LIKE BINARY queries.

damien tournoud’s picture

damien tournoud commented

No, I mean prefix selectors like field LIKE "prefix%", those can be satisfied by a range on an index starting with field. Having a custom implementation will prevent SQLite from doing so.

amateescu’s picture

amateescu commented

Ah, got it. Any entity query that uses the STARTS_WITH condition will be affected: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Entity/Query...

Also views queries that are using the string filter: http://cgit.drupalcode.org/drupal/tree/core/modules/views/src/Plugin/vie...

amateescu’s picture

amateescu commented
Title: Add user-space implementations for LIKE and LIKE BINARY in SQLite » Add a user-space implementation for LIKE BINARY in SQLite
StatusFileSize
new 2475187-8.patch2.37 KB
new interdiff.txt1.86 KB

Ok, so let's add just the LIKE BINARY implementation, which is needed to support case-sensitive conditions. Anyone who uses SQLite should know from the start that they won't have proper UTF-8 support :/

amateescu’s picture

amateescu commented
Assigned: Unassigned » damien tournoud

@DamZ, are you ok with the latest patch that doesn't change anything for LIKE?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This looks great to me, and I think it resolves DamZ's concerns.

Also if we find a better solution later on, that's a great thing to do with a 100% sqlite pass rate.

We only have this problem at all because we rely on LIKE being case-insensitive on MySQL to avoid LOWER() (or extra lowercased columns in the schema), tbh even if that meant not using indexes on sqlite I'd have been OK with it, but great that abusing glob gets around this.

Moving to RTBC but leaving assigned to Damien and I won't personally commit this for a few days - someone else can though.

amateescu’s picture

amateescu commented
Issue summary: View changes

Updated the issue summary since we're not overloading LIKE anymore.

fabianx’s picture

fabianx commented

RTBC + 1 - This is the last remaining SQlite issue, so would be great to get in.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -257,6 +263,21 @@ public static function sqlFunctionRegexp($pattern, $subject) {
+    $pattern = str_replace(array('%', '_'), array('.*?', '.'), preg_quote($pattern, '/'));
+    return preg_match('/^' . $pattern . '$/', $subject);

Can we explain a bit how / why this implementation works?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I pinged Damien via e-mail to raise this issue to his attention.

webchick’s picture

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

And #13 sounds like a needs work for documentation.

amateescu’s picture

amateescu commented
Priority: Normal » Major
Status: Needs work » Needs review
StatusFileSize
new 2475187-16.patch2.55 KB
new interdiff.txt840 bytes

Can we explain a bit how / why this implementation works?

I'm not sure I understand the question, do you have any concern that it will not work? Would something like this be enough for documentation?

fabianx’s picture

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

Looks great to me!

  • catch committed fe2eeca on 
    Issue #2475187 by amateescu: Add a user-space implementation for LIKE...
catch’s picture

catch
he/him
Primary language English
commented
Assigned: damien tournoud » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

@Damien if you have comments on this, please open a follow-up and link here, but to me this is a good approach.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Yeah and worst-case it's only affecting SQLite, not any of the main DB drivers. Hooray!

Side-note: perhaps folks working on these issues would care to nominate themselves for additional PostgreSQL and/or SQLite maintainers? :D

Status: Fixed » Closed (fixed)

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