Bartosz Dziewoński has uploaded a new change for review. https://gerrit.wikimedia.org/r/310074
Change subject: Minor code conventions and code quality changes all over ...................................................................... Minor code conventions and code quality changes all over I read through most of the code today in a big code review. This changes things that were easier to fix than to file bugs about, or things I wouldn't bother complaining about because they're trivial. ApiFileAnnotations.php: * Use htmlspecialchars() everywhere, except where we explicitly need to allow HTML syntax. * More robustly check for the single-link-in-single-paragraph case. Example inputs that would break old logic: "----" (PHP warning), "" (PHP warning), "http://example.com/ Some text" ("Some text" ignored). extension.json: * Change hook functions to just be named "on<hookname>". * Remove unused "ExtensionFunctions" and "callback", they do nothing. * Rename ResourceLoader module to "ext.fileannotations". FileAnnotations.hooks.php: * Match changes in extension.json. * Avoid implicit $wgTitle usage from "new Message()" by using the msg() function from context. FileAnnotationsContent: * Whitespace changes per code conventions. * Avoid implicit $wgTitle usage from "new Message()" by specifying the title. * Show informational message in user language with correct parser cache handling. fileannotations.less: * Minor code conventions changes. fileannotations.js: * Avoid magic namespace numbers. * Pass titles to the API in canonical text form, to avoid 'normalized' warnings in output. Change-Id: Ib0d1060a3d4bd6b295322deec6d8fa261b65fa7b --- M ApiFileAnnotations.php M FileAnnotations.hooks.php M extension.json M includes/FileAnnotationsContent.php M resources/src/fileannotations.js M resources/src/fileannotations.less 6 files changed, 51 insertions(+), 55 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/FileAnnotations refs/changes/74/310074/1 diff --git a/ApiFileAnnotations.php b/ApiFileAnnotations.php index 73dafbd..fdcbba4 100644 --- a/ApiFileAnnotations.php +++ b/ApiFileAnnotations.php @@ -131,8 +131,8 @@ $src = $info['thumburl']; $imagesHtml .= - '<a class="category-member" href="' . $href . '">' . - '<img src="' . $src . '" />' . + '<a class="category-member" href="' . htmlspecialchars( $href ) . '">' . + '<img src="' . htmlspecialchars( $src ) . '" />' . '</a>'; } @@ -140,7 +140,7 @@ // @FIXME: i18n! $seeMoreHtml = $pages - ? '<a href="' . $href . '">' . 'See more images' . '</a>' + ? '<a href="' . htmlspecialchars( $href ) . '">' . 'See more images' . '</a>' : ''; $html = @@ -204,14 +204,15 @@ // There's only one page, so just do it here $html = '<div class="wikipedia-article-annotation">' . + // The API result here should be safe HTML $page['extract'] . '<p class="pageimage">' . '<img src="' . - $page['thumbnail']['source'] . + htmlspecialchars( $page['thumbnail']['source'] ) . '" width="' . - $page['thumbnail']['width'] . + htmlspecialchars( $page['thumbnail']['width'] ) . '" height="' . - $page['thumbnail']['height'] . + htmlspecialchars( $page['thumbnail']['height'] ) . '" />' . '</p>' . '</div>'; @@ -291,26 +292,26 @@ if ( isset( $labels[$currentLang] ) ) { $label = '<h2 class="wikidata-label">' . - $labels[$currentLang]['value'] . + htmlspecialchars( $labels[$currentLang]['value'] ) . '</h2>'; } elseif ( isset( $labels['en'] ) ) { // Blatantly strange fallback, but we don't want to have // no label...hopefully this works for 99% of things. $label = '<h2 class="wikidata-label">' . - $labels['en']['value'] . + htmlspecialchars( $labels['en']['value'] ) . '</h2>'; } if ( isset( $descriptions[$currentLang] ) ) { $description = '<p class="wikidata-description">' . - $descriptions[$currentLang]['value'] . + htmlspecialchars( $descriptions[$currentLang]['value'] ) . '</p>'; } elseif ( isset( $descriptions['en'] ) ) { $description = '<p class="wikidata-description">' . - $descriptions['en']['value'] . + htmlspecialchars( $descriptions['en']['value'] ) . '</p>'; } @@ -368,8 +369,8 @@ $info = $page['imageinfo'][0]; return '<div class="wikidata-image">' . - '<a class="commons-image" href="' . $info['descriptionurl'] . '">' . - '<img src="' . $info['thumburl'] . '" />' . + '<a class="commons-image" href="' . htmlspecialchars( $info['descriptionurl'] ) . '">' . + '<img src="' . htmlspecialchars( $info['thumburl'] ) . '" />' . '</a>' . '</div>'; } @@ -379,15 +380,18 @@ $parsed = $presult->mText; // Check to see if we can return a special display for this annotation. + // We can't just the $text against the regexes, since the link might be generated from a + // wikitext link like [[commons:Foo]] or a template. $dom = new DOMDocument(); - $domFragment = $dom->createDocumentFragment(); - $domFragment->appendXml( $presult->mText ); + $dom->loadXML( '<root>' . $presult->mText . '</root>' ); - // The first element will always be a paragraph. Get its first child. - $possibleLink = $domFragment->firstChild->firstChild; + $xpath = new DOMXPath( $dom ); + // If the output is just a single link `<a>` wrapped in a single paragraph `<p>`, optionally + // with some whitespace around it, do something special. + $matches = $xpath->query( '//root[count(*)=1]/p[count(*)=1][normalize-space(text())=""]/a' ); + $possibleLink = $matches->item( 0 ); - // Check if it's a link element. - if ( $possibleLink->nodeType === XML_ELEMENT_NODE && $possibleLink->nodeName === 'a' ) { + if ( $possibleLink ) { // Find out if the link is something we care about /** @noinspection PhpUndefinedFieldInspection */ $href = $possibleLink->attributes->getNamedItem( 'href' )->value; diff --git a/FileAnnotations.hooks.php b/FileAnnotations.hooks.php index 1717f72..b5aa666 100644 --- a/FileAnnotations.hooks.php +++ b/FileAnnotations.hooks.php @@ -22,20 +22,13 @@ */ class FileAnnotationsHooks { - public static function onRegistration() { - return true; - } - - public static function onSetup() { - return true; - } - - public static function getModulesForFilePage( &$out, &$skin ) { + public static function onBeforePageDisplay( &$out, &$skin ) { // Dump it on every page. - $out->addModules( [ 'fileannotations' ] ); + $out->addModules( [ 'ext.fileannotations' ] ); } - public static function addFileAnnotationsTab( SkinTemplate &$sktemplate, array &$links ) { + public static function onSkinTemplateNavigation( SkinTemplate &$sktemplate, array &$links ) { + // Add the "File annotations" tab on file pages $title = $sktemplate->getTitle(); if ( $title->inNamespace( NS_FILE ) ) { $fatitle = Title::makeTitle( @@ -43,7 +36,7 @@ $title->getDBkey() ); - $tabMessage = new Message( + $tabMessage = $sktemplate->msg( 'fileannotations-tab' ); diff --git a/extension.json b/extension.json index b673417..7701046 100644 --- a/extension.json +++ b/extension.json @@ -8,7 +8,6 @@ "descriptionmsg": "fileannotations-desc", "license-name": "GPL-3.0", "type": "other", - "callback": "FileAnnotationsHooks::onRegistration", "namespaces": [ { "name": "File_Annotations", @@ -36,26 +35,23 @@ ] }, "Hooks": { - "BeforePageDisplay": "FileAnnotationsHooks::getModulesForFilePage", - "SkinTemplateNavigation": "FileAnnotationsHooks::addFileAnnotationsTab" + "BeforePageDisplay": "FileAnnotationsHooks::onBeforePageDisplay", + "SkinTemplateNavigation": "FileAnnotationsHooks::onSkinTemplateNavigation" }, "ResourceModules": { - "fileannotations": { + "ext.fileannotations": { "scripts": [ "resources/src/fileannotations.js" ], - "styles": [ "resources/src/fileannotations.less" ], - "dependencies": [ "mediawiki.Title", "jquery.ui.draggable", "jquery.ui.resizable", "oojs-ui" ], - "messages": [ "fileannotations-create", "fileannotations-save", @@ -69,9 +65,6 @@ "localBasePath": "", "remoteExtPath": "FileAnnotations" }, - "ExtensionFunctions": [ - "FileAnnotationsHooks::onSetup" - ], "AutoloadClasses": { "ApiFileAnnotations": "ApiFileAnnotations.php", "FileAnnotationsHooks": "FileAnnotations.hooks.php", diff --git a/includes/FileAnnotationsContent.php b/includes/FileAnnotationsContent.php index 85fccc3..0d299ca 100644 --- a/includes/FileAnnotationsContent.php +++ b/includes/FileAnnotationsContent.php @@ -32,12 +32,12 @@ throw new JsonSchemaException( wfMessage( 'eventlogging-invalid-json' )->parse() ); } - $schema = include ( __DIR__ . '/FileAnnotationsSchema.php' ); + $schema = include __DIR__ . '/FileAnnotationsSchema.php'; - $arrayAnnotations = (array) $annotations; + $arrayAnnotations = (array)$annotations; foreach ( $arrayAnnotations['annotations'] as $i => $annotation ) { - $arrayAnnotations['annotations'][$i] = (array) $annotation; + $arrayAnnotations['annotations'][$i] = (array)$annotation; } return EventLogging::schemaValidate( $arrayAnnotations, $schema ); @@ -66,15 +66,17 @@ $title->getDBkey() ); - $fileMsg = new Message( + $fileMsg = wfMessage( 'fileannotations-go-to-filepage', - [ $fileTitle->getPrefixedDBkey() ] - ); + $fileTitle->getPrefixedDBkey() + ) + // Set a title to avoid implicit $wgTitle usage + ->title( $title ) + // Show in user language. Note that this automatically splits the parser cache per-language. + ->inLanguage( $options->getUserLangObj() ); $output->setText( - '<p>' . - $fileMsg->parse() . - '</p>' . + $fileMsg->parseAsBlock() . $output->getRawText() ); } diff --git a/resources/src/fileannotations.js b/resources/src/fileannotations.js index 10f7321..c7aa0c9 100644 --- a/resources/src/fileannotations.js +++ b/resources/src/fileannotations.js @@ -1,7 +1,7 @@ ( function ( $, mw, OO ) { var pageAnnotator, pageTitle = mw.Title.newFromText( mw.config.get( 'wgPageName' ) ), - isFilePage = pageTitle.getNamespaceId() === 6, + isFilePage = pageTitle.getNamespaceId() === mw.config.get( 'wgNamespaceIds' ).file, $fileLink = $( '#file a' ); /** @@ -44,7 +44,7 @@ $( 'body' ).append( this.$container ); - this.annotationsTitle = mw.Title.newFromText( 'File_Annotations:' + this.fileTitle.getMain() ); + this.annotationsTitle = mw.Title.newFromText( 'File Annotations:' + this.fileTitle.getMain() ); this.getAndRenderAnnotations().then( function () { var $body = $( 'body' ); @@ -147,7 +147,7 @@ prop: 'revisions', rvprop: 'content', indexpageids: true, - titles: this.annotationsTitle.getPrefixedDb() + titles: this.annotationsTitle.getPrefixedText() } ).then( function ( data ) { var rv, text, annotations, pages = data.query.pages, @@ -180,7 +180,7 @@ FileAnnotator.prototype.saveAnnotations = function ( annotations, summary ) { return this.api.postWithToken( 'csrf', { action: 'edit', - title: this.annotationsTitle.getPrefixedDb(), + title: this.annotationsTitle.getPrefixedText(), text: JSON.stringify( annotations ), summary: summary } ); @@ -198,7 +198,7 @@ action: 'query', indexpageids: true, prop: [ 'fileannotations', 'imageinfo' ], - titles: this.fileTitle.getPrefixedDb(), + titles: this.fileTitle.getPrefixedText(), faparse: true, iiprop: 'size' } ).then( function ( data ) { diff --git a/resources/src/fileannotations.less b/resources/src/fileannotations.less index 1c69165..1eefa36 100644 --- a/resources/src/fileannotations.less +++ b/resources/src/fileannotations.less @@ -29,11 +29,13 @@ .category-members { width: 550px; + .category-member { margin: 5px; width: 100px; height: 100px; text-align: center; + img { display: inline-block; } @@ -47,7 +49,7 @@ width: 510px; padding: 5px; - & > p { + > p { width: 250px; display: inline-block; } @@ -78,8 +80,10 @@ &.editing-annotation { border: 1px dotted green; + .annotation-container { display: block; + .file-annotation, .annotation-edit-buttons { display: none; -- To view, visit https://gerrit.wikimedia.org/r/310074 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib0d1060a3d4bd6b295322deec6d8fa261b65fa7b Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/FileAnnotations Gerrit-Branch: master Gerrit-Owner: Bartosz Dziewoński <matma....@gmail.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
