Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119532
MIME-Version: 1.0
References: <Y9pXdfDdYqnzwnIt@swift.blarg.de> <1cb213ea-ff7d-c4b2-5345-fadbc5953c94@bastelstu.be>
 <Y+VKekLm+PTaEG7y@swift.blarg.de> <CAJp_myV3MOoGckgBr0f8-0nhiUWkLJ8Bt_Z3apzsJH7i+ePY3Q@mail.gmail.com>
In-Reply-To: <CAJp_myV3MOoGckgBr0f8-0nhiUWkLJ8Bt_Z3apzsJH7i+ePY3Q@mail.gmail.com>
Date: Mon, 13 Feb 2023 00:58:39 +0000
Message-ID: <CAFPFaMJmkG=M8dnAq=NY-mzejteqxq-wA_DA+g_O+WdHZKNdnQ@mail.gmail.com>
To: "Matthew Weier O'Phinney" <mweierophinney@gmail.com>
Cc: Max Kellermann <max+php@blarg.de>, internals@lists.php.net
Content-Type: multipart/alternative; boundary="000000000000aa63b905f48a591a"
Subject: Re: [PHP-DEV] [VOTE] include cleanup
From: george.banyard@gmail.com ("G. P. B.")

--000000000000aa63b905f48a591a
Content-Type: text/plain; charset="UTF-8"

On Thu, 9 Feb 2023 at 22:09, Matthew Weier O'Phinney <
mweierophinney@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
> mweierophinney@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

--000000000000aa63b905f48a591a--