silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

Regression: MySQL Enum does not allow Empty values #2935

Closed nathanbrauer closed 10 years ago

nathanbrauer commented 10 years ago

From https://github.com/silverstripe/silverstripe-framework/pull/2707 Commit: https://github.com/wilr/sapphire/commit/20e082d00e6390eeb1d2f4e00f6850127d9a1882

This now causes applications which include an empty Enum value by default to fail.

E.g. Here is valid MySQL Enum used in a class we have. (We use this throughout our entire application.)

class Event extends Page {

    private static $singular_name = 'Event';

    private static $can_be_root = false;
    private static $icon = '_static/images/silverstripe/icons/Event.png';
    private static $allowed_children = array(
        'Event',
        'RedirectorPage',
    );

    private static $default_sort = 'EventSortOrder';

    private static $db = array(
        'Type'              => "Enum(',Inherit,Conference,Product Demo,Webinar,Local Event','')",
        'Start'             => 'SS_Datetime',
        'End'               => 'SS_Datetime',
        'Location'          => 'Varchar(255)',

        //If child of event, these are overridden by the Parent's
        'Timezone'          => 'Varchar(255)',
        'TargetRegion'      => "MultiEnum('Americas,Europe,Asia Pacific')",

        //Only available to child events
        'LandingPageURL'    => 'Varchar(255)',

        //Only available to parent events
        'IsParentEvent'     => 'Boolean',
        'DateDisplay'       => "Enum('Smart,Range,Strict','Smart')",
        'ChildFormat'       => "Enum(',DateList,DetailedList,Hide','')",

        // Hidden fields
        'EventSortOrder'    => 'Int(11)',
    );

In this, Type and ChildFormat allow an empty value (Enum does not allow an empty value unless explicitly implied) AND the default is set to the empty value. This allows logic elsewhere to function properly (e.g. we cannot enforce a default event type, it makes a big difference to the application whether an event is a conference or a webinar).

MultiEnum, however, is by default able to be empty. Therefore, the change there makes sense.

(Also note that if it's decided that this change is good and should be kept after all, this is an API change and therefore shouldn't have been released until 3.2.)

cc: @wilr @chillu @halkyon

nathanbrauer commented 10 years ago

I'm pretty sure you see what I'm saying already, but just in case you'd like to see what this looks like on MySQL itself, here's a few MySQL outputs.

The model is exactly the same in both. The first is with this code merged. The second is with it reverted. (Notice also the differences in the Default column.)

mysql> describe Event;
+----------------+---------------------------------------------------------------------+------+-----+----------+----------------+
| Field          | Type                                                                | Null | Key | Default  | Extra          |
+----------------+---------------------------------------------------------------------+------+-----+----------+----------------+
| ID             | int(11)                                                             | NO   | PRI | NULL     | auto_increment |
| Type           | enum('Inherit','Conference','Product Demo','Webinar','Local Event') | YES  |     | Inherit  |                |
| Start          | datetime                                                            | YES  |     | NULL     |                |
| End            | datetime                                                            | YES  |     | NULL     |                |
| Location       | varchar(255)                                                        | YES  |     | NULL     |                |
| Timezone       | varchar(255)                                                        | YES  |     | NULL     |                |
| TargetRegion   | set('Americas','Europe','Asia Pacific')                             | YES  |     | NULL     |                |
| LandingPageURL | varchar(255)                                                        | YES  |     | NULL     |                |
| IsParentEvent  | tinyint(1) unsigned                                                 | NO   |     | 0        |                |
| DateDisplay    | enum('Smart','Range','Strict')                                      | YES  |     | Smart    |                |
| EventSortOrder | int(11)                                                             | NO   |     | 11       |                |
| FeaturedTileID | int(11)                                                             | NO   | MUL | 0        |                |
| ChildFormat    | enum('DateList','DetailedList','Hide')                              | YES  |     | DateList |                |
+----------------+---------------------------------------------------------------------+------+-----+----------+----------------+
13 rows in set (0.00 sec)

mysql> describe Event;
+----------------+------------------------------------------------------------------------+------+-----+---------+----------------+
| Field          | Type                                                                   | Null | Key | Default | Extra          |
+----------------+------------------------------------------------------------------------+------+-----+---------+----------------+
| ID             | int(11)                                                                | NO   | PRI | NULL    | auto_increment |
| Type           | enum('','Inherit','Conference','Product Demo','Webinar','Local Event') | YES  |     |         |                |
| Start          | datetime                                                               | YES  |     | NULL    |                |
| End            | datetime                                                               | YES  |     | NULL    |                |
| Location       | varchar(255)                                                           | YES  |     | NULL    |                |
| Timezone       | varchar(255)                                                           | YES  |     | NULL    |                |
| TargetRegion   | set('Americas','Europe','Asia Pacific')                                | YES  |     | NULL    |                |
| LandingPageURL | varchar(255)                                                           | YES  |     | NULL    |                |
| IsParentEvent  | tinyint(1) unsigned                                                    | NO   |     | 0       |                |
| DateDisplay    | enum('Smart','Range','Strict')                                         | YES  |     | Smart   |                |
| EventSortOrder | int(11)                                                                | NO   |     | 11      |                |
| FeaturedTileID | int(11)                                                                | NO   | MUL | 0       |                |
| ChildFormat    | enum('','DateList','DetailedList','Hide')                              | YES  |     |         |                |
+----------------+------------------------------------------------------------------------+------+-----+---------+----------------+
13 rows in set (0.01 sec)
tylerkidd commented 10 years ago

This has affected a number of our sites as well. Our work around has been to use a throw away string, "none", "null", "default" - but that doesn't always work if the value is supposed to be used and not checked against.