Page MenuHomePhabricator

Migrate FlaggedRevs "jsReviewNeedsChange" inline script to use OutputPage::addJsConfigVars
Open, LowPublic

Description

Setting one variable in an one script tag into the global scope is not best practice. Please move this var to mw.config.

$:foo\> cd ../mediawiki-extensions/FlaggedRevs/
$:foo\> grep -r jsReviewNeedsChange .
./frontend/modules/ext.flaggedRevs.review.js:		/*global jsReviewNeedsChange*/
./frontend/modules/ext.flaggedRevs.review.js:		if ( typeof jsReviewNeedsChange !== 'undefined' && jsReviewNeedsChange === 1 ) {
./frontend/RevisionReviewFormUI.php:		$s .= '<script type="text/javascript">var jsReviewNeedsChange = ' .

Details

Reference
bz34716

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 12:15 AM
bzimport set Reference to bz34716.

There are also two places, where two function (onFRChangeExpiryDropdown/onFRChangeExpiryField) are added with a own script tag. Maybe create a new module or move it to the existing modules.

(In reply to comment #1)

Patch welcome :-D

MakeGlobalVariablesScript hook?

Aklapper renamed this task from Use ResourceLoader/mw.config for jsReviewNeedsChange to Use ResourceLoader/mw.config for jsReviewNeedsChange variable.Dec 22 2016, 4:50 PM
Aklapper updated the task description. (Show Details)
Krinkle renamed this task from Use ResourceLoader/mw.config for jsReviewNeedsChange variable to Migrate FlaggedRevs "jsReviewNeedsChange" inline script to use OutputPage::addJsConfigVars.Apr 1 2021, 12:57 AM

Instead of moving to mw.config, I’d set an attribute (say data-jsdisabled) on the affected button (#mw-fr-submit-reject). This requires less refactoring and doesn’t pollute mw.config with a piece of information unlikely to be useful for script/gadget authors, or any code outside of this single FlaggedRevs module. It also keeps logically connected pieces of information together.