Non security little things

If installed, the extension enhances Special:Search by an advanced parameters form and improves the way how to configure namespaces.

and also on https://www.mediawiki.org/wiki/Extension:AdvancedSearch

I'm struggling to grok that as a whole sentence. Configure namespaces? Configure them for what? (I'm presuming it doesn't mean configure namespaces akin to adding them etc in LocalSettings)

• onSpecialSearchPowerBox hook subscriber is a noop, should be removed
• Indenting in getBetaFeaturePreferences for info-link and discussion-link values, should either be on the same line, or should be an extra tab before =>
• No COPYING/LICENCE file (but licence is in extension.json)
• Vagrant role (should be good first task ) would be useful
• Screenshot on https://www.mediawiki.org/wiki/Extension:AdvancedSearch is out of date
• advancedsearch-field-or should be "these" not "those"
• Has this extension had i18n review?
• I noticed on Special:Search?uselang=qqx that a few messages didn't change to message keys. Having then found the javascript below, it's clear that these messages can't be localised. This need fixing before deployment on WMF wikis

	var i18n = {
		de: {
			'advanced-search': 'Erweiterte Suchoptionen:',

			// TODO Move these to i18n
			text: 'Seite enthält …',
			structure: 'Struktur',
			files: 'Dateien und Bilder'
		},
		en: {
			'advanced-search': 'Advanced parameters:',
			text: 'The page should include …',
			structure: 'Structure',
			// FIXME: Why does this need to mention both, like images are not files?
			files: 'Files and images'
		}
	};

• 'The page should include …' - Should this be an ellipses? I don't think we use this syntax anywhere else
• RL loader code that looks wrong module is defined for each file. Could be moved to ResourceFileModulePaths to save some duplication/words

Non security little things

i18n: Also note that splitting strings like ("The page should include..." "this word") currently does not tell translators that the second string requires accusative declension when trying to translate the second string properly...

In T168860#3428486, @Aklapper wrote:

Non security little things

i18n: Also note that splitting strings like ("The page should include..." "this word") currently does not tell translators that the second string requires accusative declension when trying to translate the second string properly...

And should generally be avoided for sure!

https://www.mediawiki.org/wiki/Localisation#Avoid_fragmented_or_.27patchwork.27_messages

I guess also... "The page should include... Not this word" isn't very good english.

"The page should not include this word" obviously makes a lot more sense

"The page should include … Exactly this text:" doesn't really make sense. Include exactly this text? What do the other options do?

I wonder if these should also have tooltips with much more detailed information about what the options do... Or links

@Reedy @Aklapper Thanks for your review so far! We'll follow up on it soon.

Change 365261 had a related patch set uploaded (by Gabriel Birke; owner: Gabriel Birke):
[mediawiki/extensions/AdvancedSearch@master] Address some non-security issues from T168860

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

Change 365261 merged by jenkins-bot:
[mediawiki/extensions/AdvancedSearch@master] Address some non-security issues from T168860

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

In T168860#3428032, @Reedy wrote:

• onSpecialSearchPowerBox hook subscriber is a noop, should be removed
• Vagrant role (should be good first task ) would be useful
• Screenshot on https://www.mediawiki.org/wiki/Extension:AdvancedSearch is out of date

I think those are the only outstanding ones for the non security stuff

Vagrant roles added at https://gerrit.wikimedia.org/r/#/c/365964/

PHP code is obviously fine from a security PoV

	// TODO initialize with API call instead
	var templateModel = new mw.libs.advancedSearch.dm.MenuDataModel( {
		infoboxNature: 'Infobox Nature',
		infoboxArchitecture: 'Infobox Architecture',
		commonsLink: 'Link to Commons'
	} );

Specifically the last one there... Should that be i18n-ified? Not that any of them actually seem to be used anywhere?

	mw.loader.load( '//de.wikipedia.org/w/index.php?title=MediaWiki:Gadget-DeepCat.js&action=raw&ctype=text/javascript' );

Uh... what? Why? This doesn't feel very non WMF friendly among other things. Never mind any comments/similar explaining why it's there.

On Timeless skin... Things look rather broken in terms of layout of the subheadings

On Vector/Monobook... There seems to be some control overlap below the "Search in:" header. Doesn't look the same as the "Advanced parameters:" header in collapsed/expanded states

@Reedy Thank you for pointing out two vestiges of the early demo prototype!

• The templateModel variable is just a placeholder for an unfinished feature. This will be implemented in T165302 and the variable will be removed. No need to i18n the placeholder.
• The loading of the DeepCat gadget will also be removed. Instead, DeepCat-like functionality will be implemented for all wikis that are indexed in CatGraph. See T165297 and T170533 .

Could someone give an update on the status of the security review? I hope nobody is waiting for anything from our side.

@Bawolff @Reedy do you have any updates on this? We're aiming for a deployment as a beta feature in October or November 2017, so we should resolve any security review related issues rather soonish. Can you give a brief timeline for this?