Last modified: 2013-10-31 11:52:18 UTC
Created attachment 9023 [details] patch for passing query string mw.util.wikiGetlink should allow query parameters to be passed and thus effect the relative URL created. Examples: mw.util.wikiGetlink({ title: 'Main Page', action: 'edit' }); mw.util.wikiGetlink( "Main Page", { action: }) Both return: /w/index.php?title=Main_Page&action=edit While: mw.util.wikiGetlink( "Main Page" ); mw.util.wikiGetlink({ title: "Main Page" }) Both return "/wiki/Main_Page" Patch included.
Comment on attachment 9023 [details] patch for passing query string --- mediawiki.util.js 2011-09-07 11:10:17.000000000 -0500 +++ mediawiki-new.util.js 2011-09-07 12:04:26.000000000 -0500 @@ -124,12 +124,24 @@ /** * Get the link to a page name (relative to wgServer) * - * @param str string Page name to get the link for. - * @return string Location for a page with name of 'str' or boolean false on error. + * @param page {String} Page name to get the link for. + * @param [params] {Object} Parameters to pass with page. + * @return {String} Relative URL for page. */ - 'wikiGetlink' : function( str ) { - return mw.config.get( 'wgArticlePath' ).replace( '$1', - this.wikiUrlencode( str || mw.config.get( 'wgPageName' ) ) ); + 'wikiGetlink' : function( page, params ) { + if ( $.isPlainObject( page ) ) { + params = page; + page = params.title || mw.config.get( 'wgPageName' ); + } else if ( !page ) { + page = mw.config.get( 'wgPageName' ); + } + if ( 'title' in params ) { + delete params.title; + } + if ( $.isEmptyObject( params ) ) { + return mw.config.get( 'wgArticlePath' ).replace( '$1', this.wikiUrlencode( page ) ); + } + return mw.config.get( 'wgScript' ) + '?title=' + this.wikiUrlencode( page ) + $.param( params ); }, /**
Created attachment 9024 [details] patch to allow passing of query string accidentally included whole file rather than patch first time, and patch edit added a comment instead of what I expected.
Avoid modifying the given params map directly; the caller might be expecting to reuse it on multiple calls etc. Looks like the code makes the page parameter optional, but only params is marked as such -- probably should list [page] in the doc comments too. If allowing parameters such as other actions, $wgActionPaths may apply to canonical URL generation, which would not be picked up by the current code.
Created attachment 9039 [details] clone params, use wgActionPaths, mark page as optional, and decode titles Marked page as optional. Cloned params so changes doesn't effect the original. Used wgActionPaths for URL generation when possible. Added rawurldecode and wikiUrldecode for use in URL generation.
Assigning to myself to review and commit later. Note to self: * Base could have a ? already (see wfAppendQuery) * Add tests for cases where base has ?, as well as for action paths on combination with only passing title, only action, only extra query, and combinations. * Perhaps possible without needing a decode function
(In reply to comment #5) > Assigning to myself to review and commit later. > > Note to self: > * Base could have a ? already (see wfAppendQuery) > * Add tests for cases where base has ?, as well as for action paths on > combination with only passing title, only action, only extra query, and > combinations. > * Perhaps possible without needing a decode function I think assuming "?" at the end of page means a query string is an error. I believe wikiGetlink is intended to expand the page parameter in basically the same way as [[Page]] does, which includes encoding "?". If that is not done than wikiGetlink won't be able to be used for pages which include a "?" in their name. Though come to think of it, I didn't check for section anchors. Those need to be encoded differently. A check for the case where an anchor without a title is used is needed too.
darklama, thanks for the patch. Adding "need-review" so that other developers know this patch needs review, in case they have time before Krinkle does. And darklama, if you have time to revise and update your patch in response to the critique Krinkle has already given, we would welcome that as well. Thanks!
darklama, thanks again for the patch. After you revise your patch, are you interested in using developer access to directly suggest it into our Git source control system? That might get more code review and faster. https://www.mediawiki.org/wiki/Developer_access https://www.mediawiki.org/wiki/Git/Workflow#How_to_submit_a_patch
Looking at this patch again, I think this is becoming too complex for a simple utility function that gets the url of a page name. There is handling for all of this already in the mw.Uri module and also integrated with the mw.Title module. mw.util is a arbitrary/simple utilities, of which most really belong in a separate module. Adding features like this means re-inventing wheels that have already been polished in other modules. The Uri and Title modules aren't all that big but do their job really well (including handling for hash fragments, which you mentioned), so if you need something like this in your code, I'd say just use those modules directly and be done with it. I see your patch also contains handling for action paths, which is nice. I'm not sure if mw.Title / mw.Uri support that right now. If it makes sense to have there, that would be great to have!
(In reply to comment #9) > There is handling for all of this already in the mw.Uri module and also > integrated with the mw.Title module. mw.util is a arbitrary/simple utilities, > of which most really belong in a separate module. Adding features like this > means re-inventing wheels that have already been polished in other modules. > > The Uri and Title modules aren't all that big but do their job > really well (including handling for hash fragments, which you > mentioned), so if you need something like this in your code, I'd > say just use those modules directly and be done with it. The mw.Uri module handles parsing and proper encoding of URIs that is compliant with the URI spec. However Titles are allowed to include "?" which must be encoded to avoid interpretation as a query string when converted into URI. Titles are not allowed to contain some character which are valid in a URI, like "[" and "|". Hashes seem to be encoded differently from the URI spec as well. mw.Uri and mw.Title operate in isolation without dependency or understanding of the needs of the other. As a consequence of this isolation, none of these things are handled by mw.Uri or mw.Title. mw.Title depends on mw.util.wikiGetlink for proper conversion from Title or URI, but there are bugs and missing features. > I see your patch also contains handling for action paths, which is nice. > I'm not sure if mw.Title / mw.Uri support that right now. If it makes > sense to have there, that would be great to have! mw.Title and mw.Uri does not support that right now, just as they do not support conversation between each other.
The relevance of the question mark presence is indeed not related to the page name itself. However it is related to the url formatting. Because wgArticlePath could look like this: "/w/index.php?title=$1" Then an additional query string has to start with `&` instead of `?`. Any question mark in the title will be url encoded so a string search for that shouldn't be match the page name. (In reply to comment #10) > mw.Title and mw.Uri does not support that right now, just as they do not > support conversation between each other. That sounds like a good place to start. There is something to be said about keeping mw.Uri somewhat re-usable, so the special casing for 'title' and 'action' should ideally be handled through some kind of hook. ("paramHooks" callbacks?) so that it is easy to extend or disable in the future. Then from there we can make mw.Title use mw.Uri for url construction (returning an mw.Uri object, which users of mw.Title can optionally convert to String or then call a method on to add query parameters.
(Reviewing attachment 9039 [details]) >--- mediawiki.util.js >+++ mediawiki.util.js >@@ -110,7 +110,17 @@ > }, > > /** >- * Encode page titles for use in a URL >+ * Decode the string like PHP's rawurldecode >+ * >+ * @param str {String} String to be decoded >+ */ >+ 'rawurldecode' : function( str ) { >+ str = ( str + '' ).toString(); >+ return decodeURIComponent( str ); This doesn't make sense. This is triple conversion to string: * + '' * toString * [internal] decodeURIComponent casts value `str = String(value)` Suggest to use String(str) (not `new String`) instead of '' and/or toString So just `return decodeURIComponent( String( str ) );` >+ }, >+ >+ /** > * Get the link to a page name (relative to wgServer) > */ >- 'wikiGetlink' : function( str ) { >- return mw.config.get( 'wgArticlePath' ).replace( '$1', >- this.wikiUrlencode( str || mw.config.get( 'wgPageName' ) ) ); >+ 'wikiGetlink' : function( page, params ) { >+ var url, query; >+ if ( $.isPlainObject( page ) ) { >+ params = page; >+ page = params.title || mw.config.get( 'wgPageName' ); >+ } else if ( !page ) { >+ page = mw.config.get( 'wgPageName' ); >+ } >+ query = $.extend( params || {} , {} ); >+ if ( 'title' in query ) { >+ delete query.title; >+ } >+ if ( $.isEmptyObject( query ) ) { >+ return mw.config.get( 'wgArticlePath' ).replace( '$1', this.wikiUrldecode( page ) ); Why is this decoding the page parameter? This is a url, it should be encoded. >+ } >+ if ( 'action' in query ) { >+ url = mw.config.get( 'wgActionPaths', {} )[ query.action ]; >+ if ( url ) { >+ url = url.replace( '$1', this.wikiUrldecode( page ) ) + '?'; Why is this decoding the page parameter? It should be encoded instead of decoded. >+ delete query.action; >+ } >+ } >+ if ( !url ) { >+ url = mw.config.get( 'wgScript' ) + '?title=' + this.wikiUrlencode( page ) + '&'; >+ } >+ return url + $.param( query ); > }, > > /** * The wikiUrldecode method appears to be unneeded. * Needs unit tests.
Change-Id: I5224a0910a822f1c3b1b34f505dbcdf879052b39
*** Bug 34743 has been marked as a duplicate of this bug. ***
