Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarified type hint nullability being the first default #8828

Closed
wants to merge 4 commits into from

Conversation

fridde
Copy link
Contributor

@fridde fridde commented Jul 6, 2021

This adds the reference to the new mapping that uses explicit type hints and their nullability to the attribute reference doc.

It also clarifies that a property being typed as nullable will be mapped to a nullable column, in both attribute- as annotation-reference-doc.

@beberlei
Copy link
Member

beberlei commented Jul 10, 2021

Doesnt this contradict #8835?

@SenseException
Copy link
Member

@fridde Maybe you can take a look at the introduced changes in the mentioned PR and use it in yours?

@fridde
Copy link
Contributor Author

fridde commented Jul 26, 2021

@SenseException Sorry for the delay, I was busy with other stuff.

After reading through a few threads, like this and this I managed to confuse myself and everyone else.

I agree with @beberlei that if inferring column nullability from the type's nullability (i.e. public string $user = null breaks backwards compatibility, introducing this part of the feature was a mistake and had to be reverted.

But it would be nice if someone from the core team could comment on why this inference is not a part of the 3.0-branch (as far as I can see)?

Is this a deliberate choice? Because the BC-break would definitely be worth it and spare future developers the headache of understanding this odd anomaly.

After the comment, I'll rewrite my PR to be less confusing and more correct.

@beberlei
Copy link
Member

beberlei commented Aug 5, 2021

@fridde Its purely an issue of time and not getting to it yet that this is not part of the 3.0 branch yet. But it will also require a very precisely located and crafted deprecation message in 2.10.x so that users can prepare for this change without false-positives.

@beberlei beberlei closed this Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants