r110716 MediaWiki - Code Review archive

Why not just add it as a third param to avoid breakage? What are the callers remaining?

No, this would break everything! See https://www.mediawiki.org/wiki/Manual:Hooks/UnknownAction how it was since years (first parameter is the original &action=value

Which revision did it change in?

I have no idea when this broke (in Wiki.php). Only those extensions are(were) affected which use the hook

https://www.mediawiki.org/wiki/Category:UnknownAction_extensions

For example, I debugged and fixed https://www.mediawiki.org/wiki/Extension:WikiArticleFeeds and found the problem with $act. This extension was working for years but stopped working when I upgraded to trunk version.

I really think, it's okay, as I verbatim copied the essential part out of the (previously called) getActionName() function line 102, when the action value is read from the request.

See https://svn.wikimedia.org/viewvc/mediawiki/branches/REL1_18/phase3/includes/Wiki.php?revision=107907&view=markup

In 1.18 $act was the result of getAction(), which isn't quite the same as what happens here.

But please try to understand the logic of the hook. It makes only sense to pass the original action value, as is documented in the Manual page.

So either the core code was wrong, or the Manual page. Please try to follow the logic of an Extension like WikiArticleFeeds (which triggers upon &action=feed ,. it did this for years, and could not be wrong)

Thanks.

As an additional argument for my patch, please see line 310 in https://svn.wikimedia.org/viewvc/mediawiki/trunk/extensions/WikiArticleFeeds/WikiArticleFeeds.php?annotate=110721

if ( $action != 'feed' ) return true;

So, an extension which consumes the UnknownAction hook must be able to read the original action value (even when this is "nosuchaction" for MW core).

I am convinced, that all the https://www.mediawiki.org/wiki/Category:UnknownAction_extensions Extensions were probably broken between 1.18 and revision r110716 .

I reset my patch "new", because the performance impact of the additional "$request->getVal( 'action', 'view' )" should be discussed; my patch should of course not degrade MW core performance.

Can someone say some words (and hopefully take away my concerns regarding mw performance degradation) ?

How would that effect performance? getVal() is just quick that function that works on the $_GET global. It has negligible speed impact.

Ok, fine. I was just not sure about this. I would like to ask you to set CR status now finally to "ok" again, thanks.

This is a follow-up to r109678.