Differential D25847
Remarkup: harden how we recognize internal/external URIs (mailto, mobile phones, ...) Authored by valerio.bozzolan on Dec 5 2024, 15:52. Tags Referenced Files
Subscribers
Details Some unit tests were missing the new CSS class 'remarkup-link-ext'. Added. The CSS class was introduced here: https://we.phorge.it/rP349f006904fabf1d4df31ff4840502af3ab379a7 Also, Remarkup is now hardened a bit, to do not consider anymore these as internal links:
Also, Remarkup engine option 'uri.base' is now taken in consideration, to know if an URI is internal or not. Run these unit tests, and they finally work again: arc unit ./src/infrastructure/parser/__tests__/PhutilPygmentizeParserTestCase.php arc unit ./src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php Additionally, run this, that has new tests added: arc unit ./src/infrastructure/parser/__tests__/PhutilURIHelperTestCase.php Also run this, check that the class tree is already correct: arc liberate Have fun with the green lights.
Diff Detail
Event TimelineComment Actions I applied this patch locally on top of git master and output does not complain anymore about 'link-brackets.txt' (thus it's correct) but fails in link-edge-cases.txt now (thus it's likely not complete): FAIL PhutilRemarkupEngineTestCase::testEngine Assertion failed, expected values to be equal (at PhutilRemarkupEngineTestCase.php:66): Failed to markup HTML in file 'link-edge-cases.txt'. Expected vs Actual Output Diff --- Old Value +++ New Value @@ -1,11 +1,11 @@ -'<p><a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a></p> +'<p><a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a></p> -<p>(<a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p> +<p>(<a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p> -<p><a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a></p> +<p><a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a></p> -<p><a href="http://www.example.com/wiki/example_(disambiguation)" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/wiki/example_(disambiguation)</a></p> +<p><a href="http://www.example.com/wiki/example_(disambiguation)" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/wiki/example_(disambiguation)</a></p> -<p>(example <a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p> +<p>(example <a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p> -<p>Quick! <a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a>!</p>' +<p>Quick! <a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a>!</p>' Comment Actions
You are right. Marking as revision not ready. But this is becoming a rabbit hole. Seems the Env/Configs are not configured correctly in unit tests, and we really need them: arc unit src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php
FAIL PhutilRemarkupEngineTestCase::testEngine
EXCEPTION (Exception): Trying to read configuration "phabricator.production-uri" before configuration has been initialized.
#0 /home/boz/repos/phorge/src/infrastructure/env/PhabricatorEnv.php(423): PhabricatorEnv::getEnvConfig('...')
#1 /home/boz/repos/phorge/src/infrastructure/env/PhabricatorEnv.php(447): PhabricatorEnv::getProductionURI('...')
#2 /home/boz/repos/phorge/src/infrastructure/env/PhabricatorEnv.php(441): PhabricatorEnv::getSelfURIMap()
#3 /home/boz/repos/phorge/src/infrastructure/parser/PhutilURIHelper.php(43): PhabricatorEnv::isSelfURI('...')
#4 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(80): PhutilURIHelper->isSelf()
#5 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(186): PhutilRemarkupDocumentLinkRule->renderHyperlink('...', '...')
#6 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(133): PhutilRemarkupDocumentLinkRule->markupDocumentLink(Array)
#7 [internal function]: PhutilRemarkupDocumentLinkRule->markupAlternateLink(Array)
#8 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(17): preg_replace_callback('...', Array, '...')
#9 /home/boz/repos/phorge/src/infrastructure/markup/blockrule/PhutilRemarkupBlockRule.php(92): PhutilRemarkupDocumentLinkRule->apply('...')
#10 /home/boz/repos/phorge/src/infrastructure/markup/blockrule/PhutilRemarkupDefaultBlockRule.php(17): PhutilRemarkupBlockRule->applyRules('...')
#11 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/PhutilRemarkupEngine.php(283): PhutilRemarkupDefaultBlockRule->markupText('...', NULL)
#12 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/PhutilRemarkupEngine.php(140): PhutilRemarkupEngine->markupBlock(Array)
#13 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/PhutilRemarkupEngine.php(106): PhutilRemarkupEngine->preprocessText('...')
#14 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php(56): PhutilRemarkupEngine->markupText('...')
#15 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php(13): PhutilRemarkupEngineTestCase->markupText('...')
#16 /home/boz/repos/arcanist/src/unit/engine/phutil/PhutilTestCase.php(639): PhutilRemarkupEngineTestCase->testEngine()
#17 /home/boz/repos/arcanist/src/unit/engine/PhutilUnitTestEngine.php(69): PhutilTestCase->run()
#18 /home/boz/repos/arcanist/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php(148): PhutilUnitTestEngine->run()
#19 /home/boz/repos/arcanist/src/workflow/ArcanistUnitWorkflow.php(170): ArcanistConfigurationDrivenUnitTestEngine->run()
#20 /home/boz/repos/arcanist/scripts/arcanist.php(427): ArcanistUnitWorkflow->run()
#21 {main}I mark as changes planned, but I really don't know what to do. Let's talk in new task T15973: Fix unit test PhutilRemarkupEngineTestCase
Comment Actions transaction.search: handle unmanaged exception when objectType is not supported Closes T16054 Test Plan: Visit the page /api/transaction.search, fill the objectType with "XACT" and search: You do not see anymore this confusing exception: get_class() expects parameter 1 to be object, null given Instead, you see this exception message: In call to "transaction.search", specified "objectType" ("XACT") exists but is not supported. Try with a supported value like "TASK".
Comment Actions add some inline documentation about $array[$v] = 1 also git rebase origin/master just because we can Thanks andre
Comment Actions add a potential performance TODO in legacy code: T16061: PhabricatorEnv isSelfURI(): add small cache of these self-URIs Comment Actions Resigning as this developed into a larger patch requiring testing; shouldn't be listed as "accepted" as that referred to a revision long ago | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||