Last modified: 2011-03-13 17:45:57 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T19963, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 17963 - SkinTemplate.php's buildContentActionUrls() ignores protection for article and talk pages
SkinTemplate.php's buildContentActionUrls() ignores protection for article an...
Status: RESOLVED WONTFIX
Product: MediaWiki
Classification: Unclassified
Page protection (Other open bugs)
1.17.x
All All
: Lowest major (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-13 12:48 UTC by Dan Jacobson
Modified: 2011-03-13 17:45 UTC (History)
1 user (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Dan Jacobson 2009-03-13 12:48:18 UTC
SkinTemplate.php's buildContentActionUrls ignores protection for
article and talk pages.

It should be updated to deal with today's $wgNamespaceProtection etc.

You see, users are given (red)links to go edit Article and Talk pages
that they cannot possibly edit.

Users should not be even given any link at all, for pages that don't
exist and they have no rights to create.

In SkinTemplate.php let's take a look at this part of function buildContentActionUrls:

if( $this->iscontent ) {
	$subjpage = $this->mTitle->getSubjectPage();
	$talkpage = $this->mTitle->getTalkPage();
	$nskey = $this->mTitle->getNamespaceKey();
	$content_actions[$nskey] = $this->tabAction(
		$subjpage,$nskey,!$this->mTitle->isTalkPage() && !$prevent_active_tabs,'',true);
	$content_actions['talk'] = $this->tabAction($talkpage,'talk',
                $this->mTitle->isTalkPage() && !$prevent_active_tabs,'',true);
	if($this->mTitle->quickUserCan('edit')&&($this->mTitle->exists()||$this->mTitle->quickUserCan('create'))){

Well, the two $content_actions unfortunately come BEFORE the
quickUserCan tests.

This means the user could get (red)links to pages he cannot possibly create.

Also he will get redlinks to pages he cannot possibly edit, if they
already exist.

Case in point: In LocalSettings.php we set
$wgNamespaceProtection[NS_CATEGORY]=$wgNamespaceProtection[NS_CATEGORY_TALK]=array('editinterface');}
So, e.g., browsing a category page (with members, but no content), he
sees two redlinks, one to the category page itself, and one to the
category page's talk page (which doesn't exist.)

Well, he shouldn't see any link at all to the talk page.

And he should see a blue link to the category page itself.

And if he browses to a URL which doesn't exist, no talk page too, and he has no rights... he should see no tabs at all.
Comment 1 Dan Jacobson 2009-03-13 23:59:35 UTC
> come BEFORE the
Actually it is not as simple as placing them AFTER. They need their own tests.

Also one might say 'well, clean it up via a hook'.
Well, that would currently be very inefficient.
Also, it is a bug. Not just something one should just use a hook to workaround.
Comment 2 Dan Jacobson 2009-03-14 00:59:23 UTC
(I really am using 1.15 SVN. The code fragment above looks different because I took out the whitespace.)
Comment 3 Roan Kattouw 2009-03-17 15:59:13 UTC
(In reply to comment #0)
> SkinTemplate.php's buildContentActionUrls ignores protection for
> article and talk pages.
> 
> It should be updated to deal with today's $wgNamespaceProtection etc.
> 
It does deal with them by showing "view source" tabs instead of edit tabs above a protected page,

> You see, users are given (red)links to go edit Article and Talk pages
> that they cannot possibly edit.
> 
So? This happens for redlinks inside articles too.

> Users should not be even given any link at all, for pages that don't
> exist and they have no rights to create.
> 
That means not showing the tabs at all (a suggestion you repeat below), which is pretty confusing, especially for the article/talk tab.

Recommend WONTFIX: hiding these tabs doesn't accomplish much and will only confuse people.
Comment 4 Dan Jacobson 2009-03-17 20:31:50 UTC
> It does deal with them by showing "view source" tabs instead of edit tabs above
> a protected page,

Let's talk about when the talk page doesn't exist.
You give them a red link inviting them to go edit a talk page that
doesn't exist and they have no permission to edit.

> So? This happens for redlinks inside articles too.

That can be blamed on authors. But we're talking about
the MediaWiki provided links at the tops of pages here.

You don't give them a link to delete a page that they don't have any
permission to delete, so why then give them a talk link that they will
totally regret clicking?

> That means not showing the tabs at all (a suggestion you repeat below), which
> is pretty confusing, especially for the article/talk tab.

At least don't give them a "bait and switch" talk tab.

It is rare that a talk page would exist, but not its article. So it is
safe to check if "talk page exists or we have permission to create it"
before making a talk link, (red or blue,) ... please.
Comment 5 Roan Kattouw 2009-03-17 22:10:18 UTC
(In reply to comment #4)
> > It does deal with them by showing "view source" tabs instead of edit tabs above
> > a protected page,
> 
> Let's talk about when the talk page doesn't exist.
> You give them a red link inviting them to go edit a talk page that
> doesn't exist and they have no permission to edit.
> 
That'd confuse people, because they expect the talk tab to be there. When the talk tab is just gone, it's far from obvious that that's because the talk page doesn't exist and can't be created by the user.

> You don't give them a link to delete a page that they don't have any
> permission to delete,
True, delete tabs are hidden for users without delete rights, and move tabs are hidden on move-protected pages. Still, these are different: move and delete are actions, the article/talk tabs are for navigation.

> so why then give them a talk link that they will
> totally regret clicking?
> 
"totally regret" is kind of hyperbolic: seeing a page that says you're not allowed to create the talk page and then clicking Back to get back to where you were is not a big deal. Also, I believe the principle of least surprise applies here: showing a link that leads to an error message is less confusing than not showing the link at all when people expect it to be there.

> > That means not showing the tabs at all (a suggestion you repeat below), which
> > is pretty confusing, especially for the article/talk tab.
> 
> At least don't give them a "bait and switch" talk tab.
> 
Then what do you suggest? You either show the tab or you don't, and not showing it is the more confusing option.

Closing this bug as WONTFIX, since it requests something that'll harm rather than improve usability.
Comment 6 Dan Jacobson 2009-03-18 05:59:36 UTC
Ah: a greyed out unclickable talk link! But wait, that would make the
user ever more furious: "why won't you let me even read it!?"

Also greyed out unclickable links would require big code enhancement. Hmmm.

Wait, I have made a workaround via a hook to undo the ugliness after it has been made:

function JidanniLessRedContentActions($sktemplate,$content_actions){
  if('new'==$content_actions['talk']['class']&&!$sktemplate->mTitle->quickUserCan('createtalk')){
	unset($content_actions['talk']);}
  if('selected new'==$content_actions['nstab-category']['class']){
	$content_actions['nstab-category']['class']='selected';}
  return true;}
$wgHooks['SkinTemplateTabs'][]='JidanniLessRedContentActions';

I'll use my hook for now...
Comment 7 Dan Jacobson 2010-06-04 04:21:23 UTC
Well now here we are in 2010 and Vector is the default skin in CVS which is what my wikis use. And now most of my workarounds need to researched and rewritten for Vector, to prevent my website from looking ugly which it has now become again.
Comment 8 Dan Jacobson 2010-11-21 22:52:40 UTC
(In reply to comment #6)
> function JidanniLessRedContentActions($sktemplate,$content_actions)
OK, I finally figured out how to do that in Vector. For the record here it is:
  function JidanniLessRedContentActionsVectorTypeSkins($sktemplate,$links){
    if(isset($links['namespaces']['talk']['class'])&&
       'new'==$links['namespaces']['talk']['class']&&
       !$sktemplate->mTitle->quickUserCan('createtalk')){
          unset($links['namespaces']['talk']);
	  if(isset($links['actions']['watch'])){unset($links['actions']['watch']);};};
    if(isset($links['namespaces']['category_talk']['class'])&&
       'new'==$links['namespaces']['category_talk']['class']&&
       !$sktemplate->mTitle->quickUserCan('createtalk')){
          unset($links['namespaces']['category_talk']);
	  if(isset($links['actions']['watch'])){unset($links['actions']['watch']);};};
    if(isset($links['namespaces']['category']['class'])&&
       'selected new'==$links['namespaces']['category']['class']){
          $links['namespaces']['category']['class']='selected';}
	  return true;} $wgHooks['SkinTemplateNavigation'][]='JidanniLessRedContentActionsVectorTypeSkins';}

Odd that when one uses vector, both hooks mention above get run...
Comment 9 Dan Jacobson 2010-12-06 10:03:33 UTC
(In reply to comment #8) even better, just for the record,
in case anybody needs it:
  function JidanniLessRedContentActionsVectorTypeSkins($sktemplate,$links){
    if(isset($links['namespaces'])&&
       is_array($links['namespaces'])&&
       !$sktemplate->mTitle->quickUserCan('createtalk')){
      foreach(array_keys($links['namespaces']) as $ns){
	if(strpos($ns,'talk')!==false){
	  if(isset($links['namespaces'][$ns]['class'])&&
	     'new'==$links['namespaces'][$ns]['class']){
	    unset($links['namespaces'][$ns]);}}}
      if(isset($links['actions']['watch'])){unset($links['actions']['watch']);}}
    if(isset($links['namespaces']['category']['class'])&&
       'selected new'==$links['namespaces']['category']['class']){
      $links['namespaces']['category']['class']='selected';}
    return true;}
  $wgHooks['SkinTemplateNavigation'][]='JidanniLessRedContentActionsVectorTypeSkins';
Comment 10 Mark A. Hershberger 2011-03-13 17:45:57 UTC
Changing all WONTFIX high priority bugs to lowest priority (no mail should be generated since I turned it off for this.)

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links