Parsoid Config/* interfaces should perhaps be abstract classes
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	cscott
	Aug 24 2020, 7:47 PM

Description

The code in Parsoid/extension/src was copied into VE in order to provide 'zero-configuration' VE for MW 1.35 LTS.

Parsoid later renamed a method in DataAccess. This broke the copy embedded in VE -- and what's more, broke it in a way we couldn't easily "migrate". Because the Parsoid/Config/* classes are *interfaces* not *abstract classes*, we couldn't ship (even temporarily) a stub DataAccess::fetchPageContent() method that redirected to DataAccess::fetchTemplateSource(), or vice-versa. Basically, it's not safe to add or rename any methods in an interface, or your implementers will immediately break.

Maybe this is a non-issue because the only ones implementing the Config interfaces is us. But it may also be worth considering whether we want to change them from interfaces to abstract classes before releasing these as stable, so that we do deprecation/renaming/new methods/etc in the future safely.

Details

	Subject	Repo	Branch	Lines +/-
	Bump parsoid to 0.15.0-a22	mediawiki/vendor	master	+512 -401
	Change Config/* classes to be abstract classes instead of interfaces	mediawiki/services/parsoid	master	+22 -22

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Open	None	T261329 Prepare Parsoid for MW
Resolved	MarkAHershberger	T302117 ZeroConf VE for MW 1.38
Open	None	T302118 Move MWParsoid\SiteConfig (and etc) to MediaWiki\Parser\Parsoid\SiteConfig (etc)
Resolved	ssastry	T261161 Parsoid Config/* interfaces should perhaps be abstract classes

Event Timeline

cscott created this task.Aug 24 2020, 7:47 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 24 2020, 7:47 PM

ssastry triaged this task as Low priority.Aug 25 2020, 4:55 PM

ssastry moved this task from Needs Triage to Tech Debt / Big changes on the Parsoid board.

cscott added a parent task: T261329: Prepare Parsoid for MW.Aug 26 2020, 5:02 PM

cscott updated the task description. (Show Details)Jan 23 2021, 9:57 PM

Since this was filed we seem to have fixed some of these?

SiteConfig and PageConfig are now abstract classes
DataAccess and PageContent is still an interface
ContentMetadataCollector is an interface (since ParserOutput needs to implement it, and it already has a superclass), but there's a ContentMetadataCollectorCompat trait that helps provide fallback implementations.

So it seems like we should either make DataAccess and PageContent abstract classes, for consistency, or else add DataAccessCompat and PageContentCompat traits in the same way ContentMetadataCollector has.