r103893 MediaWiki - Code Review archive

Oh right, I forgot. Also renamed MWNamespace::isMain to MWNamespace::isSubject:

• Because it's more consistent (we're using getSubject so isSubject makes sense).
• Because isMain is confusing, MWNamespace::isMain sounds almost like you're asking if the namespace you pass is NS_MAIN.

Of course I included a MWNamespace::isMain back compat method.

I see the benefit to abstracting away these comparisons (I finished your first section of your wikitech-l post thinking "whytf", but it actually makes some sense to create more flexibility in these comparisons), but this is not a very object-oriented or modular way of going about it. Surely the more elegant way of doing this is to rework the MWNamespace class into an actual lightweight object that can be instantiated, and return it from $title->getNamespace(). "is this namespace one of these namespaces" is logic that has nothing to do with Title objects, so it does not really belong in Title.php; we're trying to break up those huge ancient classes, not expand them. $title->getNamespace()->isOneOf( NS_FOO, NS_BAR ) is no harder to code, but rather cleaner, especially when we want to start expanding those comparison methods beyond stubs.

Sure... that's a possibility.

Though it'll have even more friction than this change. Every current getNamespace() == NS_USER may break and we'll have to have an entirely new class because MWNamespace has static functions and we'll need to switch to instance methods.

I'll think about that.

We'll probably need a new Title method though.

There are loads of ways to avoid breaking stuff:

1. Convert the NS_ constants to objects and thereby convert to doing object comparisons 'under the hood'.
2. Allow an implicit cast from a Namespace object to an int; would be nicer, but I don't think PHP allows it.
3. Redefine the NS_ constants as strings, and define the __toString() magic method on the Namespace class to return the namespace number as a string.
4. Add a $returnObj parameter to getNamespace() and gradually migrate calls to use it. Or add a separate getNamespaceObj() method to Title, and migrate similarly.
5. Just migrate stuff (this can be done in conjunction with one of the above). You've basically already said that all getNamespace() == NS_FOO calls will need to be replaced at some point anyway to accommodate new features; it's only slightly more disruptive to do it all in the space of a single release.

Constants must be scalar values: http://us2.php.net/define

And of course, the database can't store namespace objects into integer fields. Ultimately the DB schema comes down to an integer ID number, and that's what's stored in the various _namespace fields.

:)

You could play with magic toString methods to have them return the number, which might work, but ick! It feels like it would be very fragile.

I would love some nice lightweight namespace objects in a next-gen interface, but probably distinct to avoid conflicts and confusion -- eg, don't override existing getNamespace() methods but have a new method with a different name that returns an object; have things that take namespaces change to take either.

My preference is for getNamespaceObj(). Using __toString() will break in lots of subtle ways.

> class Foo { function __toString() { return '1'; } }

> $f = new Foo;

> var_dump($f == '1')
bool(true)

> var_dump($f == 1)
PHP Notice:  Object of class Foo could not be converted to int in ... : eval()'d code on line 1
bool(true)

> var_dump($f === 1)
bool(false)

> var_dump($f === '1')
bool(false)

getNamespaceObj was the one name I could come up with too. Though, TBH that '-Obj' pattern feels like that kind that extension developers will consider a little ugly and potentially decide to just opt to keeping getNamespace() without ever bothering to even start using the new interface in their code.

I almost wish we would have started with a bad naming scheme like getNS. If we were using a name like that we could make use of that as an excuse to switch to a better name like getNamespace for the new api... but we're not. I can't quite think of any name nicer than getNamespace.

> class Foo { function __toString() { return '1'; } }

> $f = new Foo;

> var_dump($f === '1')
bool(false)

That last test is probably the best example to never use __toString() :p

In the "$f == 1" case the notice is masking what an epic failure it is. So maybe this is a better example:

> class Foo { function __toString() { return '2'; } }

> $f = new Foo;

> var_dump($f == 2)
PHP Notice:  Object of class Foo could not be converted to int in ... eval()'d code on line 1
bool(false)

> print intval($f)
PHP Notice:  Object of class Foo could not be converted to int in ... eval()'d code on line 1
1