On Thu, 9 Feb 2023 at 22:09, Matthew Weier O'Phinney < mweierophin...@gmail.com> wrote:
> I'm not directly involved in maintenance, but my take on the scenario was > that these were rejected and reverted because they caused breakage, whether > that was in compiling a spare PHP build, or in extensions that were > assuming that using certain headers would slurp in everything they needed. > This breakage was unacceptable without an RFC. I saw chatter from a number > of folks after the changes were merged about builds no longer compiling; > considering the stability of the php-src tree, inability to build will > always be a source of alarm. > > [...] > > As I pointed out earlier, the changes previously merged led to breakages > when compiling the project. How is that not enough? And dumping a huge > bunch of PRs with such changes without first discussing it with maintainers > means a lot of effort reviewing [...]. > The other point that has been brought up multiple times is that it > introduces breaking changes for extension maintainers. > > Should these extensions be relying on one or more "god" headers instead of > the specific headers for the symbols they use? Probably not. Will forcing > the issue without giving them a chance to review and understand the > changes, and have a roadmap for when and how those changes occur be a net > positive? No; it will cause a lot of busy work for a lot of people, almost > all of whom are volunteers and most of whom would rather be building out > user-requested features or fixing user-reported bugs. > > I'm unsure why that's unclear or not enough for you. > > -- > Matthew Weier O'Phinney > mweierophin...@gmail.com > https://mwop.net/ > he/him > I'm going to ignore Max's subpar behaviour and frustration in the aftermath of this. However, as the reviewer of these PRs, they did NOT completely break the build, I was working on php-src and compiling master without ANY issues during the weekend after landing the changes. The *only* build being broken was the DTrace build, which would have been fixed by a follow-up PR. We have had completely broken builds for longer days due to some other random changes, and we didn't revert them but fixed them as a follow-up. We still, for over 6 months now, have a "broken" ASAN build due to phpdbg messing up the analyser and crashing the test runner on 8.2 and master, something that multiple core devs, me included, need to work around by monkey patching the run-test.php file. These changes were initially agreed upon and multiple people were in favour, even by Dmitry. I will also say, that I spend a significant amount of time reviewing those PRs, and I would have continued, as I think they are beneficial to the project. But my opinion and the work I put into the reviews is apparently worthless. Therefore, I maintain that, IMHO, these commits should not have been reverted. The unique complaint, that should be addressed to me, is in how I merged the PRs in that I didn't squash them. The fact external extensions were broken and that we should add all relevant headers to php.h is a fair complaint and would have been fixed quite easily in a single commit and did not affect anyone effectively at this stage because we are, checks notes, 6 months at minimum before the first alpha builds of PHP 8.3 are released. The only reason they were reverted is that Dmitry demanded them as they broke the DTrace build, which he is apparently the only one to use and is not tested on CI, a failure of the project's CI infrastructure. As I previously complained, the RFC process for this kind of change is completely inadequate and demonstrates that the PHP project has a massive issue with governance and how to handle cases like this. As a final note, if the complaint had been made by anyone else other than Dmitry, I doubt these changes would have been reverted, and can we please stop pretending otherwise. Sincerely, George P. Banyard
