r90331 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r90330‎ | r90331 | r90332 >
Date:09:17, 18 June 2011
Author:krinkle
Status:resolved (Comments)
Tags:
Comment:
Implement mw.Title in core

* Based on UploadWizard/resources/mw.Title.js

* Refactored to use local scope and prototypes instead of re-declaring them per-instance in the private scope through 'this' (re-use by reference, faster instantiation and performance)

* Fix potential ReferenceError in the check for wgArticlePath (inline if statements will fail for undeclared variables, needs typeof undefined check). Using mw.config instead to avoid this problem.

* The following two methods were not ported from UploadWizard because they were or became redundant and/or merged with another method:
-- setNameText (redundant with the improved setName)
-- setPrefix (redundant wit the improved setNamespace)

* Ported all jasmine tests to QUnit. Left them exactly the same to make sure it's compatible with UploadWizard. Perhaps I'll expand or adjust the suite later to be less file-specific, but for now make letting this revision go through TestSwarm to be sure it's compatible and behaves exactly the same.

* Added getName() method instead, replacing direct access to '_name' This in order to check for wgCaseSensitiveNamespaces (bug 29441; r90234)
-- Prevents breakages on wiktionary and other wikis with case sensitivity. ie. on a Wiktionary:
new mw.Title('apple').toString()
> "Apple"
-- This fix will make it return 'apple' instead of 'Apple' (that is, if 0 is in wgCaseSensitiveNamespaces).

* There used to be a strip-bug in scenarios where a namespace-like string appears inside of a title. Imagine pagename: "Category:Wikipedia:Foo bar" (exist on several wikis; NS_CATEGORY= 14)

new mw.Title( 'Wikipedia:Foo bar', 14 ).toString()
> "Category:Foo_bar" // Stripped "Wikipedia:" !!

In order to fix this:
-- Removed assumption that every title has a namespace prefix. UploadWizard/mw.Title has one initialization RegExp (which was ported as-is to "setAll"). In addition there is now a "setNameAndExtension" method (which doesn't extract or set the namespace). Now the above case:

new mw.Title( 'Wikipedia:Foo bar', 14 ).toString()
> "Category:Wikipedia_Foo_bar" // Better, but now the colon is gone..

-- In order to fix that, "\x3a" was removed from the clean() function. Colons are valid in MediaWiki titles, no need to escape.

new mw.Title( 'Wikipedia:Foo bar', 14 ).toString()
> "Category:Wikipedia:Foo_bar" // Yay!

* Last but not least, another little bug fixed due to the previous point. It also fixed a thrown exception in case a colon is part of the title in the main namespace (not rare for movies and books):

new mw.Title( 'The Wiki: Awesomeness')
> Error: mw.Title> Unrecognized canonical namespace: the_wiki

This exception is thrown from setNamespace(). That exception would make sense if setNamespace() was called by the user direcly, but when called from setAll() it should gracefully fallback by putting the prefix in the name instead and assuming NS_MAIN (just like the server side does). To achieve this I added a try/catch around setAll() and fallback to the new setNameAndExtension().

* Passes JSHint.

* Additional feature: exists(). Return true/false if known, otherwise null. Extensions can populate this for titles they are interested in and the front-end can construct url's and UI elements with correct redlink-status. Gadgets can populate it as well but that would require an API-request to get the information. A bit of a stub for later use, although I think it works fine.

* Bugfix in jquery.qunit.completenessTest.js (first triggered by the introduction of mw.Title). Don't traverse the 'constructor' property (recursive loop, ouch!)

---

(bug 29397) Implement mw.Title module in core
Modified paths:
  • /trunk/phase3/RELEASE-NOTES-1.19 (modified) (history)
  • /trunk/phase3/resources/Resources.php (modified) (history)
  • /trunk/phase3/resources/jquery/jquery.qunit.completenessTest.js (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.Title.js (added) (history)
  • /trunk/phase3/tests/qunit/index.html (modified) (history)
  • /trunk/phase3/tests/qunit/jquery.qunit.completenessTest.config.js (modified) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES-1.19
@@ -53,6 +53,7 @@
5454 * (bug 28904) Update jQuery version from 1.4.4 to 1.6.1 (the latest version)
5555 * (bug 29441) Expose CapitalLinks config in JS to allow modules to properly
5656 handle titles on case-sensitive wikis.
 57+* (bug 29397) Implement mw.Title module in core.
5758
5859 === Bug fixes in 1.19 ===
5960 * (bug 28868) Show total pages in the subtitle of an image on the
Index: trunk/phase3/tests/qunit/index.html
@@ -36,6 +36,7 @@
3737 <script src="../../resources/jquery/jquery.autoEllipsis.js"></script>
3838 <script src="../../resources/jquery/jquery.colorUtil.js"></script>
3939 <script src="../../resources/jquery/jquery.tabIndex.js"></script>
 40+ <script src="../../resources/mediawiki/mediawiki.Title.js"></script>
4041
4142 <!-- QUnit: Load framework -->
4243 <link rel="stylesheet" href="../../resources/jquery/jquery.qunit.css" />
@@ -52,6 +53,7 @@
5354 <script src="suites/resources/jquery/jquery.autoEllipsis.js"></script>
5455 <script src="suites/resources/jquery/jquery.colorUtil.js"></script>
5556 <script src="suites/resources/jquery/jquery.tabIndex.js"></script>
 57+ <script src="suites/resources/mediawiki/mediawiki.Title.js"></script>
5658
5759 <!-- TestSwarm: If a test swarm is running this,
5860 the following script will allow it to extract the results.
Index: trunk/phase3/tests/qunit/jquery.qunit.completenessTest.config.js
@@ -1,13 +1,18 @@
22 // Return true to ignore
33 var mwTestIgnore = function( val, tester, funcPath ) {
44
5 - // Don't record methods of the properties of mw.Map instances
6 - // Because we're therefor skipping any injection for
7 - // "new mw.Map()", manually set it to true here.
 5+ // Don't record methods of the properties of constructors,
 6+ // to avoid getting into a loop (prototype.constructor.prototype..).
 7+ // Since we're therefor skipping any injection for
 8+ // "new mw.Foo()", manually set it to true here.
89 if ( val instanceof mw.Map ) {
910 tester.methodCallTracker['Map'] = true;
1011 return true;
1112 }
 13+ if ( val instanceof mw.Title ) {
 14+ tester.methodCallTracker['Title'] = true;
 15+ return true;
 16+ }
1217
1318 // Don't record methods of the properties of a jQuery object
1419 if ( val instanceof $ ) {
Index: trunk/phase3/resources/jquery/jquery.qunit.completenessTest.js
@@ -155,6 +155,7 @@
156156
157157 // ...the prototypes are fine tho
158158 $.each( currVar.prototype, function( key, value ) {
 159+ if ( key === 'constructor' ) return;
159160
160161 // Clone and brake reference to parentPathArray
161162 var tmpPathArray = $.extend( [], parentPathArray );
@@ -182,6 +183,7 @@
183184
184185 // ... the prototypes are fine tho
185186 $.each( currVar.prototype, function( key, value ) {
 187+ if ( key === 'constructor' ) return;
186188
187189 // Clone and brake reference to parentPathArray
188190 var tmpPathArray = $.extend( [], parentPathArray );
Index: trunk/phase3/resources/Resources.php
@@ -439,6 +439,9 @@
440440 'mediawiki.htmlform' => array(
441441 'scripts' => 'resources/mediawiki/mediawiki.htmlform.js',
442442 ),
 443+ 'mediawiki.Title' => array(
 444+ 'scripts' => 'resources/mediawiki/mediawiki.Title.js',
 445+ ),
443446 'mediawiki.user' => array(
444447 'scripts' => 'resources/mediawiki/mediawiki.user.js',
445448 'dependencies' => array(
Index: trunk/phase3/resources/mediawiki/mediawiki.Title.js
@@ -0,0 +1,316 @@
 2+/**
 3+ * mediaWiki.Title
 4+ *
 5+ * @author Neil Kandalgaonkar, 2010
 6+ * @author Timo Tijhof, 2011
 7+ * @since 1.19
 8+ *
 9+ * Relies on: mw.config (wgFormattedNamespaces, wgNamespaceIds, wgCaseSensitiveNamespaces), mw.util.wikiGetlink
 10+ */
 11+(function( $ ) {
 12+
 13+ /* Local space */
 14+
 15+ /**
 16+ * Title
 17+ * @constructor
 18+ *
 19+ * @param title {String} Title of the page. If no second argument given,
 20+ * this will be searched for a namespace.
 21+ * @param namespace {Number} (optional) Namespace id. If given, title will be taken as-is.
 22+ * @return {Title} this
 23+ */
 24+var Title = function( title, namespace ) {
 25+ this._ns = 0; // integer namespace id
 26+ this._name = null; // name in canonical 'database' form
 27+ this._ext = null; // extension
 28+
 29+ if ( arguments.length === 2 ) {
 30+ this.setNameAndExtension( title ).setNamespaceById( namespace );
 31+ } else if ( arguments.length === 1 ) {
 32+ // If title is like "Blabla: Hello" ignore exception by setNamespace(),
 33+ // and instead assume NS_MAIN and keep prefix
 34+ try {
 35+ this.setAll( title );
 36+ } catch(e) {
 37+ this.setNameAndExtension( title );
 38+ }
 39+ }
 40+ return this;
 41+ },
 42+
 43+ /**
 44+ * Strip some illegal chars: control chars, colon, less than, greater than,
 45+ * brackets, braces, pipe, whitespace and normal spaces. This still leaves some insanity
 46+ * intact, like unicode bidi chars, but it's a good start..
 47+ * @param s {String}
 48+ * @return {String}
 49+ */
 50+ clean = function( s ) {
 51+ if ( s !== undefined ) {
 52+ return s.replace( /[\x00-\x1f\x23\x3c\x3e\x5b\x5d\x7b\x7c\x7d\x7f\s]+/g, '_' );
 53+ }
 54+ },
 55+
 56+ /**
 57+ * Convert db-key to readable text.
 58+ * @param s {String}
 59+ * @return {String}
 60+ */
 61+ text = function ( s ) {
 62+ if ( s !== null && s !== undefined ) {
 63+ return s.replace( /_/g, ' ' );
 64+ } else {
 65+ return '';
 66+ }
 67+ };
 68+
 69+ /* Static space */
 70+
 71+ /**
 72+ * Wether this title exists on the wiki.
 73+ * @param title {mixed} prefixed db-key name (string) or instance of Title
 74+ * @return {mixed} Boolean true/false if the information is available. Otherwise null.
 75+ */
 76+ Title.exists = function( title ) {
 77+ var type = $.type( title ), obj = Title.exist.pages, match;
 78+ if ( type === 'string' ) {
 79+ match = obj[title];
 80+ } else if ( type === 'object' && title instanceof Title ) {
 81+ match = obj[title.toString()];
 82+ } else {
 83+ throw new Error( 'mw.Title.exists: title must be a string or an instance of Title' );
 84+ }
 85+ if ( typeof match === 'boolean' ) {
 86+ return match;
 87+ }
 88+ return null;
 89+ };
 90+
 91+ /**
 92+ * @var Title.exist {Object}
 93+ */
 94+ Title.exist = {
 95+ /**
 96+ * @var Title.exist.pages {Object} Keyed by PrefixedDb title.
 97+ * Boolean true value indicates page does exist.
 98+ */
 99+ pages: {},
 100+ /**
 101+ * @example Declare existing titles: Title.exist.set(['User:John_Doe', ...]);
 102+ * @example Declare titles inexisting: Title.exist.set(['File:Foo_bar.jpg', ...], false);
 103+ * @param titles {String|Array} Title(s) in strict prefixedDb title form.
 104+ * @param state {Boolean} (optional) State of the given titles. Defaults to true.
 105+ * @return {Boolean}
 106+ */
 107+ set: function( titles, state ) {
 108+ titles = $.isArray( titles ) ? titles : [titles];
 109+ state = state === undefined ? true : !!state;
 110+ var pages = this.pages, i, len = titles.length;
 111+ for ( i = 0; i < len; i++ ) {
 112+ pages[ titles[i] ] = state;
 113+ }
 114+ return true;
 115+ }
 116+ };
 117+
 118+ /* Public methods */
 119+
 120+ var fn = {
 121+ constructor: Title,
 122+ /**
 123+ * @param id {Number} Canonical namespace id.
 124+ * @return {mw.Title} this
 125+ */
 126+ setNamespaceById: function( id ) {
 127+ // wgFormattedNamespaces is an object of *string* key-vals,
 128+ var ns = mw.config.get( 'wgFormattedNamespaces' )[id.toString()];
 129+
 130+ // Cannot cast to boolean, ns may be '' (main namespace)
 131+ if ( ns === undefined ) {
 132+ this._ns = false;
 133+ } else {
 134+ this._ns = Number( id );
 135+ }
 136+ return this;
 137+ },
 138+
 139+ /**
 140+ * Set namespace by any known namespace/id pair (localized, canonical or alias)
 141+ * On a German wiki this could be 'File', 'Datei', 'Image' or even 'Bild' for NS_FILE.
 142+ * @param ns {String} A namespace name (case insensitive, space insensitive)
 143+ * @return {mw.Title} this
 144+ */
 145+ setNamespace: function( ns ) {
 146+ ns = clean( $.trim( ns.toLowerCase() ) ); // Normalize
 147+ var id = mw.config.get( 'wgNamespaceIds' )[ns];
 148+ if ( id === undefined ) {
 149+ throw new Error( 'mw.Title: Unrecognized canonical namespace: ' + ns );
 150+ }
 151+ return this.setNamespaceById( id );
 152+ },
 153+
 154+ /**
 155+ * Get the namespace number.
 156+ * @return {Number}
 157+ */
 158+ getNamespaceId: function(){
 159+ return this._ns;
 160+ },
 161+
 162+ /**
 163+ * Get the namespace prefix (in the content-language).
 164+ * In NS_MAIN this is '', otherwise namespace name plus ':'
 165+ * @return {String}
 166+ */
 167+ getNamespacePrefix: function(){
 168+ return mw.config.get( 'wgFormattedNamespaces' )[this._ns].replace( / /g, '_' ) + (this._ns === 0 ? '' : ':');
 169+ },
 170+
 171+ /**
 172+ * Set the "name" portion, removing illegal characters.
 173+ * @param s {String} Page name (without namespace prefix)
 174+ * @return {mw.Title} this
 175+ */
 176+ setName: function( s ) {
 177+ this._name = clean( $.trim( s ) );
 178+ return this;
 179+ },
 180+
 181+ /**
 182+ * The name, like "Foo_bar"
 183+ * @return {String}
 184+ */
 185+ getName: function() {
 186+ if ( $.inArray( this._ns, mw.config.get( 'wgCaseSensitiveNamespaces' ) ) !== -1 ) {
 187+ return this._name;
 188+ } else {
 189+ return $.ucFirst( this._name );
 190+ }
 191+ },
 192+
 193+ /**
 194+ * The name, like "Foo bar"
 195+ * @return {String}
 196+ */
 197+ getNameText: function() {
 198+ return text( this.getName() );
 199+ },
 200+
 201+ /**
 202+ * Get full name in prefixed DB form, like File:Foo_bar.jpg,
 203+ * most useful for API calls, anything that must identify the "title".
 204+ */
 205+ getPrefixedDb: function() {
 206+ return this.getNamespacePrefix() + this.getMain();
 207+ },
 208+
 209+ /**
 210+ * Get full name in text form, like "File:Foo bar.jpg".
 211+ * @return {String}
 212+ */
 213+ getPrefixedText: function() {
 214+ return text( this.getPrefixedDb() );
 215+ },
 216+
 217+ /**
 218+ * The main title (without namespace), like "Foo_bar.jpg"
 219+ * @return {String}
 220+ */
 221+ getMain: function() {
 222+ return this.getName() + this.getDotExtension();
 223+ },
 224+
 225+ /**
 226+ * The "text" form, like "Foo bar.jpg"
 227+ * @return {String}
 228+ */
 229+ getMainText: function() {
 230+ return text( this.getMain() );
 231+ },
 232+
 233+ /**
 234+ * Set the "extension" portion, removing illegal characters.
 235+ * @param s {String}
 236+ * @return {mw.Title} this
 237+ */
 238+ setExtension: function( s ) {
 239+ this._ext = clean( s.toLowerCase() );
 240+ return this;
 241+ },
 242+
 243+ /**
 244+ * Get the extension (returns null if there was none)
 245+ * @return {String|null} extension
 246+ */
 247+ getExtension: function() {
 248+ return this._ext;
 249+ },
 250+
 251+ /**
 252+ * Convenience method: return string like ".jpg", or "" if no extension
 253+ * @return {String}
 254+ */
 255+ getDotExtension: function() {
 256+ return this._ext === null ? '' : '.' + this._ext;
 257+ },
 258+
 259+ /**
 260+ * @param s {String}
 261+ * @return {mw.Title} this
 262+ */
 263+ setAll: function( s ) {
 264+ var matches = s.match( /^(?:([^:]+):)?(.*?)(?:\.(\w{1,5}))?$/ );
 265+ if ( matches.length ) {
 266+ if ( matches[1] ) { this.setNamespace( matches[1] ); }
 267+ if ( matches[2] ) { this.setName( matches[2] ); }
 268+ if ( matches[3] ) { this.setExtension( matches[3] ); }
 269+ } else {
 270+ throw new Error( 'mw.Title: Could not parse title "' + s + '"' );
 271+ }
 272+ return this;
 273+ },
 274+
 275+ /**
 276+ * @param s {String}
 277+ * @return {mw.Title} this
 278+ */
 279+ setNameAndExtension: function( s ) {
 280+ var matches = s.match( /^(?:)?(.*?)(?:\.(\w{1,5}))?$/ );
 281+ if ( matches.length ) {
 282+ if ( matches[1] ) { this.setName( matches[1] ); }
 283+ if ( matches[2] ) { this.setExtension( matches[2] ); }
 284+ } else {
 285+ throw new Error( 'mw.Title: Could not parse title "' + s + '"' );
 286+ }
 287+ return this;
 288+ },
 289+
 290+ /**
 291+ * Return the URL to this title
 292+ * @return {String}
 293+ */
 294+ getUrl: function() {
 295+ return mw.util.wikiGetlink( this.toString() );
 296+ },
 297+
 298+ /**
 299+ * Wether this title exists on the wiki.
 300+ * @return {mixed} Boolean true/false if the information is available. Otherwise null.
 301+ */
 302+ exists: function() {
 303+ return Title.exists( this );
 304+ }
 305+ };
 306+
 307+ // Alias
 308+ fn.toString = fn.getPrefixedDb;
 309+ fn.toText = fn.getPrefixedText;
 310+
 311+ // Assign
 312+ Title.prototype = fn;
 313+
 314+ // Expose
 315+ mw.Title = Title;
 316+
 317+})(jQuery);
Property changes on: trunk/phase3/resources/mediawiki/mediawiki.Title.js
___________________________________________________________________
Added: svn:eol-style
1318 + native

Follow-up revisions

RevisionCommit summaryAuthorDate
r90350Test suite for mw.Title (Follow-up r90331). Right now it matches the test sui...krinkle17:44, 18 June 2011
r93702mediawiki.Title: Removing public setter functions. Title object should be imm...krinkle03:11, 2 August 2011
r946441.18: Back out mw.Title, which isn't used by anything in 1.18 (AJAXCategories...catrope15:22, 16 August 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r90234(bug 29441) Expose CapitalLinks config in JS to allow modules to properly han...krinkle21:20, 16 June 2011

Comments

#Comment by Krinkle (talk | contribs)   09:19, 18 June 2011

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

#Comment by Nikerabbit (talk | contribs)   13:07, 18 June 2011

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?

#Comment by Krinkle (talk | contribs)   12:24, 19 June 2011

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)

#Comment by Nikerabbit (talk | contribs)   14:30, 19 June 2011

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

#Comment by Krinkle (talk | contribs)   03:43, 2 August 2011

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.

#Comment by Catrope (talk | contribs)   14:34, 16 August 2011

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.

#Comment by Catrope (talk | contribs)   15:11, 16 August 2011

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

#Comment by Catrope (talk | contribs)   15:14, 16 August 2011

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

Status & tagging log