feat: ParsoidModule implementation#166
feat: ParsoidModule implementation#166originalauthority wants to merge 57 commits intoUniversal-Omega:mainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest commit c036061 actually fixes the Parsing issue, such that wikitext is parsed properly to HTML when it is passed to the template for display. That fixes the most major bugs, and we essentially have PortableInfobox for Parsoid working. One issue that did crop up, however, is that Parsoid automatically inserts h2 or h3 and wraps them.
This corresponds to the section the infobox exists on the page (I assume this is to signal to VisualEditor which section on the page the infobox is at, so when a section is edited, it knows to pull the infobox in. This issue did not crop up during development as I - this seems like an idiot move - was using the VisualEditor primarily to see the result from Parsoid. Obviously, the VisualEditor API does not return these section tags, but they are returned when using the ParsoidMigration extension. I can't figure out a way to disable this behaviour, so I'm going to ask on Phabricator if anyone has any ideas. |
…aWikiParser::class and parsing it early causes Parsoid to just return the infobox src.
… base class to reduce duplication
feat: deal with complex arguments
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #166 +/- ##
============================================
- Coverage 60.78% 52.28% -8.51%
- Complexity 503 594 +91
============================================
Files 36 43 +7
Lines 1525 1775 +250
============================================
+ Hits 927 928 +1
- Misses 598 847 +249 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Delete unused TagController
KockaAdmiralac
left a comment
KockaAdmiralac
left a comment
There was a problem hiding this comment.
Thanks for this patch! We've been using it on Undertale and Deltarune wikis since September, and we've recently switched to Parsoid for read views, so I wanted to mention what changes we needed to make.
| // this differs from earlier as we need the frame to be able to grab the | ||
| // params the user passed - parsoid handles this internally it appears | ||
| 'processInNewFrame' => false, |
There was a problem hiding this comment.
This comment seems to be no longer true? I'm not seeing any difference between true and false for this value.
We actually had an issue where DSRs were off unless this value was true, but that doesn't seem to be the case anymore. Parsoid update maybe?
| public function replaceVariables( $wikitext ) { | ||
| // no-op - I think handled by ->wikiTextToDOM? | ||
| } |
There was a problem hiding this comment.
This should return $wikitext;, because otherwise <image><default>{{{parameter}}}</default></image> doesn't work. Or doesn't work with galleries, at least.
| $child = $node->firstChild; | ||
|
|
||
| // insipiration taken from Ext:Cite | ||
| // and also <gallery> | ||
| while ( $child !== null ) { | ||
| $nextChild = $child->nextSibling; |
There was a problem hiding this comment.
This only processes infoboxes on the top-level, and not infoboxes nested elsewhere within the DOM (within divs, in bullet point lists, etc.) See utdrwiki@f37e7f3 for how we fixed it.
| $variableTheme = $this->paramMap[ $attr['theme-source'] ]; | ||
| if ( $variableTheme ) { |
There was a problem hiding this comment.
This emits a warning when theme-source is used but the parameter isn't passed.
| $variableTheme = $this->paramMap[ $attr['theme-source'] ]; | |
| if ( $variableTheme ) { | |
| $variableTheme = $this->paramMap[ $attr['theme-source'] ] ?? ''; | |
| if ( !empty( $variableTheme ) ) { |
| $dataMw = DOMDataUtils::getDataMw( $child ); | ||
| $parsoidData = DOMDataUtils::getDataParsoid( $child )->src; | ||
|
|
||
| $parts = $dataMw->parts; |
There was a problem hiding this comment.
$dataMw might not have parts, so this emits a warning.
| $child->removeChild( $child->firstChild ); | ||
| } | ||
|
|
||
| foreach ( $parts as $part ) { |
There was a problem hiding this comment.
This only renders infoboxes which have parameters passed. Infoboxes with no parameters passed should also be rendered (utdrwiki@c99d568).
3 participants
This PR is a draft implementation of a ParsoidModule for PortableInfobox; it displays PortableInfoboxes for reads on Parsoid (and also has the benefit of displaying them in the VisualEditor, and they are editable.) It supersedes #165 with a more robust implementation. It closes #132
It's a bit of a draft at present, and could probably be refined. A few of the classes (such as the
PortableInfoboxRenderServicehave been copied and adjusted, and are namespaced underPortableInfobox\Parsoid.It's still a work in process, and hasn't been tested against complex infoboxes. But it's a MVP that works at present. There are some issues:
The wiki text is for some reason not currently parsed, and just displays as wikitext, rather than the resultant HTML.Issue No. 2 is the most problematic. It feels a bit like a catch-22 situation. Doing everything in the
->extTagToDOMis too early, as we don't know the parameters the user passed to the template on the page, so we rely on getting it later in thewtPostProcess, however this seems to return just plain wiki text and there doesn't appear to be any way to re-feed this to Parsoid to get the parsed value back.I'm PRing what I have for now, and will continue to work on it - but if there are any ideas how we can get this Parsed, I'm all ears. May need to ask on WMF Phabricator if we can't figure it out.
Changes to wider PortableInfoboxes
NodeFactory::newFromSimpleXMLandNodeFactory::newFromXMLnow accept an$externalParser. This should either be theMediaWikiParseror theParsoidMediaWikiParserthis is needed so we can differentiate between whether we are in aParser.phpcontext, orParsoidcontext. Mostly for the<image>and<media>tags, becauseNodeImagecurrently calls out to the legacy parser functions which we don't want. It is null by default, so that no change needs to be made for the legacy implementation.MediaWikiParserService::addImageto add the image to the HTML output for selection byPageImages. InternallyPageImagesrelies on several Parser.php hooks which are not guaranteed to be supported. Further investigation is needed to whether this can be supported when/ifPageImagesis made Parsoid-compat.