Merging pull requests instead of closing them View as Markdown

Hi internals,

I want to bring up something that's been bothering me since my contribution: many PRs in php-src are being closed instead of merged, with the changes pushed separately as standalone commits.

The problem is that GitHub marks these PRs as "Closed", not "Merged". So from the outside — and from the contributor's perspective — it looks like the work was rejected. Their GitHub profile shows no merged contribution, even though the code was accepted and is now in the repo.

Beyond statistics, it also disconnects the commit from the review discussion, and it's just confusing, especially for newer contributors who don't know the convention.

I get that there might be reasons for this — keeping a linear history, concerns about merge commits, etc. But rebasing or squashing before merging solves all of that. Asking a contributor to rebase or squash is totally normal, and most people are happy to do it.

Merging PRs is the standard across open source. It's how you signal that a contribution was accepted.

So my question is: is there a way to make merging the default practice in php-src? Has this been discussed before? If so, what were the blockers?

Merging PRs that target master is common practice.
However, when merging bug fixes our process is to merge into the lowest branch and then up, something GitHub cannot do nor support, as sometimes we need to make changes to the fix in the merge up process.
There has been discussion about this previously off list, but changing the merge process is difficult and no-one has come to an agreement.
However, 90% of you complain are issues with GitHub. We amend the commit message to close the PR so that the link is preserved, and GitHub, if it would be smarter, would be able to understand that it actually means "merged" not "closed".
I think relying on GitHub to provide useful statistics is just a fools errand as it doesn't even consistently mark certain things as "reviews" or not if you want to gather usage statistics.

Best regards,
Gina P. Banyard

Hi Valentin

GitHub marks PRs as "Merged" under either of these two conditions:

• The PR is merged via GitHub UI
• The latest commit of the source branch is pushed to the target branch

As explained by Gina, most of us already merge most PRs that target
master via GitHub UI, unless they require additional changes like a NEWS
entry. When merging commits manually, the source branch usually requires
a rebase (as feature/bugfix branches are merged via fast-forward merge).
Additionally, we frequently amend some information to the commit
message, like adding a reference to the PR. Either of those change the
commit SHA and will break GitHub's ability to track the merge status of
the PR and leave the PR open. To fix this, we usually add "Closes
GH-xyz" to the commit message to auto-close the PR. Some also just close
the PR manually with a link to the merged commit. The catch is that the
status of the PR will be "Closed" rather than "Merged".

To make GitHub pick up on the merge, the source branch would need to be
force-pushed after the rebase so that the fast-forwarded commit once
again matches that of the latest commit of the source branch. However,
this also triggers CI and would incur a non-trivial amount of fully
redundant resource usage. That's the main reason this has not been
implemented (apart from the additional step).

The benefit OTOH is that the PR changes would be fully in sync with what
has actually been merged, which is something Tim has argued for in the
past. If we can find a way to avoid the additional CI resource usage,
I'd be happy to implement these changes.

Ilija

Hi internals,

I want to bring up something that's been bothering me since my
contribution https://github.com/php/php-src/pull/13029: many PRs in
php-src are being closed instead of merged, with the changes pushed
separately as standalone commits.

The problem is that GitHub marks these PRs as "Closed", not "Merged".
So from the outside — and from the contributor's perspective — it
looks like the work was rejected. Their GitHub profile shows no merged
contribution, even though the code was accepted and is now in the repo.

Beyond statistics, it also disconnects the commit from the review
discussion, and it's just confusing, especially for newer contributors
who don't know the convention.

I get that there might be reasons for this — keeping a linear history,
concerns about merge commits, etc. But rebasing or squashing before
merging solves all of that. Asking a contributor to rebase or squash
is totally normal, and most people are happy to do it.

Merging PRs is the standard across open source. It's how you signal
that a contribution was accepted.

So my question is: is there a way to make merging the default practice
in php-src? Has this been discussed before? If so, what were the blockers?

Thanks,
Valentin Udaltsov