Page MenuHomePhabricator

wgRestrictionEdit should be defined (as an empty array) on Education Program pages
Closed, ResolvedPublic

Description

According to the docs, this is supposed to be an array (possibly empty) of strings, but on pages such as
https://pt.wikipedia.org/wiki/Ensino:Faculdade_C%C3%A1sper_L%C3%ADbero/Ci%C3%AAncia_Pol%C3%ADtica_%28R%C3%A1dio,_TV_e_Internet%29_%282016%29
it is actually null instead of []. This caused errors in one of our gadgets (which assumed that would be defined, and tested for mw.config.get( 'wgRestrictionEdit' ).length to determine if the array was empty or not).

Event Timeline

He7d3r created this task.Apr 21 2016, 1:15 PM
Restricted Application added subscribers: TerraCodes, Base, Aklapper. · View Herald TranscriptApr 21 2016, 1:15 PM
D3r1ck01 updated the task description. (Show Details)Nov 13 2017, 12:51 PM
D3r1ck01 added a project: Google-Code-in-2017.
D3r1ck01 added a subscriber: D3r1ck01.

Will mentor this for Google-Code-in-2017.

Aklapper updated the task description. (Show Details)Nov 20 2017, 1:51 PM
divadsn claimed this task.Dec 24 2017, 1:32 PM
divadsn added a subscriber: divadsn.

I will fix that issue :)

divadsn added a comment.EditedDec 25 2017, 1:31 AM

So after looking up the code I noticed that EducationProgram pages are not extending OutputPage, but implementing the Page interface in order to use a WikiPage instance, which does also not extend the OutputPage.
Looking further I noticed also that those JS vars are only applied for OutputPage instances in ParserOutput, so the only dirty way would be by adding that to includes/DefaultSettings.php.

In the end those Gadgets needs to be fixed manually by adding a default parameter, so instead of this:

mw.config.get( 'wgRestrictionEdit' ).length

It should be:

mw.config.get( 'wgRestrictionEdit', [] ).length

Which should then return an empty array if no such key exists, which should also then prevent the rest from erroring. Please correct me if my analysis is wrong. :)

Edit: Actually OutputPage does handle Education Program pages as well, but without restriction types.

Change 400134 had a related patch set uploaded (by Divadsn; owner: Divadsn):
[mediawiki/core@master] Always define wgRestrictionEdit as an empty array for pages

https://gerrit.wikimedia.org/r/400134

Change 400134 abandoned by Divadsn:
Always define wgRestrictionEdit as an empty array for pages

Reason:
Terrible idea, there should be a better way to fix that.

https://gerrit.wikimedia.org/r/400134

Change 400134 restored by Divadsn:
Always define wgRestrictionEdit as an empty array for pages

https://gerrit.wikimedia.org/r/400134

divadsn removed divadsn as the assignee of this task.Dec 26 2017, 9:08 AM
Umherirrender added a subscriber: Umherirrender.

According to Title::getFilteredRestrictionTypes the edit permission is not present for non-existing titles, so the javascript variable 'wgRestrictionEdit' is not set on client side (it is not null).

EducationProgram sets the known flag for the pages in hook TitleIsAlwaysKnown, but that is not checked. I am not sure, if that should be checked when setting the javascript variable or the restriction list in api [1]

Use mw.config.exists to check, if the variable is set or use the empty array as default.
Or create the page you want edit.
Or use hook TitleGetRestrictionTypes to set edit permission and remove the create permission similar to the TitleIsAlwaysKnown hook (seems the best solution)

Or update the documentation on mw.org

[1] https://pt.wikipedia.org/w/api.php?action=query&prop=info&inprop=protection&titles=Ensino:Faculdade%20C%C3%A1sper%20L%C3%ADbero/Ci%C3%AAncia%20Pol%C3%ADtica%20(R%C3%A1dio,%20TV%20e%20Internet)%20(2016)

{
    "batchcomplete": "",
    "query": {
        "pages": {
            "-1": {
                "ns": 446,
                "title": "Ensino:Faculdade C\u00e1sper L\u00edbero/Ci\u00eancia Pol\u00edtica (R\u00e1dio, TV e Internet) (2016)",
                "missing": "",
                "known": "",
                "contentmodel": "wikitext",
                "pagelanguage": "pt",
                "pagelanguagehtmlcode": "pt",
                "pagelanguagedir": "ltr",
                "protection": [],
                "restrictiontypes": [
                    "create"
                ]
            }
        }
    }
}

Couldn't this be changed somewhere in the extension itself? However: I updated the doc to say, that if a page does not exist, that the variable is not set.

Albert221 claimed this task.Jan 1 2018, 9:04 PM
This comment was removed by Albert221.

Change 400134 abandoned by Divadsn:
Always define wgRestrictionEdit as an empty array for pages

Reason:
As previously stated, terrible solution.

https://gerrit.wikimedia.org/r/400134

Here's the code that is eventually adding $wgRestrictionEdit to vars:

foreach ( $title->getRestrictionTypes() as $type ) {
    // Following keys are set in $vars:
    // wgRestrictionCreate, wgRestrictionEdit, wgRestrictionMove, wgRestrictionUpload
    $vars['wgRestriction' . ucfirst( $type )] = $title->getRestrictions( $type );
}

I really don't think that adding a wgRestrictionEdit index to $vars array as @divadsn did in his patch would be a good solution, because why edit restriction would be in this array and create, move and upload not? It would make no logical sense and would lead to confusing other contributors and just complicated code. I second what @Florian wrote, I'm going to look at this thing from other perspective :)

EDIT:
It also - theoretically - cannot be null on the backend side, because Title::getRestrictions() (used in code above) never returns null, either empty array or some array from Title::mRestrictions.

public function getRestrictions( $action ) {
    if ( !$this->mRestrictionsLoaded ) {
        $this->loadRestrictions();
    }
    return isset( $this->mRestrictions[$action] )
            ? $this->mRestrictions[$action]
            : [];
}
Albert221 added a comment.EditedJan 1 2018, 10:17 PM

The problem is not only with wgRestrictionEdit but with Move and Upload one too:

mw.config.get('wgRestrictionCreate')
> []
mw.config.get('wgRestrictionEdit')
> null
mw.config.get('wgRestrictionUpload')
> null
mw.config.get('wgRestrictionMove')
> null

so it tells us, that Title::getFilteredRestrictionTypes() method is being called with false parameter and then upload key is removed because namespace of the page is not a File.
This false parameter means, that page does not exist. It's from the Title::exists() that returns false.

As of the Title::exists() PHP documentation:

For historical reasons, this function simply
checks for the existence of the title in the page table, and will
thus return false for interwiki links, special pages and the like.
If you want to know if a title can be meaningfully viewed, you should
probably call the isKnown() method instead.

I've finally found the cause of this problem! It's because Title::getArticleID() returns Title::mArticleID which is... 0 and this makes Title::exists() return false!

So the solution for that problem is to manually set the mArticleID to something other than 0, but I don't know whether that wouldn't change in the MediawikiCore and I would like a response from @Florian about that :)

If that would help in this case (haven't checked), I would suggest to use the TitleExists hook and return true, whenever the EducationProgram page should be recognized as existing (I'm not sure, if there's a function to find out if such a page has content or something like that).

Change 401411 had a related patch set uploaded (by Albert221; owner: Albert221):
[mediawiki/extensions/EducationProgram@master] Fix wgRestrictionEdit not exists in mw.config

https://gerrit.wikimedia.org/r/401411

Florian closed this task as Resolved.Jan 3 2018, 9:54 AM

Change 401411 merged by jenkins-bot:
[mediawiki/extensions/EducationProgram@master] Fix wgRestrictionEdit not exists in mw.config

https://gerrit.wikimedia.org/r/401411