Page MenuHomePhabricator

Fix for MultimediaViewer $wgMediaViewerEnableByDefault
Closed, ResolvedPublic

Description

Again I found a minor bug and cannot submit patch directly to master branch.

In the file: https://phabricator.wikimedia.org/diffusion/EMMV/browse/master/MultimediaViewerHooks.php
Line 157
$wgDefaultUserOptions['multimediaviewer-enable'] = true;
Should be changed to
$wgDefaultUserOptions['multimediaviewer-enable'] = 1;

I am really surprised that a official extension not following the official guide......


https://www.mediawiki.org/wiki/Manual:$wgDefaultUserOptions
Examples
To disable section editing links by default (for new and anonymous users), set the following in LocalSettings.php:
$wgDefaultUserOptions['editsection'] = 0;

Event Timeline

Baskice created this task.Oct 18 2016, 4:44 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 18 2016, 4:44 AM

@Baskice:

Again I found a minor bug and cannot submit patch directly to master branch.

Why not?

I am really surprised that a official extension not following the official guide......

What "official guide"? Link welcome.

Arseny1992 closed this task as Invalid.EditedOct 18 2016, 1:57 PM
Arseny1992 added a subscriber: Arseny1992.

@Aklapper , the manual guide of rMW options for $wgDefaultUserOptions .
@Baskice , each extension is free to choose which value type to use on custom hooks added on top of default variables. Following an "official guide" designed for end-users is irrelevant in this case, the code guides that a developer needs to rely on, are on Developer hub (development policy and coding conventions)

See (pointing out that you said you are not a php coder in T148284) : in any language you have to define the type of an object: be it a boolean string that can return either true or false , or an integer .

See http://php.net/manual/en/language.types.boolean.php#80433

A boolean expresses a truth value.
It does not say "a boolean expresses a 0 or 1".

So

if ( $wgMediaViewerEnableByDefault ) {
  $wgDefaultUserOptions['multimediaviewer-enable'] = true;
}

means if $wgMediaViewerEnableByDefault is set (is true) then multimediaviewer-enable custom hook for $wgDefaultUserOptions is also true (a truth value expressing if the value is set or not)

On line 183 later in that same file rEMMV/MultimediaViewerHooks.php

if ( !$user->isLoggedIn() && isset( $wgMediaViewerEnableByDefaultForAnonymous ) ) {
  return (bool)$wgMediaViewerEnableByDefaultForAnonymous;
} else {
  return (bool)$user->getOption( 'multimediaviewer-enable' );
}

stands for

if user is NOT logged in AND $wgMediaViewerEnableByDefaultForAnonymous is set then return trueness explicitly (return (bool)) else also explicitly return(bool) for the earlier variable

Baskice added a comment.EditedOct 18 2016, 3:01 PM

@Arseny1992
All right, thank you for your clarification!
The problem were described here: https://www.mediawiki.org/wiki/Topic:T0mjz41gyrqntkrq

I am trying to fix the error that the login-ed user have to enable MultimediaViewer manually even if $wgMediaViewerEnableByDefault were set to true in MW 1.27 version. I tried the 1.27 snapshot and the master version MultimediaViewer. Neither of them react to $wgMediaViewerEnableByDefault = true; under MW 1.27.1.

By adding $wgDefaultUserOptions['multimediaviewer-enable'] = 1; to Localsettings.php, I could get all users enjoy MultimediaViewer by default, as a temporary fix.
Sorry for the wrong solution gave in post, and hope this error could be fixed soon. OR remove the $wgMediaViewerEnableByDefault from description page will be useful "fix" too.

Arseny1992 reopened this task as Open.Oct 18 2016, 4:25 PM

@Baskice so you mean if you do the fix you described , in your local copy, (only the require_once() in LocalSettings, and 1 for the expanded variable in the hook file) it does actually work? Which PHP version do you run? Perhaps PHP 7 handles some bool stuff differently than the ancient php (since 8 years) used by the commenters of php.net . Just to confirm your configuration. If works that way, then I can submit the patch

Arseny1992 closed this task as Invalid.EditedOct 18 2016, 8:19 PM

Closing as not reproducible

Tried to reproduce on both rMW REL_1.27 and master (using the appropriate branches of core, skin, extension)

IIS 8.5 / PHP 7.0.12 (cgi-fcgi) / MySQL 5.7.16-log

In both cases tested both the classic require_once() install method and wfLoadExtension( 'MultimediaViewer' ); (Actually almost all extensions manpages ( that support MediaWiki 1.25+ and call if ( function_exists( 'wfLoadExtension' ) ) { in extension-name.php ) need to be updated, as that way of installing extensions is up since 1.25)

Did not need to override $wgMediaViewerEnableByDefault = true; on localSettings, as that's already set in rEMMV /MultimediaViewerHooks.php on line 119 . Tested with explicit set, as expected led to no change, i.e. extension remains enabled in any case, both logged in and anonymous users.

Tgr added a subscriber: Tgr.Oct 18 2016, 8:43 PM

$wgDefaultUserOptions['multimediaviewer-enable'] = 1; seems like a reasonable thing to do as it would match other option handling logic. Just make sure it does not negatively affect existing behavior (especially the the AJAX updates for that option).

(Although as far as I see there should be no difference between true and 1... setting to false could be more problematic as it is stringified differently from 0.)

Change 316706 had a related patch set uploaded (by Arseny1992):
Update for $wgMediaViewerEnableByDefault

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

@Tgr feel free to amend this if needed, as per T71942#735506 you said there are some database complications if one overrides this to false

@Tgr In addition to said above, is this impacted in some way by T147537 ?

Jdlrobson reassigned this task from Arseny1992 to Tgr.Nov 8 2016, 11:35 AM
Jdlrobson added a subscriber: Jdlrobson.

(until we can get your input on task and patch)

Jdlrobson changed the task status from Open to Stalled.Dec 5 2016, 9:56 PM
dr0ptp4kt triaged this task as Normal priority.Apr 18 2017, 3:27 PM
dr0ptp4kt moved this task from Backlog to Next on the MediaViewer board.
MarkTraceur changed the task status from Stalled to Open.Sep 25 2017, 8:13 PM
MarkTraceur moved this task from To Do to Needs QA on the Multimedia-Team-Working-Board board.

Change 316706 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Update for $wgMediaViewerEnableByDefault

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

Ramsey-WMF moved this task from Untriaged to Next up on the Multimedia board.Oct 6 2017, 6:03 PM
Ramsey-WMF added a subscriber: Ramsey-WMF.

in progress.

Cparle added a subscriber: Cparle.Oct 10 2017, 10:44 AM

Erm ... what's up with this ticket? It has a merged patch, but according to jenkins "Post-merge build failed"

Jdlrobson closed this task as Resolved.Oct 11 2017, 5:28 PM

looks done to me!

Tgr added a comment.Oct 11 2017, 9:14 PM

@Cparle all the MediaViewer postmerge builds are failing. Looks like some kind of documentation problem (seems like jsduck checks are only run on merge?). Someone should look into that but it doesn't effect functionallity (but prevents the docs from being updated).