Page MenuHomePhabricator

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


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.

Event Timeline

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

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.

Ya, I don't know / remember why we even went with interfaces here.

Change 765321 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/services/parsoid@master] Change Config/* classes to be abstract classes instead of interfaces

Change 765321 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Change Config/* classes to be abstract classes instead of interfaces

ssastry claimed this task.

Change 766858 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/vendor@master] Bump parsoid to 0.15.0-a22

Change 766858 merged by jenkins-bot:

[mediawiki/vendor@master] Bump parsoid to 0.15.0-a22