Address deprecations #12108
Address deprecations #12108
Conversation
There was a problem hiding this comment.
Directionally, the change looks right to me. I'm only unsure about the BC impact. It would be nice to enumerate some typical use cases (e.g. when someone wants to opt out) and see why and what the upgrade will look like.
If it's a bug (which I agree it is), then maybe we don't need to provide the way to retain the incorrect behavior.
| public readonly bool $deferrable = false, | ||
| public readonly bool $unique = false, | ||
| public readonly bool $nullable = true, | ||
| public readonly bool|null $nullable = null, |
There was a problem hiding this comment.
Isn't the fact that now this property can contain null a breaking change? I don't really like that a seemingly boolean property is now nullable. Instead, a separate flag could be introduced like $wasNullableSetExplicitly (well, unfortunately, it's a public property that can be modified w/o triggering any code).
There was a problem hiding this comment.
Yes, that's exactly the issue. And with PHP, I don't know of a way to detect that a named argument was explicitly set: https://3v4l.org/uesWA#v8.4.9
There was a problem hiding this comment.
I don't know the impact of this change on the projects, that are all out there when they update ORM. Hard for me to assess.
There was a problem hiding this comment.
@SenseException let me point out one thing: I did not have to change a single functional test.
UPGRADE.md
Outdated
| If you want to delay the migration to the new behavior, you can explicitly mark | ||
| the column as nullable in your mapping. |
There was a problem hiding this comment.
Doesn't this also mean that unless the user opts out of the fixed behavior, the behavior will change, so it's a breaking change.
FWIW, the DBAL will mutate this column and make it nullable and also create a non-nullable column in the database. At this point, it's not clear why exactly someone might want to opt out and why.
There was a problem hiding this comment.
Yes, it's a breaking change, and that's why I want to provide the user with a way to opt out, although I am not sure it will be useful at all; maybe they have some big table on which they want to delay the migration, in order to do it in a different way, with a different tool than the one they use for other tables.
There was a problem hiding this comment.
I don't think a schema migration will be necessary after the upgrade to the new ORM version. DBAL 4 will still make the PK columns non-nullable, regardless of what the metadata says.
There was a problem hiding this comment.
Oh! I didn't realize that! Then I guess this makes this a lot less of a breaking change, doesn't it? And it also mean we indeed don't need to provide users with a way to opt out of this.
There was a problem hiding this comment.
It sounds right. FWIW, here's the test that demonstrates that DBAL 4 will make the PK columns non-nullable, regardless of what the metadata says.
I plan to nudge users into switching to the correct behavior in 3.6.x, with a deprecation. I think since I'm doing a breaking change in a patch release, I should allow people to opt out of the change. |
UPGRADE.md
Outdated
| @@ -1,3 +1,53 @@ | |||
| # Upgrade to 3.5.2 | |||
There was a problem hiding this comment.
The fact that we need to mention this change in the upgrade file tells me that this kind of change should go into a x.y.0 release.
There was a problem hiding this comment.
You often insist on having deprecations being treated as bugs and addressed in the patch branch. Are you maybe suggesting we use https://github.com/doctrine/deprecations?tab=readme-ov-file#suppressing-specific-deprecations for this one, then retarget this?
There was a problem hiding this comment.
You often insist on having deprecations being treated as bugs and addressed in the patch branch.
I do. But you're about to change external behavior. Let's not do that in a bugfix release.
Are you maybe suggesting we use https://github.com/doctrine/deprecations?tab=readme-ov-file#suppressing-specific-deprecations for this one, then retarget this?
Not sure if suppressing the deprecation is worth it, tbh.
There was a problem hiding this comment.
OK. Given what has been said in other threads, I'm rather going to remove this upgrade guide as well as the possibility to opt-out of this change since it will not result in a database change after all.
There was a problem hiding this comment.
It doesn't? Then I'm fine targeting 3.5.x. 🙂
Using a nullable column that references another table as part of a primary key makes no sense, and is ignored by DBAL. Let us ignore it at the ORM level.
greg0ire
force-pushed
the
address-deprecations
branch
from
ae3abe9 to
257c509
Compare
4 participants
By deprecating the usage of nullable columns as part of a primary key,
doctrine/dbal4.3 points out several bad defaults in the ORM:Let us change the minimum amount of bad defaults, while still allowing people to explicitly request a join column be nullable.
On 3.6.x, we should deprecate setting
nullablein the above cases, and on 4.0.x, we should throw when that's the case.