PHP 8.4 | Fix implicitly nullable parameters#880
Conversation
|
Failing build is unrelated to this PR and should be fixed via #878 |
65f8f13 to
4cd8b9f
Compare
PHP 8.4 deprecates implicitly nullable parameters, i.e. typed parameter with a `null` default value, which are not explicitly declared as nullable. Includes updating the documentation to match. Ref: https://wiki.php.net/rfc/deprecate-implicitly-nullable-types
4cd8b9f to
66cc21b
Compare
|
@jtojnar A little detail about merging: I can see you have forced-pushed, which destroys the git history, comments, etc. and also removes the ownership of the commit from the original author (@jrfnl ). I can personally better like to use the GitHub flow of updating the branch (just click on the button) and then squash and merge to have a nice git history in the main branch.
P.S. Hurrah for the merge and congrats with your new role :-) |
|
👍 for not using force-pushes. |
|
I did not actually force push, I used the “Update branch” button, choosing the “Update branch with rebase” option for cleaner git history. (First time I noticed this.) I am not sure what you mean by destroying the history – squashing would do that but rebase preserves the original commit author as well as the commit messages. The only thing that changes is the committer tag, which makes sense as I am integrating the commit into the repository. |
|
Ah, right, rebase is the culprit. It is not needed if you use Squash and merge just after. |
|
As the author of these commits - I'm generally not a fan of squash-merging when the original commits are atomic commits with detailed commit messages - like mine are, as it destroys the context (commit + message being closely bound). When there is one atomic commits and the follow-up commits are "fix up" commits, that's, of course, a different matter. |
|
Oh and as a side-note, I'll happily rebase any PR of mine whenever you think it's needed. |
|
There are different preferences of course. I typically avoid all the processes rewriting the git history, including rebase. It breaks Squash and merge also allows a combined message for the whole PR, while keeping the PR for the details of how it ended up as it is (which is lost with rebase). Without it, it gave the following in the
But those things are details, personal preferences, and I am happy to see the PRs merged faster :-) |
Well, squash merge will suffer from exactly the same problems, in addition to removing the context as @jrfnl mentions. Yes, the context will remain in the PR itself but it is pretty much useless there, as tools like So I would argue that all the relevant context (especially the reasoning behind any non-trivial decisions) should be included in the commit messages or code itself whenever possible. Atomic commits also help since when there is only one thing the commit message can refer to, the context is clearer.
I agree that history rewrites are problematic and should be avoided on persistent public branches. But I generally consider PR branches ephemeral where everything goes. Especially before the merge itself, I think rebase/squash makes sense since it will actually result in the PR containing the same commits as the target branch (unlike the default Merges other than the fast-forward one). And then the branch is typically deleted anyway. It is true that history rewrites make stacking multiple PRs together annoying with plain git but nowadays, there are tools that make this a breeze like one of the following:
Looks like there are new ones every time I look, I should really start using one of them 😹
Right, I do not trust GitHub with the context. What is not accessible through If the commits are atomic and contain all the context like the @jrfnl’s ones from #881, there should be no need to ever go to the PR. And if you for some reason need it, you can still access the PR from the commit in GitHub UI. And IMO, it is still cleaner than this 😉
Right. It would be good to settle on a common policy, though. |
Hear hear! It's something I always try to mention in my git talks too and why I try to always put all the relevant info in the commit message. |
|
I do not really mind the policies here, but just to clarify a few things:
That is a big assumption to make, which requires the original author to be around, and makes review processes more cumbersome. Furthermore, if a PR contains multiple unrelated commits, it should be multiple PRs instead
When the original author happens to provides a PR that is better suited for a merged commit, and if not change is needed from the maintainers' side (neither in the code nor in the commit messages - which might be rare I believe), then no reason for a squash indeed
Indeed, and it is mostly this scenario (which I believe to be quite usual), which I am discussing here
The squash commit message can be anything the maintainer / merger wants including a better message than what the original author(s) may have provided or not. Especially true at the end of a review process, when the explanation to be put inside the message may have changed.
We were not talking about the same situation. My point is that
That is a good principle but independent on whether squashed commits are used or not
Indeed, good principle, but orthogonal to using squashed commits or not, and actually this can be made even better thanks to squashed commits, as the maintainer has full control about the commit message
Indeed, in this case no need for a squashed commit, but I do not believe this is the majority of the situations, and insisting on it would add quite some overhead I am out :-) |
PHP 8.4 deprecates implicitly nullable parameters, i.e. typed parameter with a
nulldefault value, which are not explicitly declared as nullable.Includes updating the documentation to match.
Ref: https://wiki.php.net/rfc/deprecate-implicitly-nullable-types