Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » PHP code refactoring (was: include cleanup) »

PHP code refactoring (was: include cleanup)

php.internals

Dmitry Stogov

3 years ago
Hi @internals, I have to say that we came to a serious conflict. Recently we voted for inluce cleanup RFC https://wiki.php.net/rfc/include_cleanup and it was declined. Despite that a series of code refactoring commits from Max were silently merged into the master. As this is a violation of the community rules and we should do something. In my opinion, this should be reverted (I would even think about rebasing to not pollute the git history). Personally, I don't like this refactoring, because it is mainly about coding preferences and habits. Anyway, I see that some people like this. Maybe this may attract new developers. OK. Let's do this, but do this in a managed way. The massive uncontrolled changes is the main problem in the current situation. Let's define the goal(s), rules, make a plan, summarize this in a new RFC, accept it. Then most of the work should be done in a separate branch and merged into the master all together after a final review. I (and other authority contributors) wouldn't be able to object against the terms accepted in RFC. So a good RFC should be a half of success... It would be great if PHP Foundation could assign some experienced developer(s) to be part of this work. Thanks. Dmitry.

Dan Liebner

3 years ago
Which commit? On Tue, Feb 28, 2023, 3:17 PM Dmitry Stogov <dmitrystogov@gmail.com> wrote:

Dmitry Stogov

3 years ago
On Tue, Feb 28, 2023 at 11:59 PM Dan Liebner <dliebner@gmail.com> wrote:
> Which commit? >
just some of them: https://github.com/php/php-src/commit/0270a1e54c0285fa3c89ee2b0120073ef57ab5fa https://github.com/php/php-src/commit/b98f18e7c3838cf587a1b6d0f033b89e9909c79d https://github.com/php/php-src/commit/42577c6b6b7577c57c161ee4a74cb193382bf1e0 https://github.com/php/php-src/commit/c7637ed1c03f556c6fb65884cfc5bfea4920b1c7 https://github.com/php/php-src/commit/371ae12d890f1887f79b7e2a32f808b4595e5f60 Thanks. Dmitry.

Max Kellermann

3 years ago
On 2023/02/28 22:31, Dmitry Stogov <dmitrystogov@gmail.com> wrote:
> https://github.com/php/php-src/commit/0270a1e54c0285fa3c89ee2b0120073ef57ab5fa
This kind of change was favored by a supermajority. You argue that this supermajority vote is irrelevant, and formally it indeed is, but pondering about formalities is kind of ignorant against the now well-known community opinion.
> https://github.com/php/php-src/commit/b98f18e7c3838cf587a1b6d0f033b89e9909c79d
No vote was made on this, therefore this doesn't violate any community rules, does it? If you think this should be reverted, explain why.
> https://github.com/php/php-src/commit/42577c6b6b7577c57c161ee4a74cb193382bf1e0
Favored by supermajority, see above.
> https://github.com/php/php-src/commit/c7637ed1c03f556c6fb65884cfc5bfea4920b1c7
No vote, no rule violation, see above.
> https://github.com/php/php-src/commit/371ae12d890f1887f79b7e2a32f808b4595e5f60
As you see in the commit message, this implements an (unwritten) rule cited by Nikita Popov (which is now written as of https://github.com/php/php-src/pull/10630). I personally don't agree with this rule (there's a thread on this mailing list about it), and I would favor reverting this commit - I only submitted this trying to help with implementing a rule even though I don't agree with it. If this gets reverted, then https://github.com/php/php-src/pull/10630 should be reverted as well. Again, not my opinion, I'm just trying to help implement somebody else's opinion. Max

Dmitry Stogov

3 years ago
On Wed, Mar 1, 2023 at 1:21 AM Max Kellermann <max+php@blarg.de> wrote:
> On 2023/02/28 22:31, Dmitry Stogov <dmitrystogov@gmail.com> wrote: > > > https://github.com/php/php-src/commit/0270a1e54c0285fa3c89ee2b0120073ef57ab5fa > > This kind of change was favored by a supermajority. > > You argue that this supermajority vote is irrelevant, and formally it > indeed is, but pondering about formalities is kind of ignorant against > the now well-known community opinion. > > > > https://github.com/php/php-src/commit/b98f18e7c3838cf587a1b6d0f033b89e9909c79d > > No vote was made on this, therefore this doesn't violate any community > rules, does it? >
Please reread https://wiki.php.net/RFC/voting#voting RFC is accepted by a supermajority of the primary vote. The secondary votes may be used to make decisions about implementation details. Thanks. Dmitry.

Max Kellermann

3 years ago
On 2023/02/28 23:34, Dmitry Stogov <dmitrystogov@gmail.com> wrote:
> > https://github.com/php/php-src/commit/b98f18e7c3838cf587a1b6d0f033b89e9909c79d > > > > No vote was made on this, therefore this doesn't violate any community > > rules, does it? > > > > Please reread https://wiki.php.net/RFC/voting#voting > RFC is accepted by a supermajority of the primary vote. > The secondary votes may be used to make decisions about implementation > details.
That doesn't answer my question. Does it violate any community rules or not? Max

Bob Weinand

3 years ago
Hey Max and Dmitry,
> Am 28.02.2023 um 23:34 schrieb Dmitry Stogov <dmitrystogov@gmail.com>: > > On Wed, Mar 1, 2023 at 1:21 AM Max Kellermann <max+php@blarg.de> wrote: > >> On 2023/02/28 22:31, Dmitry Stogov <dmitrystogov@gmail.com> wrote: >>> https://github.com/php/php-src/commit/0270a1e54c0285fa3c89ee2b0120073ef57ab5fa >> >> This kind of change was favored by a supermajority. >> >> You argue that this supermajority vote is irrelevant, and formally it >> indeed is, but pondering about formalities is kind of ignorant against >> the now well-known community opinion. >> >>> https://github.com/php/php-src/commit/b98f18e7c3838cf587a1b6d0f033b89e9909c79d >> >> No vote was made on this, therefore this doesn't violate any community >> rules, does it? >> > > Please reread https://wiki.php.net/RFC/voting#voting > RFC is accepted by a supermajority of the primary vote. > The secondary votes may be used to make decisions about implementation > details. > > Thanks. Dmitry.
In this case, while the primary concern of the RFC was rejected, I think it's pretty clear, that there was a supermajority for something specific. I do agree, that the topic, given hat it conflicts with the rules, a mail to internals on that topic should have been sent. I did vote no on that topic, however I still think there was some mandate by the community that this is wanted. I would thus not make too much of a fuss on these grounds about the splitting.
>> >> If you think this should be reverted, explain why. >> >>> https://github.com/php/php-src/commit/42577c6b6b7577c57c161ee4a74cb193382bf1e0 >> >> Favored by supermajority, see above. >> >>> https://github.com/php/php-src/commit/c7637ed1c03f556c6fb65884cfc5bfea4920b1c7 >> >> No vote, no rule violation, see above.
This is no violation and I think Dmitrys tone was a bit too accusing for that. But I also strongly disagree with that change. Let's revert it. Quickly seeing which number is which type is quite valuable for debugging. By now I know most numbers by heart, but most people won't.
>>> https://github.com/php/php-src/commit/371ae12d890f1887f79b7e2a32f808b4595e5f60 >> >> As you see in the commit message, this implements an (unwritten) rule >> cited by Nikita Popov (which is now written as of >> https://github.com/php/php-src/pull/10630). I personally don't agree >> with this rule (there's a thread on this mailing list about it), and I >> would favor reverting this commit - I only submitted this trying to >> help with implementing a rule even though I don't agree with it. >> >> If this gets reverted, then https://github.com/php/php-src/pull/10630 >> should be reverted as well. Again, not my opinion, I'm just trying to >> help implement somebody else's opinion.
No, it should not. It makes a lot of sense. It's however not a hard rule, but a guideline. Applying these changes to ZEND_API though always shall be evaluated with due care. In this specific instance, a fairly new API I think nobody uses yet, it's sort of acceptable. I wouldn't mind reverting it though
>> >> Max >>
Max, while it's nice to see a couple cleanups in the PHP codebase, I would like to ask you to be a bit less quick at moving things around. There's value in having a codebase evaluate slowly. Obviously not at the cost of progress, new features etc.. Keep in mind, that at least extension authors will need to work with the php-src codebase of version 8.2 until probably 7 years in the future (basically until PHP is EOL on all Linux distributions). They will appreciate not looking at a vastly different codebases. Finally, these sorts of code moves are also making a git blame harder to track back. Thanks, Bob

Christian Schneider

3 years ago
Am 01.03.2023 um 01:13 schrieb Bob Weinand <bobwei9@hotmail.com>:
> In this case, while the primary concern of the RFC was rejected, I think it's pretty clear, that there was a supermajority for something specific.
I didn't vote on this RFC but I have to disagree with you and Max here: My understanding of such a voting process is that the secondary votes are dependent on the acceptance of the first one. This means I would interpret it as 'if and only if "Should #include directives be cleaned up?" Is accepted then I'm voting yes for "Is it allowed to split a large header to reduce dependencies"'. I can't know whether the people voting here meant it that way or not, I'm just saying that taking the second vote out of context seems problematic to me. Reminds me a bit of looking at the numbers of an A/B test when the precondition "is the result significant" is false. Regards, - Chris

Nikita Popov

3 years ago
I'm a bit out of the loop on the higher level discussion, but as I got named dropped here, a quick note... On Tue, Feb 28, 2023, at 23:21, Max Kellermann wrote:
> On 2023/02/28 22:31, Dmitry Stogov <dmitrystogov@gmail.com> wrote: > > https://github.com/php/php-src/commit/0270a1e54c0285fa3c89ee2b0120073ef57ab5fa > > This kind of change was favored by a supermajority. > > You argue that this supermajority vote is irrelevant, and formally it > indeed is, but pondering about formalities is kind of ignorant against > the now well-known community opinion. > > > https://github.com/php/php-src/commit/b98f18e7c3838cf587a1b6d0f033b89e9909c79d > > No vote was made on this, therefore this doesn't violate any community > rules, does it? > > If you think this should be reverted, explain why. > > > https://github.com/php/php-src/commit/42577c6b6b7577c57c161ee4a74cb193382bf1e0 > > Favored by supermajority, see above. > > > https://github.com/php/php-src/commit/c7637ed1c03f556c6fb65884cfc5bfea4920b1c7 > > No vote, no rule violation, see above.
This commit broke a valuable debugging reference: I used to often check these defines to determine what a type code means. If you get type 18, good luck figuring out what that means now.
> > > https://github.com/php/php-src/commit/371ae12d890f1887f79b7e2a32f808b4595e5f60 > > As you see in the commit message, this implements an (unwritten) rule > cited by Nikita Popov (which is now written as of > https://github.com/php/php-src/pull/10630). I personally don't agree > with this rule (there's a thread on this mailing list about it), and I > would favor reverting this commit - I only submitted this trying to > help with implementing a rule even though I don't agree with it.
This is a pretty cherry-picked statement. In that mailing list thread I explicitly pointed out that while this is the rule (and if you introduce new code, that would be the convention to follow), that does not necessarily mean that it's a good idea to fix existing cases that don't follow it. I also pointed out that for public APIs, this is a very insidious API break, because it doesn't make using code fail to compile: It just silently inverts the result from it's previous meaning. It looks like those parts of my mail just got ignored. Regards, Nikita

Max Kellermann

3 years ago
On 2023/02/28 21:16, Dmitry Stogov <dmitrystogov@gmail.com> wrote:
> Recently we voted for inluce cleanup RFC > https://wiki.php.net/rfc/include_cleanup and it was declined. > Despite that a series of code refactoring commits from Max were silently > merged into the master. > As this is a violation of the community rules and we should do something.
I don't get any of that. Please be more specific. Which community rule was violated by whom? In your opinion, what exactly does the outcome of the vote mean? Does it mean that include cleanups are now forbidden (despite being wanted by the majority of voters)? Or does it mean that nothing changes because no new rule has taken effect? (I asked that already, but nobody replied.) What do you mean by "merged silently"? All of my PRs were submitted in the public, and everybody had a chance to comment on these (which you did, for example, in https://github.com/php/php-src/pull/10641). It usually takes a week or so before they are merged. If you feel that is not enough time, why not post a RFC and vote on a mandatory minimum review duration for all merges/pushes? Though, by contrast, you merge most of your own PRs shortly after you create them without any code review. Quite often, you even push commits without any PR. In my opinion, that's more "silent" than my work, don't you agree? Which specific commits do you wish to revert? Is this about include cleanups (none of which were merged) or about header splitting (which a supermajority voted for) or about other kinds of code refactoring (which was not voted on)?
> Then most of the work should be done in a separate branch and merged > into the master all together after a final review.
The problem with merge conflicts was your major complaint about branch merging (and that was only about merging bug fixes, not about merging two volatile branches together) - and now you suggest something that will certainly lead to merge conflicts because your suggestion postpones the big merge for as long as possible. That will inflict major pain to PHP maintainers (though not to outside contributors like me, so why would I care). Another big problem: if it's uncertain that some changes ever get merged, because the "final review" may reject it after months or years of work, nobody will ever want to clean up the PHP code again, because it's very likely that you'll veto it again, and then all the time would be wasted. Not a great prospect. If you want to scare away contributors, that's how to do it. My opinion is: if a patch improves a piece of code, it should be merged. Of course, whether something is an improvement is debatable; but postponing that decision is pointless and harmful. Max

Christian Schneider

3 years ago
Am 28.02.2023 um 22:04 schrieb Max Kellermann <max+php@blarg.de>:
> On 2023/02/28 21:16, Dmitry Stogov <dmitrystogov@gmail.com> wrote: >> Recently we voted for inluce cleanup RFC >> https://wiki.php.net/rfc/include_cleanup and it was declined. > > Which specific commits do you wish to revert? Is this about include > cleanups (none of which were merged) or about header splitting (which > a supermajority voted for) or about other kinds of code refactoring > (which was not voted on)?
You keep saying that people voted yes on the secondary vote (clarifications on how to clean up). Just to clarify: This is irrelevant as the primary vote was declined.
> My opinion is: if a patch improves a piece of code, it should be > merged. Of course, whether something is an improvement is debatable; > but postponing that decision is pointless and harmful.
As far as I can see there seems to be a disagreement whether it is an improvement, which to me sounds like there must be more discussion. Which means the decision has to be postponed, no? I second Dmitry's request to do things in a managed way. What exactly caused the disruption I'm not sure and it is not about blaming someone but finding a workable solution. Regards, - Chris

Dmitry Stogov

3 years ago
On Wed, Mar 1, 2023 at 12:04 AM Max Kellermann <max+php@blarg.de> wrote:
> On 2023/02/28 21:16, Dmitry Stogov <dmitrystogov@gmail.com> wrote: > > Recently we voted for inluce cleanup RFC > > https://wiki.php.net/rfc/include_cleanup and it was declined. > > Despite that a series of code refactoring commits from Max were silently > > merged into the master. > > As this is a violation of the community rules and we should do something. > > I don't get any of that. Please be more specific. > > Which community rule was violated by whom? >
Merging the things that were rejected. You may name this differently but this is still code refactoring. In your opinion, what exactly does the outcome of the vote mean? Does
> it mean that include cleanups are now forbidden (despite being wanted > by the majority of voters)? Or does it mean that nothing changes > because no new rule has taken effect? (I asked that already, but > nobody replied.) >
Include cleanups RFC was rejected. No refactoring RFC was presented. A lot of changes that affect all core contributors are committed into master. If you think this is questionable, we may ask for help from some arbitr.
> > What do you mean by "merged silently"? All of my PRs were submitted > in the public, and everybody had a chance to comment on these (which > you did, for example, in https://github.com/php/php-src/pull/10641). > It usually takes a week or so before they are merged. If you feel > that is not enough time, why not post a RFC and vote on a mandatory > minimum review duration for all merges/pushes? >
I don't review every PR. Only when I asked. I'm mainly busy with new development and bug fixes.
> Though, by contrast, you merge most of your own PRs shortly after you > create them without any code review. Quite often, you even push > commits without any PR. In my opinion, that's more "silent" than my > work, don't you agree? >
Currently, I merge into master only bug fixes. And yes, sometimes my fixes may introduce new bugs. My new development is done in a separate branch and will be presented in RFC when ready.
> Which specific commits do you wish to revert? Is this about include > cleanups (none of which were merged) or about header splitting (which > a supermajority voted for) or about other kinds of code refactoring > (which was not voted on)? >
All code refactoring commits. Can you imagine you commited something like this into Linux, JVM, GCC?
> > Then most of the work should be done in a separate branch and merged > > into the master all together after a final review. > > The problem with merge conflicts was your major complaint about branch > merging (and that was only about merging bug fixes, not about merging > two volatile branches together) - and now you suggest something that > will certainly lead to merge conflicts because your suggestion > postpones the big merge for as long as possible. That will inflict > major pain to PHP maintainers (though not to outside contributors like > me, so why would I care). >
PHP-6 was developed in a separate branch and we were able to stop it when understood that it is not good enough. PHPNG was developed in a separate branch and was presented and accepted only when we showed the benefits. A new JIT has been developed in a separate branch for more than a year...
> Another big problem: if it's uncertain that some changes ever get > merged, because the "final review" may reject it after months or years > of work, nobody will ever want to clean up the PHP code again, because > it's very likely that you'll veto it again, and then all the time > would be wasted. Not a great prospect. If you want to scare away > contributors, that's how to do it. >
As I said, I won't object to the terms of accepted RFC. I already made much more noise than I liked. My opinion is: if a patch improves a piece of code, it should be
> merged. Of course, whether something is an improvement is debatable; > but postponing that decision is pointless and harmful. >
Changing code back and force without a plan is also harmful. I proposed to you a way to do what you like in agreement with other contributors. Doing this without agreement won't work. Thanks. Dmitry.

Max Kellermann

3 years ago
On 2023/02/28 23:08, Dmitry Stogov <dmitrystogov@gmail.com> wrote:
> > Which community rule was violated by whom? > > > > Merging the things that were rejected. You may name this differently but > this is still code refactoring.
That sidesteps my question, and answers something else.
> In your opinion, what exactly does the outcome of the vote mean? Does > > it mean that include cleanups are now forbidden (despite being wanted > > by the majority of voters)? Or does it mean that nothing changes > > because no new rule has taken effect? (I asked that already, but > > nobody replied.) > > > > Include cleanups RFC was rejected. > No refactoring RFC was presented. > A lot of changes that affect all core contributors are committed into > master.
Do you mean to imply that code changes that do not implement RFC or fix a bug should always be rejected? If that is true, I'll point you to a huge bunch of refactoring commits that must be rejected until an RFC is formally accepted by a supermajority. Just to name a few very recent ones: https://github.com/php/php-src/commit/4177257178d6a1a44f0aa6d6b23d02b91e0a58d3 https://github.com/php/php-src/commit/9108a32bfe881c3b1e2f3b2949b0e9fe1b9c6dda https://github.com/php/php-src/commit/07fe46fb5db9d6f34e72f513ae053fc8c9ad67a https://github.com/php/php-src/commit/900472536775b71d5d72a0d66eaa46ae7c7d7ad9 https://github.com/php/php-src/commit/f0cfebc2b867a6a96a88c4526cf9f3b4cd01f04b https://github.com/php/php-src/commit/f079aa2e242b251c6297bddf5365c33c126b7dcc https://github.com/php/php-src/commit/b14dd85dca3b67a5462f5ed9b6aa0dc22beb615c Do you believe these should be reverted as well?
> > What do you mean by "merged silently"? > > > > I don't review every PR. Only when I asked. I'm mainly busy with new > development and bug fixes.
That doesn't answer my question. You said my work as merged "silently", and I'd like to know what you mean by that.
> > Though, by contrast, you merge most of your own PRs shortly after you > > create them without any code review. Quite often, you even push > > commits without any PR. In my opinion, that's more "silent" than my > > work, don't you agree? > > > > Currently, I merge into master only bug fixes. > And yes, sometimes my fixes may introduce new bugs. > My new development is done in a separate branch and will be presented in > RFC when ready.
That, again, doesn't answer my question. Do you mean that merging bug fixes is allowed to be "silent"? (Whatever "silent" means.)
> > Which specific commits do you wish to revert? Is this about include > > cleanups (none of which were merged) or about header splitting (which > > a supermajority voted for) or about other kinds of code refactoring > > (which was not voted on)? > > > > All code refactoring commits.
Okay, so you demand to revert all code refactoring commits - all of them, not just the ones I authored? Including the ones I linked above?
> Can you imagine you commited something like this into Linux, JVM, GCC?
Yes. That happens all the time in those projects. I'm surprised by your conservatism.
> PHP-6 was developed in a separate branch and we were able to stop it when > understood that it is not good enough. > PHPNG was developed in a separate branch and was presented and accepted > only when we showed the benefits.
That's different. These were major rewrites, and my work is small incremental improvements, just formal changes, no actual code changes.
> A new JIT has been developed in a separate branch for more than a year...
The new JIT, the one that you used as a reason to reject all changes to the current JIT, because changing the current JIT would cause merge conflicts for your new JIT? IMO that demonstrates what's wrong with that approach. Max

Max Kellermann

3 years ago
On 2023/02/28 23:33, Max Kellermann <max+php@blarg.de> wrote:
> > Include cleanups RFC was rejected. > > No refactoring RFC was presented. > > A lot of changes that affect all core contributors are committed into > > master. > > Do you mean to imply that code changes that do not implement RFC or > fix a bug should always be rejected?
CONTRIBUTING.md says: "PHP welcomes pull requests to add tests, fix bugs and to implement RFCs." Indeed it appears Dmitry is right - code refactoring is generally NOT allowed (unless there is an explicit RFC vote, and I havn't seen one). This implies that all those commits (and hundreds of others):
> https://github.com/php/php-src/commit/4177257178d6a1a44f0aa6d6b23d02b91e0a58d3 > https://github.com/php/php-src/commit/9108a32bfe881c3b1e2f3b2949b0e9fe1b9c6dda > https://github.com/php/php-src/commit/07fe46fb5db9d6f34e72f513ae053fc8c9ad67a > https://github.com/php/php-src/commit/900472536775b71d5d72a0d66eaa46ae7c7d7ad9 > https://github.com/php/php-src/commit/f0cfebc2b867a6a96a88c4526cf9f3b4cd01f04b > https://github.com/php/php-src/commit/f079aa2e242b251c6297bddf5365c33c126b7dcc > https://github.com/php/php-src/commit/b14dd85dca3b67a5462f5ed9b6aa0dc22beb615c
... should not have been merged! Where do we go from here? Really revert EVERYTHING? (Reverting just my code refactoring changes but nobody else's would make no sense at all. Don't make this personal.) Max

Dmitry Stogov

3 years ago
On Wed, Mar 1, 2023 at 8:37 AM Max Kellermann <max+php@blarg.de> wrote:
> On 2023/02/28 23:33, Max Kellermann <max+php@blarg.de> wrote: > > > Include cleanups RFC was rejected. > > > No refactoring RFC was presented. > > > A lot of changes that affect all core contributors are committed into > > > master. > > > > Do you mean to imply that code changes that do not implement RFC or > > fix a bug should always be rejected? > > CONTRIBUTING.md says: > > "PHP welcomes pull requests to add tests, fix bugs and to implement > RFCs." > > Indeed it appears Dmitry is right - code refactoring is generally NOT > allowed (unless there is an explicit RFC vote, and I havn't seen one). > > This implies that all those commits (and hundreds of others): > > > > https://github.com/php/php-src/commit/4177257178d6a1a44f0aa6d6b23d02b91e0a58d3 > > > https://github.com/php/php-src/commit/9108a32bfe881c3b1e2f3b2949b0e9fe1b9c6dda > > > https://github.com/php/php-src/commit/07fe46fb5db9d6f34e72f513ae053fc8c9ad67a > > > https://github.com/php/php-src/commit/900472536775b71d5d72a0d66eaa46ae7c7d7ad9 > > > https://github.com/php/php-src/commit/f0cfebc2b867a6a96a88c4526cf9f3b4cd01f04b > > > https://github.com/php/php-src/commit/f079aa2e242b251c6297bddf5365c33c126b7dcc > > > https://github.com/php/php-src/commit/b14dd85dca3b67a5462f5ed9b6aa0dc22beb615c > > ... should not have been merged! >
At least they should be reviewed once again. I already have questions for the https://github.com/php/php-src/commit/9108a32bfe881c3b1e2f3b2949b0e9fe1b9c6dda
> Where do we go from here? Really revert EVERYTHING? >
I'm not too paranoid about small changes and changes in extensions code. I didn't object when you proposed small refactoring steps. (e.g. adding "const" and "static" or small include clenups).
> (Reverting just my code refactoring changes but nobody else's would > make no sense at all. Don't make this personal.) >
This is not personal. I saw you are smart and may find and fix not trivial bugs. I assume all contributors have good intentions. The problem with your commits, that you started rewriting EVERYTHING (the core parts of php) without a deep knowledge and without agreement with mainteners. They already started to break things (See Nikita's notes about RC debugger). Formally they were committed after a declined RFC. I already proposed a way that might work. Thanks. Dmitry.

Niels Dossche

3 years ago
On 3/1/23 08:42, Dmitry Stogov wrote:
> On Wed, Mar 1, 2023 at 8:37 AM Max Kellermann <max+php@blarg.de> wrote: > >> On 2023/02/28 23:33, Max Kellermann <max+php@blarg.de> wrote: >>>> Include cleanups RFC was rejected. >>>> No refactoring RFC was presented. >>>> A lot of changes that affect all core contributors are committed into >>>> master. >>> >>> Do you mean to imply that code changes that do not implement RFC or >>> fix a bug should always be rejected? >> >> CONTRIBUTING.md says: >> >> "PHP welcomes pull requests to add tests, fix bugs and to implement >> RFCs." >> >> Indeed it appears Dmitry is right - code refactoring is generally NOT >> allowed (unless there is an explicit RFC vote, and I havn't seen one). >> >> This implies that all those commits (and hundreds of others): >> >>> >> https://github.com/php/php-src/commit/4177257178d6a1a44f0aa6d6b23d02b91e0a58d3 >>> >> https://github.com/php/php-src/commit/9108a32bfe881c3b1e2f3b2949b0e9fe1b9c6dda >>> >> https://github.com/php/php-src/commit/07fe46fb5db9d6f34e72f513ae053fc8c9ad67a >>> >> https://github.com/php/php-src/commit/900472536775b71d5d72a0d66eaa46ae7c7d7ad9 >>> >> https://github.com/php/php-src/commit/f0cfebc2b867a6a96a88c4526cf9f3b4cd01f04b >>> >> https://github.com/php/php-src/commit/f079aa2e242b251c6297bddf5365c33c126b7dcc >>> >> https://github.com/php/php-src/commit/b14dd85dca3b67a5462f5ed9b6aa0dc22beb615c >> >> ... should not have been merged! >> > > At least they should be reviewed once again. > I already have questions for the > https://github.com/php/php-src/commit/9108a32bfe881c3b1e2f3b2949b0e9fe1b9c6dda >
Hi Dmitry I made that commit. Please ask me the questions you have and I'll be happy to answer them. Thanks Niels

Dmitry Stogov

3 years ago
On Wed, Mar 1, 2023 at 11:34 AM Niels Dossche <dossche.niels@gmail.com> wrote:
> On 3/1/23 08:42, Dmitry Stogov wrote: > > On Wed, Mar 1, 2023 at 8:37 AM Max Kellermann <max+php@blarg.de> wrote: > > > >> On 2023/02/28 23:33, Max Kellermann <max+php@blarg.de> wrote: > >>>> Include cleanups RFC was rejected. > >>>> No refactoring RFC was presented. > >>>> A lot of changes that affect all core contributors are committed into > >>>> master. > >>> > >>> Do you mean to imply that code changes that do not implement RFC or > >>> fix a bug should always be rejected? > >> > >> CONTRIBUTING.md says: > >> > >> "PHP welcomes pull requests to add tests, fix bugs and to implement > >> RFCs." > >> > >> Indeed it appears Dmitry is right - code refactoring is generally NOT > >> allowed (unless there is an explicit RFC vote, and I havn't seen one). > >> > >> This implies that all those commits (and hundreds of others): > >> > >>> > >> > https://github.com/php/php-src/commit/4177257178d6a1a44f0aa6d6b23d02b91e0a58d3 > >>> > >> > https://github.com/php/php-src/commit/9108a32bfe881c3b1e2f3b2949b0e9fe1b9c6dda > >>> > >> > https://github.com/php/php-src/commit/07fe46fb5db9d6f34e72f513ae053fc8c9ad67a > >>> > >> > https://github.com/php/php-src/commit/900472536775b71d5d72a0d66eaa46ae7c7d7ad9 > >>> > >> > https://github.com/php/php-src/commit/f0cfebc2b867a6a96a88c4526cf9f3b4cd01f04b > >>> > >> > https://github.com/php/php-src/commit/f079aa2e242b251c6297bddf5365c33c126b7dcc > >>> > >> > https://github.com/php/php-src/commit/b14dd85dca3b67a5462f5ed9b6aa0dc22beb615c > >> > >> ... should not have been merged! > >> > > > > At least they should be reviewed once again. > > I already have questions for the > > > https://github.com/php/php-src/commit/9108a32bfe881c3b1e2f3b2949b0e9fe1b9c6dda > > > > Hi Dmitry > > I made that commit. > Please ask me the questions you have and I'll be happy to answer them. >
See the comment at the commit page on github and let's continue the related discussion there. I think github sends notification emails to commit authors. You probably missed it. Thanks. Dmitry.

Derick Rethans

3 years ago
On 28 February 2023 21:04:35 GMT, Max Kellermann <max+php@blarg.de> wrote:
>On 2023/02/28 21:16, Dmitry Stogov <dmitrystogov@gmail.com> wrote: >> Recently we voted for inluce cleanup RFC >> https://wiki.php.net/rfc/include_cleanup and it was declined. >> Despite that a series of code refactoring commits from Max were silently >> merged into the master. >> As this is a violation of the community rules and we should do something. > >I don't get any of that. Please be more specific. > >Which community rule was violated by whom?
The one where you shouldn't have tried your hardest to antagonise as many people as possible. cheers Derick