Repository navigation
Fix TypeError in the carousel gallery mode - #133
Merged
Merged
Conversation
Fixes #132 Every `<gallery mode="carousel">` failed with a TypeError, so the page did not render. `CarouselGallery::toHTML()` passed on the carousel component's parser function result, `[ $html, 'isHTML' => true, 'noparse' => true ]`, but MediaWiki's `<gallery>` tag hook must return a string, as declared from MediaWiki 1.43 to master. `toHTML()` now returns the HTML only, the same unwrapping `Collapse` already applies to its button's result. A tag hook's output is HTML the parser does not parse again, which is what the two flags ask for in a parser function. `CarouselGalleryTest` asserted the array and now expects the HTML. `{{#bootstrap_carousel:}}` is unchanged. Not in this change: core keeps only the `<gallery>` attributes that are valid on `ul`, so `fade` has no effect in the gallery mode even after this fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_ad639e9d-2eae-4d01-bc25-12c720e8ba45
malberts
added a commit
that referenced
this pull request
Oct 29, 2026
Follows-up to #133 `<gallery mode="carousel">` rebuilds each image as `[[File:...]]` wikitext from what core passes to `ImageGalleryBase::add()`. It wrote out every handler option, including the `targetlang` core adds to every gallery image. That is not image syntax, so the image parser read `targetlang=en` as the caption: it became every slide's tooltip, the alt text of a captionless slide, and a visible caption with `thumb`. The carousel also took the link from `add()`'s `$link` argument, which core always leaves empty. Core passes a per-image `link=` in the image options instead, as `link-url`, `link-title` or `no-link`, so every slide linked to its file page. The carousel now drops `targetlang` and writes `link=` from those image options, an empty `link=` for `no-link`. On MediaWiki 1.47 and later, where core also moves a per-image `class=` into the image options, that class is added to `img-fluid`. Other handler options, such as `page=` and `lang=`, are passed on as before. The tooltip core computes, `title` in the image options, is not image syntax either and stays out. `{{#bootstrap_carousel:}}` is unchanged. `CarouselGalleryTest` fed the gallery a link in `$link`, no `targetlang` and no image options, a shape core never produces, so it passed with both bugs. Its data sets now use core's shape, and the new `CarouselGalleryRenderingTest` parses `<gallery mode="carousel">` with the wiki's own parser and checks a slide's tooltip and link. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
malberts
added a commit
that referenced
this pull request
Oct 29, 2026
Follows-up to #133 `<gallery mode="carousel">` rebuilds each image as `[[File:...]]` wikitext from what core passes to `ImageGalleryBase::add()`, and it mishandled two parts of that data. Core adds a `targetlang` handler option to every gallery image. It is not an image option, so the image parser read `targetlang=en` as the caption, which became every slide's tooltip and the alt text of a captionless slide. And core passes a per-image `link=` in the image options (`link-url`, `link-title` or `no-link`), while the carousel read `add()`'s `$link` argument, which core leaves empty, so every slide linked to its file page. The carousel now drops `targetlang`, which `makeImage` sets again when the slide is parsed, and writes `link=` from the image options. Other handler options, such as `page=` and `lang=`, pass through as before. On MediaWiki 1.47 and later, where `<gallery>` accepts a per-image `class=` and passes it in the image options, that class is added to `img-fluid`. `{{#bootstrap_carousel:}}` is unchanged. `CarouselGalleryTest` fed the gallery a link in `$link`, no `targetlang` and no image options, a shape core never produces, which is how both bugs passed CI. Its data sets now use core's shape, and the new `CarouselGalleryRenderingTest` parses `<gallery mode="carousel">` with the wiki's own parser. Not in this change: a caption containing a link loses the link text in the tooltip, and a caption that is an image option keyword, such as `thumb`, is still read as that option. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
malberts
added a commit
that referenced
this pull request
Oct 29, 2026
Follows-up to #133 `<gallery mode="carousel">` rebuilds each image as `[[File:...]]` wikitext from what core passes to `ImageGalleryBase::add()`, and it mishandled two parts of that data. Core adds a `targetlang` handler option to every gallery image. It is not an image option, so the image parser read `targetlang=en` as the caption, which became every slide's tooltip and the alt text of a captionless slide. And core passes a per-image `link=` in the image options (`link-url`, `link-title` or `no-link`), while the carousel read `add()`'s `$link` argument, which core leaves empty, so every slide linked to its file page. The carousel now drops `targetlang`, which `makeImage` sets again when the slide is parsed, and writes `link=` from the image options. Other handler options, such as `page=` and `lang=`, pass through as before. On MediaWiki 1.47 and later, where `<gallery>` accepts a per-image `class=` and passes it in the image options, that class is added to `img-fluid`. `{{#bootstrap_carousel:}}` is unchanged. `CarouselGalleryTest` fed the gallery a link in `$link`, no `targetlang` and no image options, a shape core never produces, which is how both bugs passed CI. Its data sets now use core's shape, and the new `CarouselGalleryRenderingTest` parses `<gallery mode="carousel">` with the wiki's own parser. Not in this change: a caption containing a link loses the link text in the tooltip, and a caption that is an image option keyword, such as `thumb`, is still read as that option. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #132
Every
<gallery mode="carousel">fails with a TypeError, so the page does not render.CarouselGallery::toHTML()passed on the carousel component's parser function result,[ $html, 'isHTML' => true, 'noparse' => true ], but MediaWiki's<gallery>tag hook must return a string, as declared from MediaWiki 1.43 to master.toHTML()now returns the HTML only, the same unwrappingCollapsealready applies to its button's result. A tag hook's output is HTML the parser does not parse again, which is what the two flags ask for in a parser function.CarouselGalleryTestasserted the array and now expects the HTML.{{#bootstrap_carousel:}}is unchanged.Human input
Goal and shape: Claude's finding and fix, at @kghbln's request. @kghbln first asked whether
{{#bootstrap_carousel:}}breaks too (it does not) and whether an extension should hook into core's<gallery>tag at all; Claude answered that the gallery mode is the intended extension point and worth keeping.Outcome: not reviewed by a human yet. Left for you: the diff, and a
<gallery mode="carousel">on a test wiki, which no wiki has rendered with this change yet.Claude's calls
CarouselGallery; the release note goes under a new 6.1.2 heading.<gallery mode="carousel">through MediaWiki's parser. Without a local MediaWiki it could not be seen failing first, and the changed unit test already pins the string result.Carouselshould return HTML for the parser function and the gallery mode alike.Not in this PR
Core keeps only the
<gallery>attributes that are valid onul, sofadehas no effect in the gallery mode even after this fix. Not filed yet.Verification evidence
action=parseof<gallery mode="carousel">with captions, without captions, withfadeand withstylereturnedinternal_api_error_TypeError; a<gallery>without the mode and{{#bootstrap_carousel:}}parsed.CoreTagHooks::gallery(): string. REL1_43'sParser::renderImageGallery()returns whattoHTML()returns and passes the tag's attributes throughSanitizer::validateTagAttributes( $params, 'ul' ).CarouselGalleryTestagainst MediaWiki 1.43 through MediaWiki master while the test expects the array.php -lon both changed PHP files.