Page MenuHomePhabricator

Stop using the Env class outside of the Parsoid library (particularly in the extension REST handlers)
Closed, ResolvedPublic

Description

See the discussion starting at T221174#5439782

Details

Related Gerrit Patches:
mediawiki/vendor : masterBump Parsoid to 0.12.0-a8
mediawiki/services/parsoid : masterStop passing Env to DataAccess::logLinterData()
mediawiki/services/parsoid : masterMove downgrades to the entrypoint
mediawiki/services/parsoid : masterStop passing Env to "update" methods on ParsoidHandler
mediawiki/services/parsoid : masterAdd SiteConfig::languageConverterEnabledForLanguage
mediawiki/services/parsoid : masterStop passing Env to PageHandler::getPageContentResponse
mediawiki/services/parsoid : masterHoist acceptable check
mediawiki/services/parsoid : masterRemove titleShouldExist
mediawiki/services/parsoid : masterInline check for null revision content
mediawiki/services/parsoid : masterStop passing Env to ParsoidHandler::wt2html
mediawiki/services/parsoid : masterMove getPageMainContent from Env to PageConfig
mediawiki/services/parsoid : masterPass PageConfig to createEnv
mediawiki/services/parsoid : masterStop passing $env to createRedirectToOldidResponse
mediawiki/services/parsoid : masterMove substTopLevelTemplates to the entrypoint
mediawiki/services/parsoid : masterRemove Env use from content version resolution functionality
mediawiki/services/parsoid : masterEliminate use of $env in content version resolution code

Event Timeline

Arlolra created this task.Oct 11 2019, 11:00 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 11 2019, 11:00 PM
ssastry triaged this task as Medium priority.Oct 24 2019, 8:29 PM
ssastry added a subscriber: ssastry.Jan 2 2020, 5:49 AM

With some git grep, sed, sort, uniq, etc, here is how Env is being used:

Content version functionality:

$env->getInputContentVersion()
$env->getOutputContentVersion()
$env->setInputContentVersion( ... )
$env->setOutputContentVersion( ... )
$env->resolveContentVersion( ... )

Properties (some of which are available without going through env)

$env->pageWithOldid
$env->topFrame
$env->getPageConfig()
$env->getPageMainContent() 
$env->langConverterEnabled()

Document creation and logging. Logging is probably handled using the top level logger. As for document creation, need to investigate what that is being used for.

$env->createDocument( ... )
$env->log( ...)

So, overall, this refactoring might not be too hard.

Change 561441 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/services/parsoid@master] Remove $env use from content version resolution functionality

https://gerrit.wikimedia.org/r/561441

Change 561447 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/services/parsoid@master] Eliminate use of env in content version resolution code

https://gerrit.wikimedia.org/r/561447

With some git grep, sed, sort, uniq, etc, here is how Env is being used:
Content version functionality:

$env->getInputContentVersion()
$env->getOutputContentVersion()
$env->setInputContentVersion( ... )
$env->setOutputContentVersion( ... )
$env->resolveContentVersion( ... )

The above gerrit patches eliminate all but the first two, and those two are primarily initializations and can be eliminated easily as well.

...
So, overall, this refactoring might not be too hard.

Actually might require moving some functionality from the API endpoints into src/Parsoid.php interface so that the API endpoints are themselves lean and simply dispatch to the right Parsoid library API method.

Change 561447 abandoned by Subramanya Sastry:
Eliminate use of $env in content version resolution code

Reason:
squashed into parent patch

https://gerrit.wikimedia.org/r/561447

Arlolra claimed this task.Jan 6 2020, 4:41 PM

Change 561441 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Remove Env use from content version resolution functionality

https://gerrit.wikimedia.org/r/561441

Change 583401 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Move substTopLevelTemplates to the entrypoint

https://gerrit.wikimedia.org/r/583401

Change 583401 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Move substTopLevelTemplates to the entrypoint

https://gerrit.wikimedia.org/r/583401

Change 583423 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Stop passing $env to createRedirectToOldidResponse

https://gerrit.wikimedia.org/r/583423

Change 583424 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Pass PageConfig to createEnv

https://gerrit.wikimedia.org/r/583424

Change 583425 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Move getPageMainContent from Env to PageConfig

https://gerrit.wikimedia.org/r/583425

Change 583426 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Stop passing Env to ParsoidHandler::wt2html

https://gerrit.wikimedia.org/r/583426

Change 583427 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Inline check for null revision content

https://gerrit.wikimedia.org/r/583427

Change 583450 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Remove titleShouldExist

https://gerrit.wikimedia.org/r/583450

Change 583451 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Hoist acceptable check

https://gerrit.wikimedia.org/r/583451

Change 583477 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Stop passing Env to PageHandler::getPageContentResponse

https://gerrit.wikimedia.org/r/583477

Change 583478 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Add SiteConfig::languageConverterEnabledForPage

https://gerrit.wikimedia.org/r/583478

Change 583479 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Stop passing Env to "update" methods on ParsoidHandler

https://gerrit.wikimedia.org/r/583479

ssastry moved this task from To Do to Doing on the Parsing-critical-path board.Thu, Mar 26, 12:03 AM

Change 583787 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Move downgrades to the entrypoint

https://gerrit.wikimedia.org/r/583787

Noting so I don't forget. The use of Env in DataAccess::logLinterData is also suspect,
https://github.com/wikimedia/parsoid/blob/master/extension/src/Config/DataAccess.php#L331-L355

Noting so I don't forget. The use of Env in DataAccess::logLinterData is also suspect,
https://github.com/wikimedia/parsoid/blob/master/extension/src/Config/DataAccess.php#L331-L355

Yes, absolutely .. I knew it when that patch landed ... but that one should be relatively easy to fix, it appeared at that time.

Change 583423 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Stop passing $env to createRedirectToOldidResponse

https://gerrit.wikimedia.org/r/583423

Change 583424 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Pass PageConfig to createEnv

https://gerrit.wikimedia.org/r/583424

Change 583425 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Move getPageMainContent from Env to PageConfig

https://gerrit.wikimedia.org/r/583425

Change 583426 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Stop passing Env to ParsoidHandler::wt2html

https://gerrit.wikimedia.org/r/583426

Change 583427 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Inline check for null revision content

https://gerrit.wikimedia.org/r/583427

Change 583944 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Stop passing Env to DataAccess::logLinterData()

https://gerrit.wikimedia.org/r/583944

Change 583450 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Remove titleShouldExist

https://gerrit.wikimedia.org/r/583450

Change 583451 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Hoist acceptable check

https://gerrit.wikimedia.org/r/583451

Change 583477 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Stop passing Env to PageHandler::getPageContentResponse

https://gerrit.wikimedia.org/r/583477

Change 583478 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Add SiteConfig::languageConverterEnabledForLanguage

https://gerrit.wikimedia.org/r/583478

Change 583479 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Stop passing Env to "update" methods on ParsoidHandler

https://gerrit.wikimedia.org/r/583479

Change 583787 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Move downgrades to the entrypoint

https://gerrit.wikimedia.org/r/583787

Just the patch in T235307#6005051 and then this is done.

Change 583944 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Stop passing Env to DataAccess::logLinterData()

https://gerrit.wikimedia.org/r/583944

Arlolra closed this task as Resolved.Sat, Mar 28, 1:33 AM

Change 584120 had a related patch set uploaded (by Subramanya Sastry; owner: Subramanya Sastry):
[mediawiki/vendor@master] Bump Parsoid to 0.12.0-a8

https://gerrit.wikimedia.org/r/584120

Change 584120 merged by jenkins-bot:
[mediawiki/vendor@master] Bump Parsoid to 0.12.0-a8

https://gerrit.wikimedia.org/r/584120