Page MenuHomePhabricator

Review proposed ApiResult rewrite
Closed, ResolvedPublic


I think the ArchCom should look over the proposed overhaul of ApiResult (I7b37295e8862b188d). This is a pretty central part of our code base, and we should make sure the proposed changes are in line with what we would like MediaWiki architecture to look like.

Original RFC: T482

Related Objects

Event Timeline

daniel created this task.Mar 30 2015, 10:44 AM
daniel raised the priority of this task from to High.
daniel updated the task description. (Show Details)
daniel added a project: TechCom.
daniel added a subscriber: daniel.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 30 2015, 10:44 AM
Anomie added a subscriber: Anomie.Apr 2 2015, 7:10 PM

@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?

daniel added a comment.Apr 6 2015, 4:41 PM

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

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.

daniel added a comment.Apr 6 2015, 5:43 PM

@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?

daniel added a comment.Apr 6 2015, 8:39 PM

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.

daniel added a comment.Apr 7 2015, 5:47 PM

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.

daniel updated the task description. (Show Details)Apr 7 2015, 5:48 PM
daniel set Security to None.
daniel closed this task as Resolved.Jun 16 2015, 11:52 AM
daniel claimed this task.