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/help.help_topic_search.html.twig
  • core/modules/help_topics/help_topics/image.style.html.twig
  • core/modules/help_topics/help_topics/language.add.html.twig
  • core/modules/help_topics/help_topics/language.detect.html.twig
  • core/modules/help_topics/help_topics/layout_builder.overview.html.twig
  • core/modules/help_topics/help_topics/locale.import.html.twig
  • core/modules/help_topics/help_topics/locale.translate_strings.html.twig
  • core/modules/help_topics/help_topics/locale.translation_status.html.twig
  • core/modules/help_topics/help_topics/media.media_type.html.twig
  • core/modules/help_topics/help_topics/menu_ui.content_type_configuration.html.twig
  • core/modules/help_topics/help_topics/menu_ui.menu_item_add.html.twig
  • core/modules/help_topics/help_topics/menu_ui.menu_operations.html.twig
  • core/modules/help_topics/help_topics/menu_ui.overriding.html.twig

2. Ensure that no other breadcrumb links need to be updated in help-, image-, language-, layout_builder-, locale-, media-, and menu_ui-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.

_shY’s picture

Status: Active » Needs review
FileSize
25.81 KB

Copied from the original patch.

Amber Himes Matz’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the patch @_shY!

In related issues, I've brought up some nit-picks about italicizing the breadcrumb link, but as I mentioned in this comment on the parent meta, I've decided to leave that to #3121340: Fix up minor copy problems in help topics.

I tested using the following steps:

1. Installed Drupal 9.5.x
2. Applied the patch
3. (Re-) Installed Help Topics (and any other modules with help topics)
4. Cleared/Rebuilt Cache
5. Ran cron (to update the search index and use the help search to test each updated topic)
6. For each modified help topic file, reviewed the code to ensure the conversion was done correctly.
7. For each modified help topic, searched for the topic using the search on /admin/help
8. Read the help topic to ensure HTML was rendered correctly.
9. Tested each updated route link.
10. 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 looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll.

Amber Himes Matz’s picture

Status: Needs work » Needs review

Just tried re-applying the patch after a fresh pull of 9.5.x and it still applies cleanly. @smustgrave kicked off a new test run so hopefully that will verify this, if not we'll look into it.

Amber Himes Matz’s picture

Status: Needs review » Reviewed & tested by the community

The test run added today passed. I discussed this with @smustgrave in Slack and he also re-tested the patch and it cleanly applied. So, setting back to RTBC for another look, @alexpott. Thank you.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

It doesn't apply on 10.1.x... so we need a patch for that branch too. It's because seven has been removed from core and that changed this file. And a 3way merge doesn't resolve the conflict either.

smustgrave’s picture

Status: Needs work » Needs review
FileSize
25.81 KB
Amber Himes Matz’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.18 KB

Thanks @smustgrave for the 10.1.x-specific patch. It applies to a recent pull of 10.1.x.

I've attached an interdiff which shows that the only change in this patch from the one in #2 is to replace the mention of the "core Seven theme" with the "core Claro theme".

I think this is good to go, @alexpott.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 818cdde352 to 10.1.x and 16fe280fcc to 10.0.x. Thanks!
Committed 9e6bb0d and pushed to 9.5.x. Thanks!

  • alexpott committed 818cdde on 10.1.x
    Issue #3307696 by _shY, smustgrave, Amber Himes Matz: Update breadcrumbs...

  • alexpott committed 16fe280 on 10.0.x
    Issue #3307696 by _shY, smustgrave, Amber Himes Matz: Update breadcrumbs...

  • alexpott committed 9e6bb0d on 9.5.x
    Issue #3307696 by _shY, smustgrave, Amber Himes Matz: Update breadcrumbs...

Status: Fixed » Closed (fixed)

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