Use Twig to strip Twig syntax from help topics files in the syntax checker

Sounds like a good idea. This is now part of the Stable section on #3027054: Help Topics module roadmap: the path to beta and stable.

As a note, one of the things the test does is to verify that all of the text in the Twig file is translated. So whatever we do in this issue to let Twig handle rendering, we still need to verify that.

I'm still thinking about this issue... I think it can be done as suggested in #4. Some details/links:

a. We are adding a Twig extension in #3090659: Make a way for help topics to generate links only if they work and are accessible, so you can see how to do that. This one should be added in the help_topics_test module.

b. This Twig extension should only implement getNodeVisitors(), and return a new NodeVisitor class that we will need to define. The core Twig extension does this. See
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Template%...
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Template%...

c. The node visitor can (like the Core one) extend the AbstractNodeVisitor class provided by Twig.

d. To remove something from rendering, you return NULL from the leaveNode() method (or doLeaveNode() if using the AbstractNodeVisitor class).

e. So I think on our NodeVisitor class we can check a setting somewhere that will let us turn on/off the deletion of certain node types from rendering. For instance I think in doLeaveNode() we can see if it's a SetNode
https://api.drupal.org/api/drupal/vendor%21twig%21twig%21src%21Node%21Se...
and if so, return NULL to delete it (if that setting is turned on). The other types of built-in nodes are listed on
https://api.drupal.org/api/drupal/namespace/Twig%21Node/9.0.x

f. Also I think we should postpone this for the moment, because on #3090257: Add additional tests for help topic syntax (which is nearly RTBC) we are adding more things to the syntax tests, and we should finish that first.

The "Add additional tests" issue got committed, so we can proceed on this now.

Now that I'm an "expert" (hah hah) on Twig extensions (having recently worked on #3090659: Make a way for help topics to generate links only if they work and are accessible), I think I'm ready to work on a patch for this issue. Which is the only unfinished issue in the Help Topics roadmap that doesn't yet have a patch at Needs Review, hooray!

Well... I made a patch. It doesn't work. The test needs to render the Twig template several times with several different node visitor configurations, but I can't get it to render again, despite clearing the render and Twig caches (I think?). It's simply not getting to my node visitor class after the first time the template gets rendered.

I've banged my head against this all day... it might work if the caches would clear but I am not sure.

I created a fork and merge request for this issue, starting with that previous patch. I tried using a resetAll() command to do a more aggressive cache clear, but this is still not working. I'll try to get some help in Slack.

Setting to NR to trigger a test (which I expect to fail as it fails locally).

Thanks to @fabianx, I got around the cache problem. Then I managed to debug the test. It now passes locally. In the process, I realized we needed one more test case added to the "bad tests" section, so I did that too. It seems like it's probably somewhat robust now (I hope). Time will tell. Anyway, it's ready for a review!

I did some refactoring for efficiency and better code style in the test-related classes. Still needs review.

Looks like I misspelled "delimiter" as "delimeter" in one spot in the NodeVisitor class. I think I just made a commit to fix it, by editing on the GitLab site (1 character change)... might be good now.

Looks great to go, I did not check spelling in code comments

I think that last status change was a mistake? Spell check is done by the bot now, so I assume you meant to leave it at RTBC. If not, change it back...

Yes, I mean RTBC, sorry

MR!253 needs to be rebased on 9.3.x. Only the creator of the MR, which is @jhodgdon, can do that.

The only change is now core has the second line endset

-    $body = preg_replace('|\{\% set.*\%\}|sU', '', $body);
-    $body = preg_replace('|\{\% endset \%\}|sU', '', $body);

I'm not allowed to change target branch in MR so here's a patch

Remove unused argument

How come no one but me can update the MR? I'm back from vacation and can do that but maybe we should just use the patch now? Not sure what to do...

I stuck rebasing, probably we should back to patches but I can't get why it fails now... gonna tackle at weekend

I'm working on the rebase... The MR may be unstable for a bit because I'm setting it back to 9.2.x temporarily so I can see what's going on. Will update issue hopefully soon.

Oh gracious. I have completely screwed up this MR by trying to rebase it, and it still isn't rebased. I think all we can really do at this point is start over. I'll see what I can do...

OK, ignore the MR. Here's I think a patch against 9.3.x, which I hope is identical to the latest from the now broken MR... sigh, this was a mess I made. :(

There's possibly some stuff missing, I don't know...will take another look tomorrow. :(

I found another few changes, and am fixing a lint error...

Here's a new patch designed to not get the same error and tell us more... I'm not on my dev machine where I can run tests at the moment.

I closed the (sorry again!) broken merge request, to avoid confusion. If we want to go back to MR workflow, we can make a new one.

And by the way, the patch @andypost made in #26 and mine in #34 are identical (according to the interdiff utility), except for an extra blank link in the bad_help_topics.translated file in #26. So, now I think we're all on the same page and moving forward. Let's see if we can get the tests to pass.

Hm. I added a test for 9.2.x and it still fails. Weird. I suspect something is going on with the Twig caching. I am looking into it now...

I figured out what was going on in the test. It's failing in the part of the test that is verifying that each translated chunk in the template has valid HTML, and it is only currently failing for templates that have things like this in them:

{% set layout_link_text %}
{% trans %}Block layout{% endtrans %}
{% endset %}

The problem is, this is a "translated chunk", but it is never rendered in this section of the text because we are skipping all variable output in our custom Twig node visitor. So the test just needs to skip any "chunks" that don't end up getting rendered.

Test passes locally now...

Tests pass on the bot now too. Ready for review! I believe this is now the same patch that was RTBC on the MR, plus interdiffs from #36 and #41, which basically added an if() statement to check and see whether the translated "chunk" being tested actually was rendered.

It looks ready for commiter's eyes, thank you @jhodgdon

This is pretty creative. I had to read the docs on AbstractNodeVisitor to understand what is going on. Just a couple of questions.

1. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
   @@ -38,6 +41,14 @@ class HelpTopicsSyntaxTest extends BrowserTestBase {
   +    // Disable Twig caching, because we need to render topic templates
   +    // various times with different things happening.
   +    $parameters = $this->container->getParameter('twig.config');
   +    $parameters['cache'] = FALSE;
   +    $parameters['auto_reload'] = TRUE;
   +    $this->setContainerParameter('twig.config', $parameters);
   
   @@ -292,4 +322,30 @@ protected function listDirectories($type) {
   +      '#template' => $content . "\n{# " . rand() . " #}",
   

   we've got this as well as the `rand()` being appended - do we need both?

2. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicsSyntaxTest.php
   @@ -292,4 +322,30 @@ protected function listDirectories($type) {
   +   * @param string $manner
   +   *   Special manner to render the topic.
   ...
   +  protected function renderHelpTopic(string $content, string $manner) {
   

   Should we standardize the naming as 'type' here?

Good questions!

First off, here's a reroll of the previous patch (due to

use Drupal\Core\Extension\ExtensionLifecycle;

being added to the test class, whatever that is. I named this one with a .txt extension so it wouldn't get tested.

Regarding item 2, in listDirectories($type), $type is 'module', 'theme', or 'profile'. In renderHelpTopic, $manner is a special way to render the topic... Although once it gets to HelpTestTwigNodeVisitor, it is called 'type' there. I think the right choice is to call it 'manner' in the node visitor, so as not to use the same word "type" for two different things in HelpTopicsSyntaxCheck. So here's an interdiff that does that (manner-interdiff).

Regarding item 1, I'm not actually sure... so let's see. I kept running into problems with caching in the tests, and trying different things to fix it, and maybe not all of them were actually needed in the end.

I think the rand() solution is the easiest to understand, so here's an interdiff that takes out the other Twig caching (which also removes more lines of code than removing the other line). (cache-interdiff).

And here's the rerolled + 2 interdiffs patch for review/testing.

It looks like the test still works fine without disabling Twig caching. So, this is ready for review again. cc @andypost!

I think "manner" is better then broader "type", ++ to remove hack on caching (moreover it makes tests faster)

Send re-test after failure in Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderDisableInteractionsTest

Another random test failure. Sent a re-test.

Setting back to RTBC after successful re-test (due to random test failure).

Sent re-test after layout builder random test fail.

Back to rtbc

Adding a test for D10.

Have been a bit hesitant committing this one because it feels like a fair amount of test-specific code to maintain for what is essentially linting. However, we already have more fragile code doing the same linting, and this is self-contained in help topics testing.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!