Column::allows_empty_string() doesn't work as documented

[IlyaSemenov]

The following WordPress\Tabulate\DB\Column method works contradictionary to how it is documented:

    /**
     * Only NOT NULL text fields are allowed to have empty strings.
     * @return boolean
     */
    public function allows_empty_string() {
            $textTypes = array( 'text', 'varchar', 'char' );
            return $this->nullable() && in_array( $this->get_type(), $textTypes );
    }

Supposedly, it should be return !$this->nullable() ...

[samwilson]

Ah yes, good catch. The idea here is that if a text field allows nulls then it should be set to null if an empty string is passed in.

Either that, or the UI should be changed to permit explicit setting of null, vs empty strings.

What do you think?

[samwilson]

Can you see if the test at

tabulate/tests/SchemaTest.php

Line 118 in dac0b87

public function empty_string() {

is checking what it should? I think it is... but one's brain does get confused with these things... :)

[IlyaSemenov]

Frankly, I think Tabulate being a general use library should allow both NULLs and empty strings for nullable text fields. This is because you can't know beforehand which databases users will manage. In certain scenarios developers might specifically choose to store both empty strings and NULLs meaning different things (e.g. NULL = user never submitted data via some sort of the front end UI, empty = user knowingly submitted/confirmed empty string).

The UI might show the checkbox [x] NULL after the input box if the input box is empty (and hide it with javascript once the text box is filled with something).

However, I agree that the current approach that you chose still makes perfect sense, provides cleaner UI and will work perfectly in most cases.

[samwilson]

Yes, you are quite correct! I agree, a checkbox allowing either null or empty string is the way to go. I'll get it done. :)