@jelly Thanks for the PR. Please open a respective ticket in the bugtracker so the fix can be tracked in NEWS and the changelog. Also the PR should be rebased onto PHP-7.0. See also the notes regarding bug fixes. Thanks!

@cmb69 As a procedure tip, the way I usually apply PRs is by using the .patch URL and git am. For example, for this PR it would be wget https://github.com/php/php-src/pull/2835.patch and git am -3 2835.patch. This makes it easy to apply PRs regardless of which branch they were submitted against.

The phpt is missing a title, so it will be a borked test, also it seems like it doesn't expect the warning it actually adds too?

@nikic Thanks for the tip regarding the patch URL! git am wouldn't mark the PR as merged, though, if I'm not mistaken.

@cmb69 that's true, pr would have to be closed manually. But the git history would be cleaner.

Thanks.

It's not that I want to avoid to close the PR manually, but rather that I'd prefer to see on a quick glance whether a PR has been merged or closed. Sometimes I stumble over old PRs (for instance, linked from the bug tracker) and if the tag at the top says closed I have to skim over the thread to see what actually happened to the PR. Scrolling to the end might not help, since sometimes the discussion continues on closed PRs.

If the general consensus is that applying the patch is preferable, I'll change my current workflow (which basically resembles https://wiki.php.net/vcs/gitworkflow#merge_a_pull_request), of course. :)

Anyhow, in this case the PR has to be overhauled anyway – see Kalle's comment above.

This is related to bug #53070.

@cmb69 nope, i just wanted to note that "me too" using rather git am :) From the PR point - yes, having a label is probably nicer, from the git history and also flexibility POV such as being able to apply to another branch - git am. Perhaps it could work, when adding the github issue number to the commit message https://help.github.com/articles/closing-issues-using-keywords/ , but not sure which label it would then show. Also note that a PR could have been merged and then reverted, which won't change the label, so you'd still have to scroll through :)

Thanks.

@cmb69 It indeeds look like bug #53070 do I need to link the pull request to that ticket?

@weltling AFAIK, auto-closing works only for issues, but not for PRs.

Also note that a PR could have been merged and then reverted, which won't change the label, […]

Good catch! :)

@jelly Yes, please do so. I've re-opened bug 53070 and already linked to this PR.

@cmb69 thanks and is the current docref fine? Then I'll adjust the other failing tests.

In my opinion throwing a warning is fine and I don't see a potential BC break, because formerly it didn't work anyway (usually segfaulting).

Comment on behalf of cmb at php.net:

Applied with @5acb838. Thanks, Jelle!