This makes Drupal code more SQL cross compatible as it allows for less mysqlisms.

mysql> insert into simpletest386692test (id) values (21);
Query OK, 1 row affected, 1 warning (0.00 sec)
mysql> set sql_mode=STRICT_ALL_TABLES;
Query OK, 0 rows affected (0.00 sec)
mysql> insert into simpletest386692test (id) values (21);
ERROR 1364 (HY000): Field 'test' doesn't have a default value

The warning became an error.

CommentFileSizeAuthor
strictmysql.patch533 byteschx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

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

I'm 100% for this, but I guess we will not like the result (create a book, try to move a page in the hiearchy, and see that book is broken #261258: Fix node_save() insertion logic).

webchick’s picture

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

I'm 100% for this too, but let's leave it open for discussion at least until some of the database folks chime in, hm? :)

I'm running the test suite, and so far all tests seem to pass, which is interesting. We must not have a test yet for the behaviour Damien describes.

Crell’s picture

Crell CreditAttribution: Crell commented

I am open to this *if* it doesn't break other functionality. I don't know enough about strict mode to say if, for instance, that precludes things like ON DUPLICATE KEY UPDATE or multi-value inserts. This requires more research.

chx, I'll trade you this for PHP 5 E_STRICT. :-)

chx’s picture

chx CreditAttribution: chx commented

First of all this only affects INSERT/UPDATE statements. Invalid data no longer generates a warning and works but instead freaks out. http://dev.mysql.com/doc/refman/5.0/en/constraint-invalid-data.html let's see:

  1. If you try to store an out of range value into a numeric column -- this is a very unlikely event. we have normal ints for practically everything.
  2. For strings, MySQL stores either the empty string or as much of the string as can be stored in the column. -- the "as much of the string" can indeed happen but other databases might balk if you try to store longer strings than allowed so MySQL should not allow it either so we catch bugs.
  3. If you try to store a string that doesn't start with a number into a numeric column, MySQL Server stores 0. -- same as above. Surely other databases wont tolerate storing 'foo' into an integer column.
  4. Invalid values for ENUM and SET columns -- we do not use it.
  5. MySQL allows you to store certain incorrect date values into DATE and DATETIME columns -- these are not in use currently
  6. If you try to store NULL into a column that doesn't take NULL values -- that does not sound a valid operation. If you want to get the default inserted, we use the default keyword not the NULL. So says http://dev.mysql.com/doc/refman/5.0/en/insert.html "Use the keyword DEFAULT to set a column explicitly to its default value." and HEAD has $placeholders = array_pad($placeholders, count($this->defaultFields), 'default'); already.
  7. If an INSERT statement specifies no value for a column -- the current INSERT query builder does not even allow you to do this.
Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

I'm running the test suite, and so far all tests seem to pass, which is interesting. We must not have a test yet for the behaviour Damien describes.

Now we have: #299136: Add tests for deleting and editing book pages.

Crell’s picture

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

Given that Dries said we still have a few months until code freeze, I vote we put this in now and see what happens. If it ends up totally b0rking the system we can easily pull it back out; if it doesn't, great, Drupal's APIs become more anal about standards which is almost always a good thing. :-)

(From chx's description, this basically disables all of the "MySQL shortcuts that make it easier to use for newbies but piss off "real" database developers" stuff.)

webchick’s picture

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

Erm. :) Can we actually test it first to "see what happens," and then put it in? ;) Damien found a bug with book module. Are other subsystems affected? Installer? Updater? Tricky revisions stuff? Enabling all core modules at once?

I know what the tests say... But I'd love for an actual human to try playing with Drupal for a few minutes (especially with stuff we don't currently have tests for) to perform some basic functions, and report back their results. I find it a little hard to believe that that obscure bug is the only thing that happens as a result of this change; we're not that good. ;)

If we get back good news from someone taking the time to do this and reporting back what they did, then I'm fine with this going in. Thanks to chx for the thorough blow-by-blow account of the differences.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Please webchick, let this go in. What the strict mode do is that it makes MySQL a little more picky. In a sense, it makes MySQL come closer to the PostgreSQL behavior.

The bug about the book module has been hitting PostgreSQL for *months*. In fact, the book module is *totally broken* since the release of Drupal 6. The bug was first reported the bug in February (!!!) on #223820. I fixed the bug first on February 28, and even wrote a test case for it on May, 14. Long story short, I finally gave up, due to the lack of interest and the impossibility to get any meaningful review (except catch's ones, hopefully).

So this patch will make core developers that only run MySQL *very* unhappy, and that's a good thing for our database compatibility. Again, I vote for that feature.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

If this doesn't break any existing core tests, I'd agree with putting it in asap - then we're more likely to find hidden and untested bugs as we go along.

All tests on MySQL pass with and without the patch. I've also run a clean install and that was fine. I didn't test enabling modules etc. because of #229129: System module page *seriously* broken - that whole page is a train wreck at the moment, nor the 6-7 update path because I'm about to go out.

In the worst case, we can use it during the development cycle then switch it off for point releases, same as E_NOTICE, or maybe that's the idea anyway.

So someone should install a 6.x site, upgrade it to 7.x, then mark this back to RTBC. If this does go in, I'd like us to put some serious work in fixing the last couple of postgres fails asap as well (last count there were two or three), then we'll actually be on a level playing field.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Fixed

Well that was a fun waste of a half hour. I don't think it's actually possible to update Drupal to D7 right now, with or without this patch. The settings.php file doesn't match and things get all kerflooey. I'll go see if this has been reported yet.

In the meantime though, committed. Let's hope that didn't break anything too horrendous. :P

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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