Make a way for help topics to generate links only if they work and are accessible

Is there an existing Twig URL function? See also #2996305: Add support for the url() function to twig {%trans%}

Doh, right. :) So yes, that does sound like a good way to implement this functionality.

Hm.... This is interesting, but not what we really want to happen in help topics normally:

+{% set another_topic_href %}
+  {{ url('this.route_does_not_exist') }}
+{% endset %}
+{% if url %}
+  <p><a href="{{ another_topic_href }}">Another topic</a></p>
+{% endif %}

What we usually want to happen is to make a link to either another topic or an admin page if the URL exists, and if it doesn't exist, we want the text there but either for it not to be a link or to just link to # so it's not broken.

So the logic we want would usually be something like "If the URL is found, use it, and otherwise use # for the URL".

Also I've been told we should not mix things in the help topics module (currently experimental) with things in Core in general, in the same patch... so probably the test inside help topics doesn't belong in this patch.

Couldn't stand the typo any more. :)

Adding note to summary that this also applies to 403 (no permission).

Would it be possible to add something to $options that would let you say either "fail with an exception" or "return null" or even "return '#'" as the on-exception behavior? That way some Twig templates, like help templates, could use the "return '#'" option, and the default behavior or "fail with an exception" could continue for those who want it.

So maybe it would be:
$options['error_behavior']

with possible values:
- 'exception'
- 'null'
- any other string (that string will be returned if there is an exception)

There are the types of links we are making in topics:

a) Topic to topic cross-links in the text. What I've been doing when writing topics is only making links in the text from topic A to B if they are either from the same module, or if B is a dependency of A, to avoid the problem of a topic not existing and the URL failing to compute.

b) Related topics section -- for this we do not use the Twig template at all. It's just a list of machine names of topics and the help system turns them into links if they exist, and ignores if they don't.

c) Links to admin pages in the text. This is probably the main use case for this issue. For example, in a page about how to create a view, the first step would be "Navigate to Admin > Structure > Views" and "Views" would be a link to the admin/structure/views page. This link could conceivably not be available to the person reading the help topic, or could conceivably be not defined if the views_ui module was not present. However, that second problem is not an issue in topics, in practice, because the topic that needs a particular admin page link is owned by the module that defines the route, or a module that it depends on (in this example, they are in the views_ui module namespace). It is possible that the person reading the topic does not have permission to go to that link, but I think the link still gets made and they get a 403.

As a note, we are not linking to module names the way we did in the hook_help (as illustrated in the issue summary example and comment #32), where we would link to the main module overview help page.

I guess in the 8 months since this issue was created, we went from a "we need to reproduce what we can do in hook_help" mentality to a "this new system can be used in different ways from hook_help" mentality, and it's possible we don't need this now.

I agree that we do not show links the current user cannot see in menus, node tabs, views, and other places. However, the Url class itself, as far as I know, does not check access when making links. I think the caller has to check access, and the menu, node, and views modules do that checking.

I don't think we checked access in hook_help() when we linked to admin pages either.

All good points. Taking all of that into account...

a) What we're currently doing in the Help Topic Twig templates (which contain the topic text) for URLs is code that looks like this:

{% set appearance = render_var(url('system.themes_page')) %}
...
 navigate to <a href="{{ appearance }}">Appearance</a>
...

Some of the url() calls have additional variables, but that's basically it.

b) For this application, I really don't think we want to do a lot of logic and checking and contingencies... What we really want is an easy way (preferably, one function call) that would generate the URL if the route/parameters are valid and the user can access it, and return '#' if not so that the text in the help topic still works. If there are too many contingencies in the Twig file, it's not so easy for non-programmers to write or edit the documentation, and translation also becomes more difficult.

c) See also #2996305: Add support for the url() function to twig {%trans%}

Great idea -- I think this is the right solution.

OK, I think what we would need to do to make this happen is:

a) Define a help_topics.services.yml file with an entry something like this:

  help_twig.extension:
    class: Drupal\help_topics\HelpTwigExtension
    arguments: ['@renderer', '@url_generator']
    tags:
      - { name: twig.extension }

(Not sure if both of those arguments are needed?)

b) Define the HelpTwigExtension class. It would need some of the constructor stuff from the TwigExtension class that is being patched in the latest patch, and it would define methods:

  public function getFunctions() {
    return [
      new TwigFunction('help_url', [$this, 'getUrl'], ['is_safe_callback' => [$this, 'isUrlGenerationSafe']]),
    ];
   }

  // similar docs from this function in TwigExtension, but add info about what happens for missing route/parameters
  public function getUrl($name, $parameters = [], $options = []) {
      // patched function body from this patch
  }

c) The tests from this patch, but put them into a new test class under core/modules/help_topics.

d) Replace all calls to url() in existing committed help topics Twig files (in core/modules/help_topics/help_topics) to use help_url() instead.

I'm working on a patch for this.

Here's a patch that implements #40. The unit test in the patch passes locally, and I also tested several of the help topics that were updated to use the new function, and their URLs are fine.

In addition to what is in #40, I added a bit of code for a functional test, which adds a nonexistent route URL to one of the test help topics, and verifies that the link is there with URL '#'. This test passes too.

I didn't make an interdiff, since there is no real overlap between the previous patch and this one (although I used quite a bit of the code, it is in different files).

I disagree about the exceptions being more specific. This function is in the help_topics module, and is only for help topics. For this purpose, someone is trying to view a help topic because they need to learn how to do something. I think we don't want the whole topic to not display because something failed in making one link -- the person viewing the topic will most likely not know what is wrong or how to fix it. That is the whole point of this -- we want a fail-safe way to make links, only for help topics, which should be visible if it is at all possible.

Regarding having more code in the try {} vs. more code in the catch {}, I am ambivalent. If someone wants to redo the patch and rearrange it, that is fine with me.

Suggestion for comment that currently says:

+      // Return the fallback because viewer of help topic may lack of access to
+      // URL or referred route is not exists because of missing module. Also
+      // parameters for route may change since topic is written.

We want to view the topic rather than throwing an exception if a route is not found or a parameter is incorrect (usually due to a missing or updated module). In this case, return a '#' as the URL.

I also think we need to update the doc block, because it currently says the URL is "safe" and that it will always return something. With the current patch, this is not the case.

+  /**
+   * Generates a safe absolute URL, given a route name and parameters.
+   *
+   * @param string $name
+   *   The name of the route.
+   * @param array $parameters
+   *   An associative array of route parameter names and values.
+   * @param array $options
+   *   (optional) An associative array of additional options. The 'absolute'
+   *   option is forced to be TRUE.
+   *
+   * @return array
+   *   A render array with generated absolute URL for the given route. If
+   *   an exception occurs, such as a missing route or missing parameters,
+   *   the URL returned is '#'.
+   *
+   * @see \Drupal\Core\Template\TwigExtension::getUrl()
+   */
+  public function getUrl(string $name, array $parameters = [], array $options = []): array {

So the first line should probably be:

Returns an absolute URL, given route name and parameters, checking for some exceptions.

And the @return docs second sentence should be:

If a missing route, missing parameter, or invalid parameter exception occurs, the URL returned is '#'.

This all looks good to me. I could not set it to RTBC, because a lot of this patch is mine from #42.

Also, I do have one question, in core/modules/help_topics/src/HelpTwigExtension.php :

+  public function getFunctions() {
+    return [
+      new TwigFunction('help_url', [$this, 'getUrl'], ['is_safe_callback' => [$this, 'isUrlGenerationSafe']]),
+    ];
+  }

It looks like this is expecting we have a method isUrlGenerationSafe() on this class, but this method does not exist? Probably my fault, but I think we either need to not declare an is_safe_callback, or copy the isUrlGenerationSafe() method from core/lib/Drupal/Core/Template/TwigExtension.php? Probably? I really don't know what the is_safe_callback thing does.

Setting to Needs work because I don't think we should be referencing a method that doesn't exit.

Hm. In the file core/lib/Drupal/Core/Template/TwigExtension.php the mehtod isUrlGenerationSafe() has much more complicated logic to determine whether the URL is safe. Just saying "this is safe" doesn't make sense to me, since it's essentially the same URL generator function?

I still don't think #51 was the correct fix for #50. I think the correct fix is to either call or copy in the isUrlGenerationSafe() function from the core TwigExtension class.

Update title to be more generic (there are other problems, such as permissions, mentioned in the issue summary).

I also took a fresh look at this patch, and I have some comments. Aside from the first item, they all pertain to the core/modules/help_topics/src/HelpTwigExtension.php file:

a) We've probably had some commits of topics since the patch was made, so they would need to be updated also to use the new Twig function to generate URLs.

b) nitpick:

+/**
+ * A class providing Drupal Twig extensions for help topics.

We generally don't start out docs with "A class providing". This should probably just say "Twig extensions for help topics.".

c)

+   * @return array
+   *   A render array with generated absolute URL for the given route. If
+   *   an exception occurs, such as a missing route or missing parameters,
+   *   the URL returned is '#'.

We need this function to also cover the case where the user does not have permission to use the route. I don't see this in the documentation or in the code or in the test.

d) The Kernel test verifies generation of an existing route, but doesn't seem to verify any of the cases that should return '#'. Should it? It does seem to be covered in the Unit test (aside from the access permissions part) and the Functional test. Honestly, I'm not sure why we have the Kernel test?

[edit, added] e) The comments from #50 and subsequent discussions about whether that was the right fix still are open questions for me.

Updating summary.

To get this going, I have created a new patch with some of #61 addressed (I had already worked on this issue earlier so I wasn't eligible to RTBC anyway). Things I did:

1. Fixed up the API docs as I suggested in #61.
2. Modified help topics committed after the previous patch was made.
3. Added a @todo in code & unit test to remind us we still need to deal with the user has no permission to access the route case; also updated the function API docs to say it covers that. We still need to make that happen.
4. Reviewed the question about whether we can assume that our URL generation is safe or not. In
   core/lib/Drupal/Core/Template/TwigExtension.php
   the isUrlGenerationSafe() function has a bunch of cases, depending on the URL parameters, that only bypass escaping if the URL is definitely safe:

      * The URL generation process percent encodes non-alphanumeric characters.
      * Thus, the only character within a URL that must be escaped in HTML is the
      * ampersand ("&") which separates query params. Thus we cannot mark
      * the generated URL as always safe, but only when we are sure there won't be
      * multiple query params. This is the case when there are none or only one
      * constant parameter given. For instance, we know beforehand this will not
      * need to be escaped:
      * - path('route')
      * - path('route', {'param': 'value'})
      * But the following may need to be escaped:
      * - path('route', var)
      * - path('route', {'param': ['val1', 'val2'] }) // a sub-array
      * - path('route', {'param1': 'value1', 'param2': 'value2'})
      * If param1 and param2 reference placeholders in the route, it would not
      * need to be escaped, but we don't know that in advance.
   

   So I do not think it is OK to bypass that by setting is_safe to 'html'. Our choices are:
   a. Copy in the isUrlGenerationSafe() function from the core TwigExtension class (ugh! copy code!) or
   b. Make a trait for just that function so we can call it [we don't and can't extend the core class] or
   c. Don't say our URL is safe.
   In this patch, I chose to do (c). There will be a small hit in performance for the few URLs that are typically in a Help Topic template, and we won't have to worry about maintaining this function to be the same as the one in the Core class.

5. Removes the Kernel test that is in this patch. It seems like the Unit test is more comprehensive, and the Functional test (among many other things it does) verifies that the system functions. Given that, I don't think we need the Kernel test, so I took it out.

Since one item still needs to be done (verify permissions and return # if user has no permission), I am leaving this at Needs Work (after setting to Needs Review temporarily so we can see if it passes tests and code sniffs).

Note: I'm not sure whether we really *can* or *should* actually verify permissions. The idea was that a user shouldn't see links in topics that take them to 403 pages. But it would mean we would have to deal with caching and I'm not sure at this point in the rendering whether we have the ability to even do that? So the right answer might be that we need to live with 403 links in topics.

No longer needs security review as we are no longer asserting that the URL is always not needing escaping. Also setting back to needs work for the end of #63. I'll see if I can get some answers in Slack about whether we can actually check permissions and properly trigger caching from within a TwigExtension.

I asked about how/whether we could do this in Slack, and got some help from fabianx (seconded by joelpittet). What I learned:

a.

Whenever you write:
{{ $something }}
in a twig template you in essence (not literally) write:
print twig_render($context['something']);
So anything that is output goes always via "render", so any twig function can return render arrays as well.

b.

So even if it looks wrong:
<a href="{{ help_url("/admin/content") }}"
with help_url being:

function help_url() {
  $build = [
    '#markup' => "Link or not" 
  ];
  \Drupal::service('renderer')->addCacheableDependency($build, $access);
  return $build;
}

should work perfectly fine.

So, we should be able to do this. We'll need to:
- get the bubbleable metadata from the URL generator and add that to the build array (honestly I'm not sure why the core Twig url() function doesn't do this?)
- also add in an access check similar to (b) above.

I'm going to work on this today.

Doh! I should have looked more carefully at the patch we already had. We were already doing some stuff with cache tags in our URL generator.

Anyway, here's a patch that I think takes care of the access check. With tests that pass locally.

Weird. Some of those tests passed locally. Maybe I missed something in the patch, or maybe you can't inject the access manager into a Twig thing... Will take a look but not sure when. Maybe tomorrow.

Doh. Typo.

I think the Unit test should have a few lines to verify that cache stuff from the URL generation and the access check make it to the render array, just to be exhaustive. For the access check, should verify for both the no-access and the has-access check. I should have some time today to update the patch.

OK, here's one more new patch, with a few lines added to the Unit test so that it verifies cache information from both the URL and the Access object are carried through.

I'm not seeing that @todo in the latest patch file... did you review the latest patch?

Looks like one of the help topics changed a bit and the patch doesn't apply cleanly, so rerolling.

Hm.... So here's an example of the two most common ways we are using the url function (for links to other topics, and for links to admin pages). I can think of three ways we could make this look:

a) Current patch:

{% set user_topic = render_var(help_url('help.help_topic', {'id': 'user.overview'})) %}
{% set node_add = render_var(help_url('node.add_page')) %}
...
<p>{% trans %}See <a href="{{ user_topic }}">Managing user accounts and site visitors</a> for more about permissions.{% endtrans %}</p>
...
<li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Content</em> &gt; <a href="{{ node_add }}"><em>Add content</em></a>.{% endtrans %}</li>

b) If we had a help_link function:

{% set user_topic = render_var(help_link('Managing user accounts and site visitors', 'help.help_topic', {'id': 'user.overview'})) %}
{% set node_add = render_var(help_link('Add content', 'node.add_page')) %}
...
<p>{% trans %}See {{ user_topic }} for more about permissions.{% endtrans %}</p>
...
<li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Content</em> &gt; <em>{{ node_add }}</em>.{% endtrans %}</li>

c) If we had a help_link function that we embedded within the trans section:

<p>{% trans %}See {% help_link('Managing user accounts and site visitors', 'help.help_topic', {'id': 'user.overview'}) %} for more about permissions.{% endtrans %}</p>
...
<li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Content</em> &gt; <em>{% help_link('Add content', 'node.add_page') %}</em>.{% endtrans %}</li>

I don't like option (c) -- it gives translators too much opportunity to screw up the parameters, such as trying to translate IDs like 'user.overview' or "node.add_page" (I see this a lot in the user guide translations). But that would seem to be the only way to keep the link text and the rest of the sentence together in a single "chunk" of translation.

Option (b) seems sort of OK to me... The UI strings that go into the link text are for the most part either help topic titles or chunks of UI text that already appear stand-alone in the UI, so they are already being translated as separate chunks of text. But, as noted in the previous comment by catch, that would make the translation more piecemeal.

Given that, my inclination is to stick with option (a). Thoughts? Other options that would be even better?

Hm.... I was going to write up a patch that would convert using this hypothetical help_link function as in option B in #78, but I realized I missed something -- we would need to pass the link text through Twig translation. So it would need to look something like this:

{% set user_topic = render_var(help_link({% trans %}'Managing user accounts and site visitors'{% endtrans %}, 'help.help_topic', {'id': 'user.overview'})) %}
{% set node_add = render_var(help_link({% trans %}'Add content'[% endtrans %}, 'node.add_page')) %}
...
<p>{% trans %}See {{ user_topic }} for more about permissions.{% endtrans %}</p>
...
<li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Content</em> &gt; <em>{{ node_add }}</em>.{% endtrans %}</li>

I don't know how to make the syntax work here... can you even do a % trans inside of a % set? Where do you put the quotes? This just looks wrong. ???

Regarding #79 -- once we figure out what option we're going with, I'll create a followup issue to add it to the syntax test. Adding tag now and adding note to issue summary so it doesn't get forgotten.

Oh yeah, we also need to update the help topics standards once we decide, so adding that to the follow-up list.

So, I'm not sure where to go with this, given #81. Is it possible to translate text within a set statement? If so, how? Seems messy... I'm inclined to stick with the current patch, but open to alternatives.

Meanwhile, here's a new patch that fixes up the topics in recently-committed #3095734: Convert config module hook_help() to topic(s) just like everything else in this patch. I verified that all the currently-committed topics are included in the patch too.

This is all very useful -- many thanks!

So... I think we should do the following:

a) Make a help_topic_link() function. It would render into a link to a help topic, using the topic's label/title as the link text. That would eliminate the need to provide the link text, because I think we are always using the topic title. It would also mean when titles of topics change, we wouldn't have to update other topics that link to them to stay consistent.

b) Make a help_link function that works as Fabinx suggested in #85. We would use that more complex structure for links to admin pages.

c) Take out the render_var calls in our set statements, because we are not using complicated logic in our templates.

I'll take a stab at this... sometime in the next few days.

Here's most of a new patch. What I did:
a) Reverted all the changes to the help topics from the previous patch, except I forgot the ones in the test module.
b) Made new Twig functions for making links to help topics and generic routes, and took out the function for making a bare URL.
c) Modified just one of the help topics to use the new functions.
d) Edited the test topics to use the new functions.
e) Got most of the tests working [see below, the TwigExtension unit test is not working.]
f) The "interdiff" file is a bit odd because it's an interdiff from (a) rather than from the last patch. And the patch is a lot smaller because I didn't go through and modify all of the help topics to use the new functions yet. I figured we should get the system working as we want it first, and then go modify all the topics.

A few additional caveats:
1. Fabianx thought we shouldn't have to use render_var (see comment #86), but I could not get the links to render without wrapping them in render_var, so I had to put that back in.

2. The '#type' => 'link' render array expects '#url' to be a Url object, not a string and not a GeneratedUrl object. The previous patch was using an injected UrlGenerator, but the objects it produced did not work in the link, so I reverted to calling a static method on the Url class to generate the URL. Questions:
- 2a: Are we supposed to use Url::fromRoute() inside classes? I guess the core TwigExtension class is using it in places, so maybe that is OK.
- 2b: This makes the Twig functions not unit testable. That's annoying. I didn't write a new test yet, and I'm not sure exactly how would be best to do it.

Couldn't we make the Link class work with a GeneratedUrl?
Let's see... Here's the code in Link that uses the URL:
https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/C...

It seems like that might be possible. We'd need to do it in a separate issue (this issue needs to be pure Help Topics Experimental and not mix in other Core changes)... But yeah, I guess we don't have to use a Link render array. I'll work on a patch for that. Thanks for the suggestions!

Ugh. LinkGenerator also only works with Url objects, not with GeneratedUrl objects.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Utility%2...

So... ???

I've had a few discussions in Slack with fabianx and others. I decided the best way to go was to write a Functional test. So, I added some parts to the existing HelpTopicsTest to test making links with:
- Invalid routes
- Missing route parameters
- Invalid route parameters
- No access
- Valid routes/parameters
- Link to a missing topic
- Link to a valid topic
that all of these work. The test found some bugs in the code too (hooray for testing!). So, here's a new patch that now has a test.

So far it only fixes 1 non-test help topic to use the new functions, which keeps the patch small and reduces the need to redo the patch whenever new topics are committed. I propose we leave this patch doing that, and follow up with a separate issue to fix all the other topics. Adding this follow-up to the issue summary (which already had a follow-up for verifying we are using the new functions exclusively).

Good idea. Here's an updated patch.

I'm waiting on creating the follow-up issues until this is RTBC, because this issue has gone through several iterations on its ideas, and I don't want to keep redoing the followups.

Thanks for the review!

Adding similar things to Core outside of Help Topics would need to be a separate issue, because Help Topics is an experimental module and we cannot mix issues between Experimental and Stable parts of Core. This was a specific need for help topics twig files and I don't think it's necessarily applicable to other templates, which tend not to have a lot of content in them like Topics do.

This issue has been under review by several other people. I'm going to give them a chance to object before I create the followup issues. They could also be created after the commit. The last time this was RTBC, a committer put it back to Needs Work and we totally changed the approach... so I would prefer to know this is stable and really ready to commit before wasting effort creating follow-up issues. Don't worry, it will really happen.

Sorry about that! Updating issue summary & title, and adding change record: https://www.drupal.org/node/3192582

I also added the follow-up issue. #3192585: Fix up topics to use new help_topic_link function

Guess this can go back to RTBC.

The follow-up issue that was created earlier already includes that: #3192585: Fix up topics to use new help_topic_link function
I will un-postpone it now. Thanks for committing!

I spun off the tests into their own issue: #3219923: Add tests to enforce correct use of help_topic_link and help_route_link functions

And by the way, we now have two spin-off issues to use the two functions:
#3215784: [META] Fix up topics to use new help_route_link function and #3192585: Fix up topics to use new help_topic_link function