r111264 MediaWiki - Code Review archive

Your use of an anonymous function in line 215 is PHP 5.3+ only (probably even more due to the use clause).

This class is 5.3 dependent as described in the docs. You can get rid of the anon functions, but not of the late static bindings (unless you want to do evil stuff).

That's the way it is unfortunately - only code that can depend on 5.3 can use this. That means none of core for now, until we ditch 5.2, but most new extensions can happily require 5.3 (esp since 5.3 is now on the cluster).

I'm really not a fan of putting stuff in core that requires 5.3 until such a time as we have bumped the required version to 5.3. Even if only extensions use the code (which honestly, probably won't stay that way once these classes are in core) it seems rather wrong.

I see your point. This would need clear docs that it requires 5.3 to avoid accidents. Then again, these should be easy to spot. This class could theoretically be put into it's own extension, but the hassle involved with this is probably more work then fixing any misuse of this class if it where to happen when it's in core.

Sort of makes me wonder when we finally will drop 5.2.x support in core though. That version lost official support by the PHP guys long ago (2 years?). This class is one more reason to drop support.

By the late static binding, do you mean anything else than the get_called_class()? That function can be emulated very inefficiently by using debug_backtrace()

The uses of static:: are LSB. Although I remain convinced they're a hack to work around poor initial design decisions 90% of the time.

If you think this class falls in this 90%, please suggest how to get similar functionality without LSB so we can make this compatible with 5.2. I don't think you can do this in a nice way, but would be glad to be proven wrong about this :)

I'm not sure what the utility of making so much of it static is. Why not instantiate classes and then call ->delete() etc on the objects.

An instance represents a row. So if you want to do an operation on a table as a whole, it makes no sense to make an instance. I think it even does not make sense if you ignore that first argument. Why would you create an instance of an object if there is absolutely no need to?

$instance = new MyClass();
$instance->someNonInstanceBehaviour();

MyClass::someNonInstanceBehaviour();

The later one looks more sane to me.

Not necessarily more sane, just lazier. Instantiating classes to call a method is a basic tenet of OOP. Static should be used for things that truly should be called from outside of the class on a non-instance of it (such as factory methods or utility functions). Making it static to save a line of code at the expense of making the code more complex isn't a win in my book.

More complex? You have static methods instead of non-static ones. That's not more complex.

What would you propose doing instead? Having some singleton like pattern and calling these methods on an instance of the class? This is what I am doing in the Contest extension because it needed to be PHP 5.2 compat, and I think it's rather evil to have methods that should be static be non-static like that.

> Static should be used for things that truly should be called from outside of the class on a non-instance of it

I guess this is our main point of disagreement. Why do you think this is the case? And how does your reasoning fir in with things such as Linker::link, which has been made static?

> Not necessarily more sane, just lazier

This is about doing the right thing. Not about saving a line of code. No need to accuse me of being lazy.

No need for a singleton either, just instantiate it when you need it.

Linker::link() is a utility function, so is one of the "ok for static" cases I mentioned above.

So you would make the getDBTable, getFieldPrefix, getFieldTypes and similar methods non-static? And then create an instance of the class to call them and get the completely not instance related info they return? This seems evil. The info belongs to the class, not one instance.

Create two classes, one representing the table, and a second one for the rows (which would hardly need to be overriden).

Resetting to fixme, I'm not convinced that making some parts of the core require a different PHP version is wise.

I don't see the harm done really. This is an utility included in core that needs a different version. If it's not used by anything in core, it's not really part of it in a way.

Either way, I want some clarifications about the PHP 5.3 policies we want to have, so will post to the list.

http://lists.wikimedia.org/pipermail/wikitech-l/2012-February/058062.html

canHasField() should be renamed to a real verb phrase -- maybe canHaveField()

r111295 :)

I'm not convinced about this class. For one, why use $wgDBDataObjects if each object will need to get its own class anyway?

It's easier to override both getDBTable() and getFieldPrefix()

Then there's the way in how it tries to load half-columns. I assume each instance corresponds to a table row? If a system needs to load the same row twice, it's very likely that someone was coded wrong. I'm not saying we shouldn't support that, but I'd log such thing.

> It's easier to override both getDBTable() and getFieldPrefix()

This is actually what it originally did. The reason why I changed it is gone now, so this could easily be changed back. This really is a implementation detail that does not affect the rest of the code at all, so judging the class on that seems odd :)

> I assume each instance corresponds to a table row?

Yeah it does.

> Then there's the way in how it tries to load half-columns.

Sorry, I'm not following you here. half-columns? Are you referring to the fact that you can choose to load only a subset of the fields?

> If a system needs to load the same row twice, it's very likely that someone was coded wrong.

Load it twice? Huh? It does not need to do this...

I wasn't clear. I was refering to how you can load a subset, then if you need a non-loaded field, go filtering with that anonymous function. That second load shouldn't happen in good code (though it's not bad to take the possibility into account).

Yes, I agree, this should not happen. AFAIK this is not happening anywhere in the Education Program extension, which makes heavy use of this class, so it can definitely be avoided. The idea is that you load all fields you need when the object gets constructed. If you then try to get a field that you did not load, it will throw an exception. There are cases where it might be impossible to know in advance what you will need though. For this the class allows for loading additional things later on (with the option to not re-load already loaded data). I don't see any way to avoid doing 2 selects in such cases. If you have an idea that works, I'd be happy to hear it :)

I'll have to correct myself there. The original reason for having $wgDBDataObjects is not gone. It's there to also be able to automatically add prefixes for tables that do not have their own class in joins (typically tables that just link stuff). I'm actually not convinced myself that this join code is any good. Where all other code in this class makes stuff easier, this is one place where it just adds hassle. So I'm thinking of just throwing that out, after which the class can return to using overriding of the methods you mentioned.

Gone in r111305

Unrelated to other comment threads, but definitely needs some unit tests if it's kept.

+1. I will write these if this can stay in core.

In the long run, It would be ten time easier to reuses an existing ORM such as doctrine or propel (from symphony). I do not think we want that kind of home-brewed ORM :

• it adds to our maintenance / code review queue
• it is yet another barrier to new developers
• it will cause bug
• it lacks a design document

Finally I am unsure which needs it is intended to fulfill, much less if it is going to be able to fulfill the perceived needs at this time.

I would personally bury that somewhere. Start by writing the needs we have, evaluate existing solutions and then if and only if none of them fit our needs, start writing our own ORM.

> reuses an existing ORM such as doctrine or propel

> it adds to our maintenance / code review queue

You'd need to adapt these to work with our existing DB abstraction layer or throw the later out altogether. This does NOT decrease maintenance or review costs.

> it is yet another barrier to new developers

You can make this argument about every utility added. Really, this is like asking why we got Html when we had Xml. You are not forced to use this and can just stick with the regular db access functions. To be fair though, this might actually be easier to use for new developers.

> it will cause bug

Yeah, that happens with code and can be listed as a potential side effect of every addition.

> it lacks a design document

It's better documented then most of our current code.

> Finally I am unsure which needs it is intended to fulfill

Did you read the docs? It lists the reasons? It avoids a lot of work, avoids a lot of code duplication and increases consistency. I wrote the class because I felt the need to have such an utility and have used it to great effect in my last 5 extensions.

I don't like getReadDb() / setReadDb(). I would have expected getReadDb() to return a DB object. And setReadDB is used only in updateSummaryFields() to switch a parameter used two functions down in the stack.

What would you propose as alternative?

These functions are needed to set the type of database used to read so you can set it to DB_MASTER in cases where you'd otherwise get bitten by replag. This normally used from outside of the class, so has little to do with updateSummaryFields which happens to also use it.

(unindent, reply to Special:Code/MediaWiki/111264#c30852)

That's an interesting idea. I could live with having these methods non-static in a class representing a table. I'm not exactly sure how the split would work. Would you have a table class and a row class for each table? If so, the table classes would not hold a lot of stuff, they would just hold these methods plus something that associates their corresponding row class (unless you have some kind of table registration system, in which case you could have one single table class which knows what to use if you specify which table you are talking about and return an object of the right row class).

I'd love to see more ideas on how this should be done.

Move all static methods to a table class. The static::foo() calls become $this->tableClass->foo()

You interface just with the table class and the objects it returns. The base row class could usually be the used one for many applications.

Sure. We could do that, but this does not answer my questions. Do we have 2 classes per table? How is the table class linked to the row one?

Theoretically yes. In practise, I'd like that the row class would be reusable, and in most classes you don't need to extend it.

The way the parent knows the name of the class to create, it can be a (const) variable or returned by a method.

> and in most classes you don't need to extend it.

In ALL cases for every extension I used this for, it would need to be extended, in most cases very non-trivially. After all, if you have a class Course that represents a row in the courses table, all course logic goes there right. So I don't think you'd gain much advantage by doing this, although there is of course no reason to go enforce deriving. Either way, I don't think this is an issue.

What I think is silly is that the table classes will often end up just holding the methods returning table specific data. I can't think of any table specific logic I'd put there (everything that can be derived from the info the class holds can be abstracted out to the parent class).

So good things of this approach:

• No need for LSB AFAICS
• Nicer separation between table and row specific functionality.

Things I like less:

• Two classes needed for almost all types instead of one.
• Table classes will likely just hold three or so methods returning table info.

In a scenario where PHP 5.3 could be used everywhere, I don't think the advantages would be worth the disadvantages, so I'm hesitant to make such changes.

The parent table could instead provide variables overriden in the child, that could make the overriding a bit shorter.

I prefer that separation between table/rows. If rows will hold a lot of logic, well, so be it. Two classes per table.

The use of static variables to store database connection objects, and indeed heavy use of static variables generally, is not acceptable. It's basically equivalent to using global variables, except for the syntax. It's the opposite to the direction we are heading with state architecture in MediaWiki.

> The use of static variables to store database connection objects

There is one static field readDb, which is clearly documented as storing an integer (element of DB_ enum), not a database connection object.

> heavy use of static variables generally

Like I said, there is one such field, and it's not part of the core functionality (so how does this count as heavy use?), so can be killed, or probably better, be replaced by an additional optional argument for all methods that read from the db.

Well, I split up the class and got rid of the late static binding stuff (see follow up revs). There might still be some PHP 5.3 stuff left, but I'll get rid of that. So it would seem the concerns expressed here are all addressed, so this should be changed to resolved I think.

That count() function is kind of scary. There's a reason DatabaseBase::estimateRowCount() exists, since a COUNT(*) query on large tables is bad.

It's not bad on small tables. But you are right, I ought to add a warning in the method docs.

r111502