Problem/Motivation

GlossaryTest fails currently with PostgreSQL as database backend. This is because some tests expect case-insensitive results but that is currently not working for PostgreSQL.

Proposed resolution

Support case-insensitive queries by using LOWER() and some other magic.

Remaining tasks

Write patch

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#12 interdiff.txt791 bytesbzrudi71
#12 2451749-12.patch2.76 KBbzrudi71
#8 glossary_test.txt10.6 KBbzrudi71
#7 interdiff.txt1.16 KBamateescu
#7 2451749-5.patch2.23 KBamateescu
#4 2451749-4.patch2.24 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented
Title: PostgreSQL: Fix views/src/Tests/GlossaryTest.php » PostgreSQL: Fix views\src\Tests\GlossaryTest.php
amateescu’s picture

amateescu CreditAttribution: amateescu commented
Related issues: +#2462175: SQLite: Fix case sensitivity in Views' string argument plugin, +#1518506: Normalize how case sensitivity is handled across database engines

This is another occurrence of case sensitivity hell. I fixed it for SQLite in #2462175: SQLite: Fix case sensitivity in Views' string argument plugin but only because we managed to implement a user-space collation in #2454733: Add a user-space case-insensitive collation to the SQLite driver.

The fix here will probably depend on the outcome of #1518506: Normalize how case sensitivity is handled across database engines.

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented

Thanks amateescu! Very useful information, saves the PG-Team from debugging :-)

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Active » Needs review
FileSize
2451749-4.patch2.24 KB

@dawehner asked me about this issue and I think we can do something like this for PostgreSQL. Can you please try to see if it works? :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner for Drupal Association commented

The good looks great for me!

@bzrudi71
Now its up to you to say, whether this fixes the tests.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner for Drupal Association commented

The good looks great for me!

@bzrudi71
Now its up to you to say, whether this fixes the tests.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
FileSize
2451749-5.patch2.23 KB
interdiff.txt1.16 KB

Fixing the comments.

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented
FileSize
glossary_test.txt10.6 KB

Thanks so much @amateescu and @dawehner for helping out here. Unfortunately tests still fail with patch applied :-( I already tried a lot of very similar approaches to force case-insensitive in StringArgument.php (like amateescu did in the SQLite fix). I think there *might* be something wrong outside StringArgument.php that we miss, or we have overseen another problem within StringArgument.php ;-)

amateescu’s picture

amateescu CreditAttribution: amateescu commented

What would help a lot is if we can take a look at the query generated by the view with this patch applied. That should allow us to see what we're missing :)

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented

Guess we are talking about $view->build_info['query']?

Interested how that compares to SQLite or MySQL:
SELECT LOWER(SUBSTRING(node_field_data.title, 1, 1)) AS title_truncated, COUNT(node_field_data.nid) AS num_records, MIN(node_field_data.nid) AS nid, MIN(users_field_data_node_field_data.uid) AS users_field_data_node_field_data_uid FROM {node_field_data} node_field_data LEFT JOIN {users_field_data} users_field_data_node_field_data ON node_field_data.uid = users_field_data_node_field_data.uid GROUP BY title_truncated ORDER BY title_truncated ASC NULLS FIRST

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented

Okay, just another update after another hour of debugging. With patch applied I can get 100% pass by additionally:

1. force HandlerBase::caseTransform() to return lowercase in case of an 'upper' request
That fixes the current fails because of the 'unwanted' cache tags.
2. As soon as we have pass here we need another change in GlossaryTest around line 93 within the foreach loop to force the '$label' value to be lowercase instead of uppercase.

With that test passes but we need to check why this is needed and how to come around :) I'm finished for today so if someone else likes to have a look your invited!

[UPDATE] As a last feedback for today, the patch isn't required at all. With just the above changes we have 100% pass.

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented
FileSize
2451749-12.patch2.76 KB
interdiff.txt791 bytes

Sometimes it's better to have some sleep :) I found the missing part.

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented
Issue summary: View changes

Updated issue summary. Just to make sure I did a complete bot run with patch applied and don't see any new fail or exception!

jaredsmith’s picture

jaredsmith CreditAttribution: jaredsmith at Bluehost commented
Status: Needs review » Reviewed & tested by the community

I've reviewed the patch in comment 12, and re-run the DrupalCI testing on PostgreSQL 9.1

The patch solves the outstanding issues in the GlossaryTest test. It causes no additional regressions in the Views test group.

Before the patch:
Drupal\views\Tests\GlossaryTest 30 passes 3 fails 6 messages

After the patch:
Drupal\views\Tests\GlossaryTest 33 passes

Marking as RTBC, as I think this is ready to be reviewed by a core committer.

catch’s picture

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

Is this not fixable at the database driver level? Baking the logic into Views means a contrib database driver has no way to get around the same problem.

jaredsmith’s picture

jaredsmith CreditAttribution: jaredsmith at Bluehost commented

Is this not fixable at the database driver level?

Case-insensitive matching at the database or database-driver level is being discussed in #2464481: PostgreSQL: deal with case insensitivity, but I don't think that there's any consensus yet on how to best address that, especially when we probably don't always want to do case-insensitive searches across all text fields.

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented
Status: Needs work » Needs review

Setting back to needs review, because I think we have a follow up for a fix at database level with #2464481: PostgreSQL: deal with case insensitivity.

daffie’s picture

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

I agree with jaredsmith's reply in comment #16.
The patch looks good to me. So back to RTBC.

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented

@daffie thanks for review and nice to see you back again! Would you mind looking over the other open issues and leave some feedback to help moving forward there :)

alexpott’s picture

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

Given that we permitted SQLite to fix this in Views I think we should permit postgres too. We have the followup #2464481: PostgreSQL: deal with case insensitivity in place to try to address case sensitivity issue better.

Committed 7786d91 and pushed to 8.0.x. Thanks!

  • alexpott committed 7786d91 on 8.0.x 
    Issue #2451749 by amateescu, bzrudi71, jaredsmith: PostgreSQL: Fix views...

Status: Fixed » Closed (fixed)

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

david_garcia’s picture

david_garcia CreditAttribution: david_garcia commented

When I came accross this I thought it was some very old code that needed fixing... just to find out this was commited very recently.

    $formula = "SUBSTRING($this->tableAlias.$this->realField, 1, " . intval($this->options['limit']) . ")";

    if ($this->options['case'] != 'none') {
      // Support case-insensitive substring comparisons for SQLite by using the
      // 'NOCASE_UTF8' collation.
      // @see Drupal\Core\Database\Driver\sqlite\Connection::open()
      if (Database::getConnection()->databaseType() == 'sqlite') {
        $formula .= ' COLLATE NOCASE_UTF8';
      }

      // Support case-insensitive substring comparisons for PostgreSQL by
      // converting the formula to lowercase.
      if (Database::getConnection()->databaseType() == 'pgsql') {
        $formula = 'LOWER(' . $formula . ')';
      }
    }
    return $formula;

Why don't we ask the database abstraction layer how to do this instead of hardcoding it?

Having this kind of code diminishes drupal's portability.