r90331 MediaWiki - Code Review archive

On-wiki documentation on RL/DM and more unit tests for the parts that are new compared to UploadWizard still need to be written.

PHP Title class is immutable, this is not? PHP Title class doesn't handle file extensions, this one does and confusingly calls them just extensions?

I'm not sure what you are referring to with this constructor not being immutable.

• Constructor methods

JavaScript constructors are always extendable after their initial declaration (via the .prototype object). This is in contrast with PHP classes which can only be declared once (and extended under a different name, but the original is immutable). This is simply the way it is, nothing anyone can do about that.

• Existiness

If you meant the existence check rather than the class/constructor itself I see what you mean. At some point calling .exist() may return null, at another it may return true or false. This depends on whether the titles have already been loaded at this point. Although it could be worked around I think the way it currently is has the least downsides. It's no different than the mw.messages system we have. Modules define which messages they need, the front-end and back-end make sure the messages are defined before the module executes. Very efficient, all in one request. The same can be done for titles. The server side module registry simply inserts part of it's code dynamically like:

mw.Title.exist.add(['Foo', 'Bar', 'User_talk:John_Doe', 'Main_Page', 'MediaWiki:Gadget-Foobar.js]);

Or if script that is entirely managed from the front-end wishes to use this functionality (such as a Gadget) it can make a single API request to get the information for the titles it wants, and make the call to exist.add() based on that in the callback.

If you meant something else with it not being immutable, Sorry for that, please re-phrase what you meant.

Extensions

I wasn't planning to include support for extensions in the module originally, but I noticed the module NeilK wrote for UploadWizard had it included and after thinking about it made sense to integrate it because it does enable doing some cool things, with very little code. That, and it should be compatible with the one in UploadWizard.

Extensions are not exclusive to the File-namespace. They also appear in the MediaWiki-namespace (MediaWiki:Vector.js, MediaWiki:Common.css, MediaWiki:Group-Sysop.js MediaWiki:Gadget-Foobar.js), and the User-namespace (User:John/common.js, User:John/vector.css). And some wikis also in other namespaces (like Wikipedia;User_scripts/Foobar/Baz.js). Since they aren't actual files I found "file extension" inappropriate (and long), and AFAIK there are no other kind of extensions related to titles, so it seemed like the right choise. I'm open to suggestions though, so feel free to give suggestions (or make the change yourself)

I mean that Title class has not setAnything method. Once constructed the object is immutable.

Sounds reasonable, removed them as public methods in r93702.

Also removing 'needs-js-test' tag as it the CompletenessTest now indicates 0 missing tests for this module as of r93702.

Reviewing mediawiki.Title.js at trunk state, all comments here in lieu.

+		if ( s !== undefined ) {
...
+		if ( s !== null && s !== undefined ) {

Suggest using typeof s == 'string' instead, because you're about to call s.replace(). Also, if clean() and text() check the type of their input, so should fixExt().

		// toLowerCase throws exception on null/undefined. Return early.
		if ( ns == null ) {
			return false;
		}

Why not check typeof ns == 'string' here too?

		var matches = raw.match( /^(?:)?(.*?)(?:\.(\w{1,5}))?$/ );

(?:)? means 'non-capturing match of emptiness, optional', so that's totally useless. I quickly tried removing it locally and it didn't break any tests; I can't imagine it's actually doing anything other than confusing the hell out of anyone who reads that regex.

Why does Title.exist.set() return anything? It always returns true, there is no way it will ever return anything else, so it might as well be a void function.

Why do the methods have such weird names? Is that a holdover from the UploadWizard code? Wouldn't it make sense to rename some of them to be consistent with Title.php, where possible? (Some won't be able to be renamed conveniently because Title.php deals with interwikis but not with extensions. But things like getPrefixedDb vs. getPrefixedDbKey are just stupid.)

Marking resolved because the issues are minor and mw.title isn't widely used at this time, only in UploadWizard and AJAXCategories.

Also, please unpick your commits: don't combine the addition of a new module with changes to another random component (completeness test).

Hm, turns out they weren't so random after all. Ignore that comment.