5.next: Fix up allowNullableNull default. #18964
Conversation
|
I guess this depends on what bake currently generates if it's a safe change for 5.next. |
|
How come? I pointed out above that this should be a safe default either way. |
|
@markstory Can you take a look at this one too? |
| public function __construct(array|string $fields, Table|Association|string $repository, array $options = []) | ||
| { | ||
| $options += ['allowNullableNulls' => false]; | ||
| $options += ['allowNullableNulls' => true]; |
There was a problem hiding this comment.
This seems to be on purpose
- Add allowPartialNulls-flag to ExistsIn() that matches SQLs behavior of composite foreign keys having some nullable nulls #8908
- ExistsIn() should allow composite foreign keys having NULL allowed in the schema #8897
- 3.x Bug/Enhance: ExistsIn Application Rule and NULL values #8749
My concern with doing this in 5.x is breaking an existing application. Like the current failures, these will be silent. I personally don't want to have to deal with regression issues. Perhaps we can add a deprecation, and change behavior in 6.x?
There was a problem hiding this comment.
wow, that comment was lost for 3 weeks.
There was a problem hiding this comment.
I am also fine going into 6.x directly.
Just not sure what the regressions would be in this case. Like what scenario would cause it? Since this just properly excludes the null ones right now, which before it didnt. And for the ones not nullable it would error on DB level, so either way it is caught from what I understand.
There was a problem hiding this comment.
Example scenario:
// DB: category_id INT NULL (nullable but app always requires it)
$rules->add($rules->existsIn(['category_id', 'site_id'], 'SiteCategories'));
// Old behavior: rejects null category_id
// New behavior: accepts null category_idHowever: This is already fragile code because:
- If validation is bypassed (patch entities, direct DB queries), null gets saved anyway
- The validation rule's intent is to check foreign key existence, not nullability
- Proper code would have explicit validation: ->notBlank('category_id')
If we go with 5.next:
This could be clarified in the migration guide.
If we go with 6.x:
We could also forward shim this with a app.php config value (default false for full BC) if we wanted to globally allow the upgrade switch towards 6.x then.
There was a problem hiding this comment.
Full BC possible with e.g. Configure::read('ORM.nullableNull') could be used with current default false to make apps future proof in 5.3.
If people like it extra cautious. This will give us experience over time about possible edge cases.
|
@ADmad whats your take? |
|
While allowing null by default for nullable columns seems intuitive, this change could potentially cause issue. For legacy apps, at times the db schema doesn't always reflect the business requirement, which are enforced in code. So for a multi column primary key, a column could be nullable in the db even though it is required. Switching the default would remove the in-code safe guard. To get better defaults for a new apps, we can update bake instead to generate the rule with The default value for the option can be changed in 6.x. Or maybe it would be better to have a separate rule altogether |
|
Wouldnt Anyway, what do you think about my upwards shim config value? That would also solve it cleanly as opt-in. |
You mean adding a config like Not a fan of that. I would add such config only when absolutely necessary. For features that are not common, such configs are not easily discoverable by users and also get lost/forgotten, as we saw with the |
|
It depends on if we want to keep both ExistsIn and ExistsInNullable around in 6.x I can whip up the alternative and we can see what is a better fit. |
|
I prefer the updated PR here as we probably would not need 2 separate methods forward.
But I made an alternative in case people want a speaking API here. |
This is currently a hidden and not so easy to trace issue
When you have a nullable foreign key like company_id, the existsIn rule will fail for null values by default (when checking composite keys where only some are null), which is counterintuitive since:
So:
Changing the default to true would be more ergonomic because:
And you can still set it manually, if needed.
This seems to me to be a saner default now.
We can add this to the migration changelog for 5.next.