Problem/Motivation
On #3090659: Make a way for help topics to generate links only if they work and are accessible, we created two new functions for making links in help topics that degrade nicely when the link cannot be accessed or doesn't exist: help_route_link and help_topic_link.
On #3215784: [META] Fix up topics to use new help_route_link function and #3192585: Fix up topics to use new help_topic_link function, we fixed up the existing help topics to use these functions.
On this issue, we want to enforce the correct use of those functions in a test. The test should add code to core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest to make sure that:
- Both help_topic_link and help_route_link functions are being used for links and not the base Twig url() function.
- If help_route_link or help_topic_link is used, the text is translated.
- The syntax test includes "bad" examples to verify the tests are working, so there should also be "bad" examples added that violate those two test cases, to make sure the test is working properly.
Steps to reproduce
Proposed resolution
Add code to HelpTopicsSyntaxTest for these tests.
Remaining tasks
- Patch, review, commit.
- Add a change record to note that use of these functions is required by this test. See also the change record for #3090659: Make a way for help topics to generate links only if they work and are accessible: https://www.drupal.org/node/3192582
User interface changes
No, test only
API changes
Sort of -- we'll be requiring use of these functions.
Data model changes
No.
Release notes snippet
No.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | 3219923-18.patch | 2.25 KB | andypost |
| #17 | 3219923-17.patch | 1.38 KB | andypost |
| #17 | interdiff.txt | 1021 bytes | andypost |
| #9 | 3219923-9.patch | 2.25 KB | amber himes matz |
Comments
Comment #2
jhodgdonWe will not be able to complete this until #3086075: Use Twig to strip Twig syntax from help topics files in the syntax checker is done.
Comment #5
smustgrave commentedMoving to active as ticket in #2 is closed.
Comment #7
amber himes matzWorking on this. Looks like I can use
Drupal\Tests\Core\Utility\LinkGeneratorTestfor guidance.Comment #8
andypostGood idea 👍 looks that one is primary blocker
Comment #9
amber himes matzAccording to the IS, there are 3 objectives. The only thing missing from
HelpTopicsSyntaxTest(as far as I could tell) was a check for use ofurl(). I happened to have a month-old pull of 10.1.x which had 2 core help topics that still usedurl()(i.e.{% set search_url = render_var(url('search.view')) %}) and my update to this test caught them. (It doesn't catch all of them at once, just the first one it finds. When that one is fixed, then it would catch the next failure.)1. Both help_topic_link and help_route_link functions are being used for links and not the base Twig url() function.
- I added a string-match check in
HelpTopicsSyntaxTest::verifyTopic()to see ifurl(was found. I didn't know how to explicitly check for use of a certain function in this testing framework. So I hope that string-match type of check will suffice here.- I added a "bad topic" (core/modules/help_topics/tests/modules/help_topics_test/bad_help_topics/syntax/bad_help_topics.url_func_used.html.twig) that uses
url()- I added a case to
HelpTopicsSyntaxTest::verifyBadTopic()that verifies that the new bad topic fails as expected.- There is already a test topic (core/modules/help_topics/tests/modules/help_topics_test/help_topics/help_topics_test.test_urls.html.twig) that uses
help_route_link()andhelp_topic_link()in various ways and tests them inHelpTopicsSyntaxTest::verifyTopic()2. If help_route_link or help_topic_link is used, the text is translated.
- This appears to be covered already with
HelpTopicsSyntaxTest::verifyTopic()as it checks core/modules/help_topics/tests/modules/help_topics_test/help_topics/help_topics_test.test_urls.html.twig3. The syntax test includes "bad" examples to verify the tests are working, so there should also be "bad" examples added that violate those two test cases, to make sure the test is working properly.
- This appears to be covered already with
HelpTopicsSyntaxTest::verifyTopic()as it checks core/modules/help_topics/tests/modules/help_topics_test/help_topics/help_topics_test.test_urls.html.twigI'm also working on a unit test to cover
HelpTwigExtension::getRouteLink()andHelpTwigExtension::getTopicLink()but I'm struggling. Maybe it's something we can add later and isn't necessary for this issue, as it isn't in the original scope. (I can open a follow-up issue if it seems like a good idea to others.)Comment #11
amber himes matzRe-testing the patch as test failure looked like an infrastructure error and not an error from the patch.
Comment #12
andypostIt looks great and solves the issue but follow-up is required for twig functions,
\Drupal\Tests\Core\Template\TwigExtensionTest::testFileUrl()could be used as example, I'm fine with follow-upComment #13
andypostRe-reading #3090659-93: Make a way for help topics to generate links only if they work and are accessible I see that we have extensive functional testing coverage and the scope of the issue is to check all shipped topics has no usage of
url(Also I found no requirement to have unit test and moreover, this class will be moved to help module #3037230: Finalize the merge of Help Topics into Help
Comment #15
smustgrave commentedSeems random
Comment #17
andypostFixing nit, dot at the end of links we trying to prevent to get full url, leaving RTBC
Comment #18
andypostLast patch went without bad topic(
Comment #20
longwaveCommitted and pushed c42d65e4a7 to 11.x (10.2.x). Thanks!