search

berlindb / core

All of the required core code
MIT License
255 stars 29 forks source link

Unable to save null values #22

Closed ashleyfae closed 4 years ago

ashleyfae commented 4 years ago

Query::validate_item() runs stripslahes() on all values. This immediately converts null to an empty string and thus makes it impossible to intentionally set a null value.

JJJ commented 4 years ago

I cannot recall if this was intended at the time or not, but it probably shouldn’t be like this.

I also do not remember whether any of the EDD3 or SC tables allow for null values, intentionally or as default.

I’ll need to do some research. Thanks for creating this issue @nosegraze 👍

ashleyfae commented 4 years ago

RCP used a null value recently and I patched it in our copy. See https://github.com/restrictcontentpro/restrict-content-pro/pull/2617/files

ashleyfae commented 4 years ago

Related, specifically for datetime columns:

https://github.com/berlindb/core/blob/master/column.php#L739-L741

The use of ! empty( $this->default ) breaks having a null default value.

JJJ commented 3 years ago

Some interesting thoughts on this issue specifically: https://gabi.dev/2016/04/20/not-null-all-the-things/

DrewAPicture commented 3 years ago

I shared these thoughts with @ashleyfae directly, but opted to also post them here:

I generally prefer not to have null values in the database. This is mostly because it can wreak havoc if you're not doing proper type checking, which let's face it, a lot of WordPress developers don't (see: the empty() example cited above).

I recognize the value in being able to define a field as null in that it's not 0, it's just not set and that might very well be significant. At the same time, it's easy enough to make the assumption that 0 is the same thing as not set. It mostly depends on whether or not you have the need to differentiate between not set and 0, imho

I think Berlin already takes a pretty opinionated stance, so I would think the way to go would be to absolutely allow null values in Berlin. And then be deliberate about differentiating between not set and 0. I think it provides a lot more flexibility as long as that – perhaps, anomaly –doesn't bleed out and cause problems lower in the stack.

Meaning: give it the flexibility to do it, but also simultaneously set an expectation that you have to tell Berlin whether or not you want to allow it for the current table.