@daniel: Are you saying that you believe that archcom as a body needs to explicitly approve this patch? Or just that the members should have a chance to weigh in on whether the code is moving in a good or bad direction? Or....?

Do you have specific concerns about the patch? Do you have a specific proposal of something different that would be significantly better and/or not significantly more work?

I think the ArchCom should look at this patch. I don't think we need a formal approval process beyond the regular discussion/review on gerrit.

I'm worried by the fact that new static functions are added, I want to have a closer look at why. And I'm worried that since wfDeprecated is a no-no for code deployed on the wmf cluster, we will end up with a lot of if/then/else bloat in extensions that need to work with both the old and the new code, compare https://gerrit.wikimedia.org/r/#/c/197383/

In general, I believe the ArchCom should have an eye on not only RFCs, but also on the implementation of changes that impact the overall architecture of MediaWiki.

@daniel: Is there a way to add this non-RfC to the archcom in-person meeting agenda for this Wednesday? This issue is blocking a bunch of patches, and has been waiting for weeks already, so I'm trying to avoid having it stall anywhere.

@ksmith The patch is too big to review during a live meeting. We could try to get everyone to look at it in advance, and then discuss the results on Wednesday. But the time is quite short until then.

The fact that it's a huge patch is one of the things that worry me. It doesn't only mean it's hard to review, it also means it touches a lot of things and may turn out to be quite disruptive. I know that big changes are sometimes needed (I think my 20k lines patch introducing ContentHandler still holds the record), but it takes time (took me nearly a year to get ContentHandler reviewed and merged).

I will try to look into the patch in more depth tomorrow and give more detailed feedback, but I really believe other ArchCom me,bers should do the same before this goes in.

Let's separate "reviewing the entire patch" from "agreeing with the intent and direction" of the work. The patch summary seems to be a pretty clear description of the intent of the work, so I would expect that to be the level at which archcom would be involved. Obviously if anyone on or off the committee wants to dive into a deep code review that's great. But that would be different and separate.

So my proposal would be that one of us could alert the archcom members about this patch, and then during (or before) the wednesday in-person meeting, gain consensus on either "The direction/intent of the patch seems fine (just make sure the code is sufficiently reviewed)" or "The direction/intent of the patch seems problematic". In the latter case, specific next steps would be laid out.

Does that seem reasonable?

Intent and direction is not the question. That was the subject of the original RFC, which has been approved, an which I see no issue with. The thing I'm worried about is code architecture. That doesn't mean we need a full review of every line of code, but we should look at the way the intended changes were implemented.

I did bring this patch up during last weeks meeting, but there was no time to discuss it in any detail. I will try my best to get around to reviewing it more closely tomorrow, and I will bring it up again at the next meeting. The purpose of this ticket though is to get input from other ArchCom members.

I have added a number of comments on the patch now. It seems to have several flaws that could easily be addressed at this point, but would be hard to fix later, once the new code is used.