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.

Sign up for GitHub

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

Jump to bottom

Enable strict mode #8841

Merged
merged 7 commits into from
Jul 20, 2021
Merged

Enable strict mode #8841

merged 7 commits into from
Jul 20, 2021

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Jul 13, 2021 •
edited
Loading

#8836 was not enough to catch all the needed changes, I only fixed things I detected in my local setup, but there were more casts need with PHP 8. It also seems that Psalm and PHPStan detect more things in strict mode.

If you want me to split this into 2 PRs, let me know.

Closes #8819

Note that I'm targeting 2.x here, let me explain why:

  • I don't want too much divergence between both branches, especially when they touch that many files
  • there is no BC break AFAIK: strict mode is about function we call, not about our public API (and yes, the AFAIK is important, but if there is one, we can fix it with a cast IMO)
  • 3.x should be about removing deprecated stuff IMO: I think risky stuff that can be fixed quickly should be diluted in minors rather than piled up in 3.x, because having risky stuff always go to 3.x will make the migration to 3.0.0 very hard for everyone. In my vision, 3.0.0 should be very boring for somebody having addressed all deprecations, and not a big deal to upgrade to.

@greg0ire greg0ire force-pushed the strict_types branch 4 times, most recently from 5eec2e3 to c20649a Compare July 14, 2021 08:26
@greg0ire greg0ire marked this pull request as ready for review July 14, 2021 08:33
SenseException
SenseException previously approved these changes Jul 14, 2021
View reviewed changes
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried some combinations of strict type files and regular files and AFAIK2 this shouldn't be a problem, but I would wait for another reviewer to confirm this.
The additional typecasts are manageable and okay for me.

greg0ire added 7 commits July 20, 2021 21:55
@greg0ire
Ensure walkLiteral returns a string
1413111 
By contract, it is supposed to.
@greg0ire
Cast string value to float
4770008 
round() becomes stricter with PHP 8 and no longer accepts strings.
@greg0ire
Cast mode to string
decbd93 
It might be an integer, in fact it probably is, but the phpdoc says it
might be a string too.
@greg0ire
Pass strings to SimpleXMLELement::addAttribute()
270e7a4
@greg0ire
Document possible return type
efb74f3
@greg0ire
Enable strict mode
6c7e854
@greg0ire
Assert child property is not null
b7c0e97 
It is then passed to a class whose constructor cannot work with null.
@greg0ire
Copy link
Member Author

There is a conflict in the baseline, I'm going to push

@greg0ire greg0ire merged commit 0db4a39 into doctrine:2.10.x Jul 20, 2021
@greg0ire greg0ire deleted the strict_types branch July 20, 2021 21:19
@greg0ire greg0ire added this to the 2.10.0 milestone Jul 20, 2021
@github-actions github-actions bot mentioned this pull request Oct 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SenseException SenseException SenseException approved these changes

@beberlei beberlei beberlei approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@greg0ire @beberlei @SenseException