Fixed issue when creating a status label via API - default_label and show_in_nav being not nullable

[snipe]

This just sets some defaults in the code and updates the field to be nullable so it at least won't throw an error. Fixes RB#15683

[inietov]

Looks pretty straightforward. The new migration doesn't have down option because it adds a default?

[snipe]

The new migration doesn't have down option because it adds a default?

Correct. We wouldn't want to pull that default on a rollback, especially since I did a back in time migration

[uberbrady]

I don't really know what changed in Laravel that makes this stuff a little weird, but it's definitely got me scratching my head trying to figure out when it changed, or if it changed, or if I'm somehow losing my few remaining marbles. I'm glad you tackled it, because I sure as hell don't know how I want to handle it.

I think the code changes you've added there should be enough to shore us up, though I have some other ideas which are more clunky and more verbose that I'll mention for posterity's sake near the bottom.

But I still have some issues with the migration there, because I'm worried it's going to allow the database data to be inconsistent.

Once we make those fields nullable, valid field values in the DB will be:

NULL
0
1

And in general that will probably 'just work' because PHP will treat a false-ey value such as NULL similarly to how it'd treat a 0. And our default value is currently 0. Though I can't be sure that it'll even be possible, via API, to jam a NULL in there at all? This is why it's confusing.

My general policy is to make the database unable to put in 'bad' values at all, so we don't have to go through and clean it out or 'fix it up' later, but, again, I'm not even certain I could jam a NULL into that field if I wanted.

The only other thing I have is that I'm a little uncomfortable with having the default value set in both the code, and in the database. I'm worried about what would happen if the two got somehow out of sync. Like, let's say we decide to change the default to '1' from '0' - but we only remember to do it in one place? That could definitely get confusing.

So although it's really clunky to do it this way, my temptation is to do:

if($request->has('show_in_nav')) {
  $statuslabel->show_in_nav = $request->input('show_in_nav');
}

This means that only one 'default' value is being set - the one in the database. And if we change it there (for whatever reason), now the code will respect the new value going forward.

Another option which might even be worse would be to just remove the default from the database completely, but leave the field as non nullable. And trust that the code will pick the correct default value (precisely as you have written it). And it will crash if you don't. And if we want to change the default, we change it in the code. I guess that might also mean we could have different defaults in different controllers (which might be different usage contexts), which could be useful somehow? E.g. an API controller makes labels that don't show in Nav by default, but a regular Web controller might make it so they do show in Nav by default.

Anyways, that's a long way of me saying "I don't know what the right thing to do is here."

All of that being said, this change prevents certain errors from triggering and we definitely want to do that. So I'll approve it if you are comfortable enough with the possibility of some inconsistency at the DB level. The only time I've seen that before for us is when we were putting 0's in some related_id column somewhere and we had to do some one-liner SQL fixes.

[snipe]

@uberbrady alright - what if I pull the ->nullable() migration and just leave the default value (0) in the controllers?

The migration that sets things right shouldn't really be necessarily, since the DB barfs if you try to submit a null, but we also support a bunch of different versions of DB stuff, which is why I was trying to go belt and suspenders.

[uberbrady]

@uberbrady alright - what if I pull the ->nullable() migration and just leave the default value (0) in the controllers?

I'd be totally fine with that. That still leaves us a bit of belt-and-suspender action in that the DB will barf if you somehow manage to submit a NULL, but the controller shouldn't even let you anyways. (But, could be like a CSV import or something else that might let it slip past - if so, I'd rather have it puke immediately rather than muck up the data.)

[snipe]

Could casting that field as a boolean in the model make some sense?

[uberbrady]

Possibly? There's also a $request->boolean('blah') method I just found out about as I was researching my answer, which could potentially help too? I dunno, this weirds me all out. Like, what happens if I try to put a 7 in there? That's a "tinyint," right? Not sure. Lemme keep pondering it and read some docs.

[snipe]

If we do it via request, we could still end up in a place where we missed a new controller method down the line, that's why I was thinking casting it at the model level might make sense