Page MenuHomePhabricator

Fix token transformer return types
Closed, ResolvedPublic

Description

Token transformers return one of:

  • { async: false, token: .., }
  • { async: false, tokens: [..] }
  • { async: true }
  • { async: true, token: ... }
  • { async: true, tokens: [..] }
  • { async: true, tokens: [] }

And, then, we have verifyTokenIntegrity converting the return value to a normalized { tokens: .. } format. This is done in the token accumulators used in the Async TTM.

We should instead normalize all return values to canonical forms that minimizes sanity checks and inter-conversions between return type formats. This is good from both the code readability and performance standpoints.

Event Timeline

This old defensive code is from the 2012 era when Gabriel was still getting things working. There has been a lot of water that has flowed under the bridge since then and we should get rid of that legacy defensive code and get the token transformers to do the right thing.

+∞

cf https://gerrit.wikimedia.org/r/#/c/411293/11/lib/wt2html/TokenTransformManager.js@939 for example, which has more codepaths than it ought to because of this ambiguity in return types.

ssastry raised the priority of this task from Medium to High.Jun 14 2018, 8:12 PM

Let us pick this up in July as part of the Q1 work of the Platform Evolution CDP work.

Change 463557 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] Ensure LanguageConverter markup flattens token arrays in attributes

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

Change 464022 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] Always return tokens array in token transformers.

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

Change 464023 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] Ensure LinkHandler flattens its token arrays.

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

Change 464024 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] Ensure ParserFunctions flattens its token arrays.

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

Change 464025 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] Ensure that we always use {tokens:[...]} form in TTM.

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

Change 463557 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Ensure LanguageConverter markup flattens token arrays in attributes

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

Change 464022 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Always return tokens array in token transformers.

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

Change 464034 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] Clean up token handling in SyncTTM

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

Change 464035 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] No special token array handling in AsyncTTM

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

Change 464036 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] Don't emit test cases for extinct {token:...} form in TTM

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

Change 464037 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] DNM: Expensive workstack asserts

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

Change 464038 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] DNM: Expensive LinkHandler asserts

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

Change 464039 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] DNM: Expensive templatehandler asserts

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

Change 464023 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Ensure LinkHandler flattens its token arrays.

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

Change 464024 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Ensure ParserFunctions flattens its token arrays.

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

Change 464025 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Ensure that we always use {tokens:[...]} form in TTM.

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

Change 464034 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Clean up token handling in SyncTTM

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

Change 464035 merged by jenkins-bot:
[mediawiki/services/parsoid@master] No special token array handling in AsyncTTM

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

Change 464036 merged by jenkins-bot:
[mediawiki/services/parsoid@master] No need to check for extinct {token:...} form in TTM

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

cscott closed this task as Resolved.EditedOct 4 2018, 1:58 PM
cscott claimed this task.

I think I've fixed this! October's a little later than July but...

Now the only acceptable returns are:

{async:true}
{async:true|false,tokens:[....]}
{tokens:[....]}

and this is enforced by asserts in verifyTokensIntegrity in TokenTransformManager.

Change 464586 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] Tighten up (&document) the constraints on transformer returns

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

Change 464586 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Tighten up (and document) the constraints on transformer returns

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

Change 464626 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/services/parsoid@master] ParagraphWrapper: Transformers should always return a tokens array

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

Change 464039 abandoned by C. Scott Ananian:
DNM: Expensive templatehandler asserts

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

Change 464038 abandoned by C. Scott Ananian:
DNM: Expensive LinkHandler asserts

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

Change 464037 abandoned by C. Scott Ananian:
DNM: Expensive workstack asserts

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

Change 464626 merged by jenkins-bot:
[mediawiki/services/parsoid@master] ParagraphWrapper: Transformers should always return a tokens array

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