Fix for MultimediaViewer $wgMediaViewerEnableByDefault
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Baskice
	Oct 18 2016, 4:44 AM

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;

Details

Related Changes in Gerrit:

	Subject	Repo	Branch	Lines +/-
	Update for $wgMediaViewerEnableByDefault	mediawiki/extensions/MultimediaViewer	master	+1 -1

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	None	T340453 [Epic] FY 2023-24 Growth Maintenance Work
Resolved	KStoller-WMF	T347840 [Epic] Q2 FY 2023-24 Growth Maintenance Work
Open	None	T54777 user_properties table bloat
Open	None	T71942 Updating user preferences via API gives inconsistent results
Open	None	T54542 User preferences are inconsistently stored (bool/int as default, string for overrides)
Resolved	Tgr	T148492 Fix for MultimediaViewer $wgMediaViewerEnableByDefault

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.

@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".

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

@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.

@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 claimed this task.Oct 18 2016, 4:27 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.

$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

gerritbot added a project: Patch-For-Review.Oct 18 2016, 9:24 PM

@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

Arseny1992 reopened this task as Open.Oct 18 2016, 9:32 PM

Arseny1992 added a parent task: T71942: Updating user preferences via API gives inconsistent results.

Arseny1992 added a parent task: T54542: User preferences are inconsistently stored (bool/int as default, string for overrides).Oct 18 2016, 9:35 PM

• bmansurov added a project: Web-Team-Backlog-Archived.Oct 19 2016, 6:16 PM

• jhobs moved this task from Incoming to 2014-15 Q4 on the Web-Team-Backlog-Archived board.Oct 20 2016, 5:13 PM

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

(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

MBinder_WMF removed a project: Web-Team-Backlog-Archived.Apr 13 2017, 6:39 PM

dr0ptp4kt triaged this task as Medium priority.Apr 18 2017, 3:27 PM

dr0ptp4kt moved this task from Backlog to Next on the MediaViewer board.

MBinder_WMF added a project: Multimedia.Aug 7 2017, 5:38 PM

MarkTraceur added a project: Multimedia-Team-Working-Board.Sep 25 2017, 4:53 PM

MarkTraceur moved this task from To Do to Code Review on the Multimedia-Team-Working-Board board.

MarkTraceur moved this task from Code Review to To Do on the Multimedia-Team-Working-Board board.Sep 25 2017, 7:44 PM

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

ReleaseTaggerBot added a project: MW-1.31-release-notes (WMF-deploy-2017-09-26 (1.31.0-wmf.1)).Sep 25 2017, 9:00 PM

in progress.

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

looks done to me!

@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).

Fix for MultimediaViewer $wgMediaViewerEnableByDefaultClosed, ResolvedPublicActions

Description

Details

Related ObjectsSearch...

Event Timeline

Fix for MultimediaViewer $wgMediaViewerEnableByDefault
Closed, ResolvedPublic
Actions

Related Objects
Search...