Enforced fetching of fields/columns in lowercase breaks third-party integration

That was a messy issue, but this patch should be a straight rollback of the change that caused the actual bug.

#1: attr_rollback.patch queued for re-testing.

Per the previous issue, I don't see why this should be per-query. It should be per-connection, and should be a connection option to disable the case folding.

I'm not sure why that ended up getting left out of the previous patch, but that is, IMO, the correct solution.

I was thinking something along the lines of:

$databases = array(
  // ...
);
$databases['extra']['default'] = array(
  'driver' => 'mysql',
  'user' => 'hello',
  'pass' => 'world',
  'name' => 'mystupiddb',
  'case_folding' => FALSE, // defaults to TRUE
);

Or alternatively we could allow a key to override arbitrary PDO options, although that might cause more trouble than its worth.

As for MySQL and its schema... *expletive deleted*.

Tagging issues not yet using summary template.

I fully support this as I created a similar issue #1315330: entity_load does not respect case on entity keys without finding this one.

It is effectively a pain in the ass for services integration. Here is a work-around for now:

<?php
/**
 * Implements hook_cron().
 */
function wow_guild_cron() {
  $guilds = wow_guild_load_multiple(FALSE);
  foreach ($guilds as $guild) {

    // this is crap... @see: #1171866
    $guild->lastModified = $guild->lastmodified;
    $guild->achivementPoints = $guild->achievementpoints;
    unset($guild->lastmodified);
    unset($guild->achievementPoints);

    try {
      $guild = wow_guild_fetch_guild($guild, array('members', 'achievements'));
      wow_guild_save($guild);
    }
    catch (WoWException $exception) {
      if ($exception->getCode() == 304) {
        // Not Modified. Do nothing.
      }
      else {
        watchdog_exception('wow_guild', $exception);
      }
    }

  }
}
?>

Looks like we need Crell's feedback on approach here.

Now that #1309278: Make PDO connection options configurable is in for Drupal 8, does that change anything here? (Basically item 2 from #12 just happened anyway.)

That patch would have broken the latest patch here anyway, so marking back to needs work.

I tried to review #337926: Force connection with PDO::CASE_LOWER but all I could find is another case of hswong3i's irrational demands. So doing this looks good to me. The backport, alas, will be much less, we can only change schema.inc but we can not change connection options. We can make them configurable yes and that willl help you but the change to database.inc is not backportable.

Configurable connection options is on its way #1309278: Make PDO connection options configurable.

With the connection options configurable now I don't think we need to have the separate "case" option. We just change the default options. People can still change them if they really really need, give or take the warning that they could break everything by doing so. I'm fine at this point with moving back to "native" casing as the default.

As for MySQL, <expletive deleted>. If we have to force lowercase there, we should document inline why we're doing so.

Sun, would you mind pasting an example of what your _mymodule_db_query() functions contain?

Glad this is being fixed for drupal 8, was a right pain in drupal 7

Thanks for that.

Let's just go ahead and do #22. I don't know if we can backport it, but I am OK with doing so if webchick is.

If you remove the case folding in the PDO fetch mode, please be ABSOLUTELY SURE that every database driver will behave consistently with mixed-case column names. This shouldn't affect schema install/uninstall, updates, or insertions; PDO::ATTR_CASE only affects returned result sets, AFAIK. I read somewhere that the PostgreSQL driver itself returns case folded column names, even if the PDO::ATTR_CASE is set to PDO::CASE_NATURAL; note the PDO documentation:

PDO::CASE_NATURAL: Leave column names as returned by the database driver.

I am pleased enough to see that the database connection is being set to intentionally case fold in a consistent manner with the current setup. Not sure why the options are at the driver level since it's a PDO function and not a driver function. The tables are created with the correct case, and I just need to use case folded column names when fetching data.

This looks a bit too scary for committing this close to 7.13. I can take another look after Wednesday.

This patch is against 8.x anyway, so it has to go there first. :-) Once that's done let us know if you're OK with a D7 backport and we can get xjm and her minions to handle that.

I've committed/pushed this to 8.x. Moving to 7.x for backport. I think it makes sense to commit this just after the next stable 7.x release rather than just before too.

Unassigning Crell since he's signed off on this, and tagging novice for the backport.

I realise this has been signed-off, but for the record here are a few checks and verifications I did last week (hoping to add confidence to the backport):

• fix does not break automated testing. Seems a good case for relying on automated testing, as it will be far more efficient than me in testing this. We can have confidence that at least one core test would break if this were introducing issues
• checked simpletest codebase for core tests that might invoke the issue ... looking for anything that checks an index; general database tests ... YES there are tests that explicitly check indexExists in schema.test and field_sql_storage
• run those simpletests and read through what they test ... All passed
• used a modified version of the example script to test behaviour .... looks good, works as designed on mysql/pdo
• how would it break contrib?
  • I can't think of any reasonable behaviour in contrib that would break with this patch. If anything in contrib breaks with this patch, then that needs fixing in contrib.
• check backporting to D7 ...
  • key_name change should be the same: it's OK
  • database.inc change was noted by chx as not backportable because of the fact that it changes case on a per-query basis which we're not doing here. Should be ok
  • The main changes should have the same effect in D7 - they all have easily corresponding points in D7 code
• checked for instances on d8. There are 3 of them, one for each of mysql, pgsql and sqlite
  • checked for all 3 instances covered in the patch ... looks good
  • apply patch and check for instances still ... looks good
• TODO do we need it testing on pgsql and sqlite?
• TODO backport latest to D7 - IMO backport should be harmless: see above

Attaching the backport ...

Long Beach Drupal Meetup is going to test the D7 patch tonight, March 28. We have 5 members working on this. 3 experienced, and 2 novice.

rtbced #32

Tested patch 32 and verified with help from btmash.

Used this code to create table:
http://drupalbin.com/21097

then added a data to the table and pulled the table structure with:
debug(db_query("SELECT * FROM {foo} where formatId = 0")->fetchObject());

RTBCED #32 wrapping up the Long Beach Contrib Meeting. We had 3 members test the same way, on 2 different OS (linux and mac). I tested on Gentoo with PHP 5.2.17 and mysql 5.1.56. I did test using a lower case name for the SELECT Frank posted in #34. It worked.

Ashok points out this patch could be reviewed in D7 using sqlite, SQL, and I thought what my MySQL was switched to case sensitive mode for field names (does it still have that option?). Ashok's opinion was the code was all the same for all these databases, and should work.

@frob/@bvirtual: wow, that's some seriously efficient reviewing work going on there :) Nice work ...

@bvirtual: did you intend to switch the status back? That looks to me like it may have been a concurrent edit.

Sweet jebus that's a lot of testing. :-)

Different databases have different case folding rules. Technically this patch reverts back to "let the database do whatever it's going to do", for better or worse. So a quick verification on SQLite and Postgres that nothing explodes is probably a good idea, especially for D7. A quick run through some subset of the database unit tests should be sufficient, since otherwise testing SQLite and Postgres takes a LONG time. :-(

Couple of odd fails on D8/postgresql. Will come back to them once I've been through the rest. Given it's 2/2.5K fails/success, I doubt it's caused by this patch.

RESULTS
2688 passes, 1 fail, 1 exception, and 81 debug messages

Table test_task created successfully.	Other	database_test.test	63	DatabaseTestCase->installTables()	
PDOException: SQLSTATE[42803]: Grouping error: 7 ERROR: column "test.age" must appear in the GROUP BY clause or be used in an aggregate function LINE 3: (SELECT test.age AS age, age + 1 AS expression, 1 AS express... ^: SELECT COUNT(*) AS expression FROM (SELECT test.age AS age, age + 1 AS expression, 1 AS expression_2 FROM {test} test HAVING (age + 1 > 0) ) subquery; Array ( ) in PagerDefault->execute() (line 77 of /Users/alan/Sites/d8/core/includes/pager.inc).	Uncaught exception	Statement.php	58	PDOStatement->execute()	
Enabled modules: database_test	Other	database_test.test	34	DatabaseTestCase->setUp()	
Table test created successfully.	Other	database_test.test	63	DatabaseTestCase->installTables()	

Table test_task created successfully.	Other	database_test.test	63	DatabaseTestCase->installTables()	
FAIL Number of affected rows are returned.	Other	database_test.test	818	DatabaseUpdateTestCase->testExpressionUpdate()	

Ran all of database and Field API (for field_sql_storage). The fails are reliable.

D8 sqlite:
2664 passes, 2 fails, 1 exception, and 81 debug messages

Table test_task created successfully.	Other	database_test.test	63	DatabaseTestCase->installTables()	
FAIL The whole transaction is rolled back when a duplicate key insert occurs.	Other	database_test.test	3185	DatabaseInvalidDataTestCase->testInsertDuplicateData()	
The rest of the insert aborted as expected.	Other	database_test.test	3198	DatabaseInvalidDataTestCase->testInsertDuplicateData()	

Table test_task created successfully.	Other	database_test.test	63	DatabaseTestCase->installTables()	
PDOException: SQLSTATE[HY000]: General error: 1 a GROUP BY clause is required before HAVING: SELECT COUNT(*) AS expression FROM (SELECT test.age AS age, age + 1 AS expression, 1 AS expression_2 FROM {test} test HAVING (age + 1 > 0) ) subquery; Array ( ) in PagerDefault->execute() (line 77 of /Users/alan/Sites/d8/core/includes/pager.inc).	Uncaught exception	Connection.php	243	PDO->prepare()	
Enabled modules: database_test	Other	database_test.test	34	DatabaseTestCase->setUp()	

Enabled modules: field_sql_storage, field, field_test, text, number	Other	field_sql_storage.test	26	FieldSqlStorageTestCase->setUp()	
FAIL Update succeeded.	Other	field_sql_storage.test	323	FieldSqlStorageTestCase->testFieldUpdateFailure()	
Table field_data_test_text exists.	Other	field_sql_storage.test	331	FieldSqlStorageTestCase->testFieldUpdateFailure()	

The exception is the same between sqlite/postgresql, the fails are different.

D8/sqlite tests fail in the same places when patch is reverted, so no regression here ...

D8/pgsql also has the same fails when reverted, so ok there too...

Will come back to the actual fails/exception in a separate issue.

Prior to patching, pgsql shows similar fails on d7

PDOException: SQLSTATE[42803]: Grouping error: 7 ERROR: column "test.age" must appear in the GROUP BY clause or be used in an aggregate function LINE 3: (SELECT test.age AS age, age + 1 AS expression, 1 AS express... ^: SELECT COUNT(*) AS expression FROM (SELECT test.age AS age, age + 1 AS expression, 1 AS expression_2 FROM {test} test HAVING (age + 1 > 0) ) subquery; Array ( ) in PagerDefault->execute() (line 74 of /Users/alan/Sites/d7/includes/pager.inc). Uncaught exception database.inc 2136 PDOStatement->execute()

Number of affected rows are returned. Other database_test.test 810 DatabaseUpdateTestCase->testExpressionUpdate()

And sqlite also:

PDOException: SQLSTATE[HY000]: General error: 1 a GROUP BY clause is required before HAVING: SELECT COUNT(*) AS expression FROM (SELECT test.age AS age, age + 1 AS expression, 1 AS expression_2 FROM {test} test HAVING (age + 1 > 0) ) subquery; Array ( ) in PagerDefault->execute() (line 74 of /Users/alan/Sites/d7/includes/pager.inc). Uncaught exception database.inc 240 PDO->prepare()

Update succeeded. Other field_sql_storage.test 321 FieldSqlStorageTestCase->testFieldUpdateFailure()

So, first of all looking that no new fails are introduced by the patch...

sqlite shows the same fails as prior to patching.

pgsql also.

I'll check again what mysql has to say about the couple of fails I've seen, but I'm not expecting to see any problems. I think we're probably good here.

FWIW all clear on that same small subset of tests D7/8 MySQL

So... if I understand Alan Evans correctly, SQLite and Postgres are broken now in D7 and D8, and are no more or less broken with this patch in either D7 or D8. Correct?

If that's the case, then I think we can go ahead and commit the backport to D7 (webchick-permitting) and then separately try and figure out why those DBs are broken again.

Right - MySQL is all clear with the patch (which we know anyway from the testbot), sqlite and postgres show the same fails/exceptions with and without the patch. I haven't run a full battery of tests on either to see what else might be lurking, just all of "Database" and "Field API" groups.

My first guess on the PDO Exception is just some MySQL-specific SQL within the test. The failed field API update is more of a concern. The nested transaction issue is also not good. I'll report them separately. On postgres I'd also wonder if there's a server version-specific issue involved (I just downloaded and installed the latest).

OK, then I think the D7 version of this is ready. Thanks Alan and crew for the super-detailed testing!

Thanks Crell!

Agreed; thank you so much for all of your great testing!

Committed and pushed to 7.x.

Yay, I can finally remove the only core hack on one of my sites :)

Thanks for all the hard work.

Great, now I have to go through all my code and change the case folded names back to normal.

This is a very disruptive core change. This breaks all migrations that use the migration module and sql data sources with column names that use uppercase letters. I would have considered this more of a questionable architectural decision than a bug. If your sql migrations are broken after an update to 7.14 as mine were, this is likely the reason. You will need to modify your migrations to use the proper source column names with capital letters where before this change you were required to convert all column names to lowercase. I would expect that this change will break a lot of custom integration code that relies on DBTNG to access legacy or 3rd party databases.

Yes, from the migration standpoint (and if you check the original issue, that's not the only reason for this change) this is going to be disruptive in the short term. But think back to when you were first implementing those migrations from a mixed-case DB - how long did it take you, after initially doing the obvious thing and using the original case, to figure out you had to lower-case everything? Didn't you say, WTF;)? Future generations of migrators will be grateful to express their mappings in a natural way.

Since users of the Migrate module are most likely to run into trouble with this upgrade, I've added a note to the project page. Hopefully at least some people will be forewarned before upgrading.

I did say WTF, but then just assumed that it was an arbitrary decision to default to PDO::CASE_LOWER and dealt with it. I wrote a number of migrations from many camelcased tables and was only alterted to the fact that this was considered a bug when my migrations broke with a core update. My only suggestion is that this late in the game (7.14) it may have been better to:

1) Keep the PDO::CASE_LOWER default in D7.
2) Provide an option in D7 to change to CASE_NATURAL programtically.
3) Change the default to CASE_NATURAL in D8.

Oh I thought it was just going to be optional, maybe youcould define a constant in settings.php and use that for the pdo case ???

I think you can opt for pdo options in the db conf array in settings.php in 7.14 and possibly per query.

That said; This change did come as a nasty surprise though as it was quite buried in the release notes:

// ... lots of changes
#1171866 by sun, catch, Alan Evans, bvirtual, frob, btmash: Fixed Enforced fetching of fields/columns in lowercase breaks third-party integration.
// ... lots of changes

Additional communication about the change (its presence, impact, workarounds) in the release announcements and additional docs similar to a Drupal 6.1 API change would have been nice.

I agree entirely with Heine on this one. This change broke backup and migrate among other things. Had no idea where to look to deal with this. #1576812: Could not complete the backup.

Well, I definitely welcome any and all help keeping an eye on the RTBC queue/commit log to spot things that break backwards compatibility. We never intentionally break APIs.

Yes this could have done with a change notice, re-opening for that.

For reference: ("Known issues" at the release statement) http://drupal.org/drupal-7.14

Regarding the drupal-7.14 release notes, they currently say:

Solution for Migrate is to rename tables in scripts back to their proper names.

Actually, it's not the table names, it's field names in mappings. Better:

The solution for Migrate is to name source fields in mappings to match their case in the source database.

I'm new to this thread and realize it's already closed. Still, I want to express my emphatic agreement with comment #54. A change that will obviously break modules should be in a major release. What is this, drupal 4.x?

I have a module broken by this change. As far as I can tell, I need to create a new branch to support < 7.14 vs >=7.14.