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

0. Discuss and decide on the best way to handle the sentence-embedded links in these help topics.

1. Update internal links and breadcrumbs in the following help topics:

core/modules/help_topics/help_topics/forum.configuring.html.twig
core/modules/help_topics/help_topics/forum.locking.html.twig
core/modules/help_topics/help_topics/forum.moving.html.twig
core/modules/help_topics/help_topics/forum.starting.html.twig

2. Attempt to embed the link in the help topic copy simply and directly in order to help translators. If possible, use the menu link title for the link text.

3. Get feedback from Multilingual subsystem maintainer on translatability. (When issue is ready for review, the issue reporter should add "Needs subsystem maintainer review" tag and request feedback from @Gábor Hojtsy.)

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.

CommentFileSizeAuthor
#6 interdiff_3-6.txt3.71 KBpooja saraah
#6 3307694-6.patch7.97 KBpooja saraah
#3 3307694-3.patch7.92 KBsmustgrave
#3 interdiff-2-3.txt5.92 KBsmustgrave
#2 3307694-2.patch8.06 KBsmustgrave
Show 1 more file

Comments

Amber Himes Matz created an issue. See original summary.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Active » Needs review
StatusFileSize
new 3307694-2.patch8.06 KB

Copied from original patch

Verified it applies to 10.0.x branch too.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
StatusFileSize
new interdiff-2-3.txt5.92 KB
new 3307694-3.patch7.92 KB

Fixed links. Sorry about that

amber himes matz’s picture

amber himes matz
she/her
Primary language English
Location Portland, OR USA
CreditAttribution: amber himes matz at Drupalize.Me commented
Status: Needs review » Needs work

So, I think for these forum topics, we need to re-work the text, including the link text, so that we're not introducing new translation strings ("forums page") when we could re-use the default menu link title "Forums", as we are doing in other help topics that use help_route_link.

For the link text, instead of "forums page", we should say "Forums" and add, (path: <em>/forums</em>), a convention which is used in the topic "Configuring forums", and which I think sufficiently clarifies that we're talking about the forums index and not the forums configuration page.

Then for the sentence in which the link is used, re-word so that the it's phrased closer to how we would phrase a breadcrumb link. I think this sentence structure enables us to re-use the default menu link "Forums" as a translation string.

This affects 3 forum topics in exactly the same way:

  • forum.locking.html.twig
  • forum.moving.html.twig
  • forum.starting.html.twig

Here's the pattern as applied to forum.locking.html.twig. The same changes should be made to forum.moving.html.twig and forum.starting.html.twig.

  1. +++ b/core/modules/help_topics/help_topics/forum.locking.html.twig
@@ -3,12 +3,13 @@ label: 'Locking a forum topic'
+{% set index_link_text %}{% trans %}forums page{% endtrans %}{% endset %}

    Change to:
    {% set index_link_text %}{% trans %}Forums{% endtrans %}{% endset %}

    And then update the sentence in which the variable index_link is used:

    +++ b/core/modules/help_topics/help_topics/forum.locking.html.twig
@@ -3,12 +3,13 @@ label: 'Locking a forum topic'
+  <li>{% trans %}Starting from the {{ index_link }}, navigate to the forum that currently contains the topic.{% endtrans %}</li>

    Change to:
    <li>{% trans %}Starting from {{ index_link }} (path: <em>/forums</em>), navigate to the forum that currently contains the topic.{% endtrans %}</li>

Edited code formatting.

amber himes matz’s picture

amber himes matz
she/her
Primary language English
Location Portland, OR USA
CreditAttribution: amber himes matz at Drupalize.Me commented

Also, please test the patch in both 9.5.x and 10.1.x and create a separate patch for 10.1.x if the 9.5.x one doesn't apply cleanly. Thank you!

pooja saraah’s picture

pooja saraah CreditAttribution: pooja saraah at Material for Drupal India Association commented
StatusFileSize
new 3307694-6.patch7.97 KB
new interdiff_3-6.txt3.71 KB

Thanks for the suggestion @Amber Himes Matz
Addressed comment #4
Attached patch against Drupal 9.5.x

pooja saraah’s picture

pooja saraah CreditAttribution: pooja saraah at Material for Drupal India Association commented
Status: Needs work » Needs review
amber himes matz’s picture

amber himes matz
she/her
Primary language English
Location Portland, OR USA
CreditAttribution: amber himes matz at Drupalize.Me commented
Status: Needs review » Reviewed & tested by the community

Thank you @pooja saraah for the patches and interdiff and for adding a test run for both 9.5.x and 10.1.x! Super helpful.

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.
11. Tested that patch applied to latest pull of 10.1.x branch.

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 6af6c55bb9 to 10.1.x and a4548a4467 to 10.0.x and ccd70f1075 to 9.5.x. Thanks!

  • alexpott committed 6af6c55 on 10.1.x 
    Issue #3307694 by smustgrave, pooja saraah, Amber Himes Matz: Update...

  • alexpott committed a4548a4 on 10.0.x 
    Issue #3307694 by smustgrave, pooja saraah, Amber Himes Matz: Update...

  • alexpott committed ccd70f1 on 9.5.x 
    Issue #3307694 by smustgrave, pooja saraah, Amber Himes Matz: Update...

Status: Fixed » Closed (fixed)

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