Closed liborm85 closed 8 years ago
Thanks for the PR! It would make sense you to split it into smaller parts by the actual topic. Particularly, the most important thing here is the bugfix for #72988, so it should be the first thing. All the white space change and other amount of change have nothing to do with it.
Also, PHP 5 part will still need this fix. The PHP 5 support is also required for the next couple of years, for sure. But anyway, it's discussable later. IMHO every grouped change should sit in a separate PR, otherwise it is quite impossible to evaluate the impacts.
Thanks.
@weltling Ok, i splitted to few PRs, resolves what was broken attempting to implement in PHP 7.
I thought that the support for PHP 5 should be only a branch php5, because the master is only for PHP 7. Many support has been removed earlier in the master. Ok, PRs for removing compatibility with PHP 5 I will not do.
@liborm85 thanks! With PHP5 what i meant - it needs to be supported along with 7, and patches will need to be backported. As I see you wrote in #15, the patch is only relevant for 7, so it is fine then. The master branch is does not support PHP 5 already, so the cleanup can be done with a separate PR as well.
I'm going to check your split PRs in the next days, if no one beats me to it.
Cheers
PR fixed and improved the following:
I do not know how fix TODOs with zend_list_addref and zend_list_delete, but all works correctly without fixing this.