Problem/Motivation

In #3215784: [META] Fix up topics to use new help_route_link function, the patch was too large and too burdensome to review, so the issue was re-scoped and child issues created. The patches in #58 (9.5.x) and #61 (10.1.x) can be used as a reference.

On #3090659: Make a way for help topics to generate links only if they work and are accessible, we've added two new functions for making safe links with access checks in help topics.
Related change record is https://www.drupal.org/node/3192582

As an example, this is how the block.place.html.twig file was updated in the patch on that issue (that was the only production topic that was updated):

-{% set layout_url = render_var(url('block.admin_display')) %}
+{% set layout_link_text %}
+{% trans %}Block layout{% endtrans %}
+{% endset %}
+{% set layout_link = render_var(help_route_link(layout_link_text, 'block.admin_display')) %}
 
...

-  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Structure</em> &gt; <a href="{{ layout_url }}"><em>Block layout</em></a>.{% endtrans %}</li>
+  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Structure</em> &gt; {{ layout_link }}.{% endtrans %}</li>

...

-  <li>{% trans %}Configure the block and click <em>Save block</em>; see <a href="{{ configure }}">Configuring a previously-placed block</a> for configuration details.{% endtrans %}</li>
+  <li>{% trans %}Configure the block and click <em>Save block</em>; see {{ configure_topic }} for configuration details.{% endtrans %}</li>

Steps to reproduce

N/A

Proposed resolution

1. Update breadcrumb links in steps or standalone "navigate to X" links in the following help topics:

  • core/modules/help_topics/help_topics/ban.banning_ips.html.twig
  • core/modules/help_topics/help_topics/block.configure.html.twig
  • core/modules/help_topics/help_topics/block_content.add.html.twig
  • core/modules/help_topics/help_topics/block_content.type.html.twig
  • core/modules/help_topics/help_topics/book.adding.html.twig
  • core/modules/help_topics/help_topics/book.configuring.html.twig
  • core/modules/help_topics/help_topics/book.organizing.html.twig
  • core/modules/help_topics/help_topics/config.export_full.html.twig
  • core/modules/help_topics/help_topics/config.export_single.html.twig
  • core/modules/help_topics/help_topics/config.import_full.html.twig
  • core/modules/help_topics/help_topics/config.import_single.html.twig
  • core/modules/help_topics/help_topics/config_translation.overview.html.twig

2. Ensure that no other breadcrumb links need to be updated in ban-, block-, or block_content-, config-, config_translation-prefixed help topics. (Search for usages of render_var(url( or uses of HTML anchor tags (for breadcrumbs only!) in those help topics.)

3. Please do NOT include any sentence-embedded links in the patch, as these will be reviewed in a separate issue with respect to translatability.

Remaining tasks

Patch, review, commit.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Not necessary. This is an experimental module.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Amber Himes Matz created an issue. See original summary.

Amber Himes Matz’s picture

Title: Update breadcrumbs in aggregator, color, ban, block, block_content, book, config, config_translation help topics to use help_route_link function » Update breadcrumbs in ban, block, block_content, book, config, config_translation help topics to use help_route_link function
Issue summary: View changes

Updating issue to remove aggregator and color module help topics since these modules are slated for removal in Drupal 10. There is a separate issue for these modules' updates in #3307700: Update breadcrumbs in aggregator, color, and tracker help topics to use help_route_link function.

smustgrave’s picture

Status: Active » Needs review
FileSize
16.17 KB

Copied from the original main patch

Verified it applies to 10.0.x branch also.

Amber Himes Matz’s picture

Status: Needs review » Needs work

@smustgrave Thanks for the patch! Could you add the following (from the original patch):

core/modules/help_topics/help_topics/book.adding.html.twig
core/modules/help_topics/help_topics/book.configuring.html.twig
core/modules/help_topics/help_topics/book.organizing.html.twig

smustgrave’s picture

Status: Needs work » Needs review
FileSize
4.64 KB
21.25 KB

Sorry about that not sure how I missed those

Amber Himes Matz’s picture

Status: Needs review » Needs work

Thank you for the patch @smustgrave!

+++ b/core/modules/help_topics/help_topics/ban.banning_ips.html.twig
@@ -3,12 +3,13 @@ label: 'Banning IP addresses'
+  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Configuration</em> &gt; <em>People</em> &gt; <a href="{{ ban_link }}"><em>IP address bans</em></a>{% endtrans %}</li>

We want to replace the entire <a> tag with the Twig variable we set for the link. So this line should be:

  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Configuration</em> &gt; <em>People</em> &gt; <em>{{ ban_link }}</em>{% endtrans %}</li>

Same goes for the rest of the link replacements. See the original patch (in issue summary) for reference as well.

smustgrave’s picture

Status: Needs work » Needs review
FileSize
20.92 KB
15.56 KB

Sorry about that!

Amber Himes Matz’s picture

Status: Needs review » Needs work
+++ b/core/modules/help_topics/help_topics/block_content.add.html.twig
@@ -6,12 +6,13 @@ related:
+  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Structure</em> &gt; <em>Block layout</em> &gt; {{ library_link }}.{% endtrans %}</li>

Nit. Let's retain the italics on this link for consistency's sake. (So, wrap {{ library_link }} in <em></em> tags.

Otherwise, looking good.

asad_ahmed’s picture

Status: Needs work » Needs review
FileSize
20.93 KB
1.13 KB

Made changes as per #8. Please review. Thanks

Amber Himes Matz’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the patch addressing my nit-pick @asad_ahmed. I've since realized that the "italics on links" nit-pick is actually an issue onto itself or something better handled in #3121340: Fix up minor copy problems in help topics (because it applies to many links in many topics and is a Help Topics Standards issue). It also creates a tiny bit of a rabbit hole here in this issue because of the extra whitespace between the em tag and the opening double-curly braces in your patch. So while I really appreciate your time and work on creating an updated patch, I'm going to go with the patch in #7 and ignore the italics-nit-pick until it is addressed in #3121340: Fix up minor copy problems in help topics. That's why I'm removing credit here.

@smustgrave Thanks for the patch in #7.

I tested using the following steps:

  1. Checked that the patch covered all the files listed in the issue summary.
  2. Installed Drupal 9.5.x
  3. Applied the patch
  4. (Re-) Installed Help Topics (and any other modules with help topics)
  5. Cleared/Rebuilt Cache
  6. Ran cron (to update the search index and use the help search to test each updated topic)
  7. For each modified help topic file, reviewed the code to ensure the conversion was done correctly.
  8. For each modified help topic, searched for the topic using the search on /admin/help
  9. Read the help topic to ensure HTML was rendered correctly.
  10. Tested each updated route link.
  11. Ensured that the text of the link matched the menu item or the page title.

For each of the help topics in this patch, all of my tests passed. Because of that, this (patch in #7 looks good to me.

The last submitted patch, 7: 3307691-7.patch, failed testing. View results

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed bc9e8b0749 to 10.1.x and ab6c8b10a8 to 10.0.x and f62119a728 to 9.5.x. Thanks!

  • alexpott committed bc9e8b0 on 10.1.x
    Issue #3307691 by smustgrave, asad_ahmed, Amber Himes Matz: Update...

  • alexpott committed ab6c8b1 on 10.0.x
    Issue #3307691 by smustgrave, asad_ahmed, Amber Himes Matz: Update...

  • alexpott committed f62119a on 9.5.x
    Issue #3307691 by smustgrave, asad_ahmed, Amber Himes Matz: Update...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.