Problem/Motivation

This issue is undergoing re-scoping and is now a META issue. No further patches should be uploaded to this issue. Please see the child issues below to complete work on patches or review.

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. [done] Make sure that the help topics standards on https://www.drupal.org/docs/develop/documenting-your-project/help-topic-... are updated to use the new functions.
  2. Fix up the rest of the topics in core/modules/help_topics/help_topics to use the new help_route_link function.

Child issues

Remaining tasks

  1. Complete work on all child issues.
  2. After child issues are fixed, ensure that no remaining links need to be updated to use the new help_route_link function in any help topic.
  3. Create any necessary follow-up issues or add to #3121340: Fix up minor copy problems in help topics

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Not necessary. This is an Experimental module.

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost’s picture

Status: Active » Needs review
StatusFileSize
new59.99 KB

Here's find-replace patch, and looks that's enough because URLs used exactly inside of links (no label required as defined in text)

Needs to dig how to write test to prevent usage of url() inside of topics

Status: Needs review » Needs work

The last submitted patch, 3: 3215784-3.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
StatusFileSize
new149.54 KB
new149.54 KB

I was wrong, here's a fixed patch, now it will have collision with #3192585: Fix up topics to use new help_topic_link function

andypost’s picture

StatusFileSize
new148.55 KB

valid patch

Status: Needs review » Needs work

The last submitted patch, 6: 3215784-6.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
StatusFileSize
new752 bytes
new148.57 KB

Fix last link

andypost’s picture

StatusFileSize
new1011 bytes
new148.59 KB

And one more

The last submitted patch, 8: 3215784-8.patch, failed testing. View results

jhodgdon’s picture

I suspect this patch will conflict with #3192585: Fix up topics to use new help_topic_link function... Should we postpone this issue until that is done (it's RTBC)?

andypost’s picture

It will conflict, but I started to think about extracting "mentioned routes" to front-matter key like

---
label: 'Editing an existing view display'
related:
  - views.overview
  - views_ui.add_display
+routes:
+  - views: entity.view.collection
---
-{% set views = render_var(url('entity.view.collection')) %}
+{% set views_link_text %}
+{% trans %}Block layout{% endtrans %}
+{% endset %}
+{% set views_link = render_var(help_route_link(layout_link_text, front_matter.routes.views)) %}

It could allow to integrate with tours module by keeping steps of tour assigned to affected routes and elements

jhodgdon’s picture

Interesting idea. Sounds like a completely separate issue though? I'm not sure how having the route names would help integrate with steps of the tour either? I think it needs more thought.

larowlan’s picture

Interesting idea. Sounds like a completely separate issue though? I'm not sure how having the route names would help integrate with steps of the tour either? I think it needs more thought.

I agree.
I think its good to have the dependencies (in terms of routes) in the frontmatter. But I'm not sure how it relates to tours

andypost’s picture

jhodgdon’s picture

Issue summary: View changes

Making sure that the test part of this issue (which we could split into its own issue) also addresses the help_topic_link function.

jhodgdon’s picture

jhodgdon’s picture

Issue summary: View changes

Updating issue summary to take out tests, as per previous comment.

daffie’s picture

The last child issues of #3041924: [META] Convert hook_help() module overview text to topics have landed. Should the patch from this issue be updated for that?

andypost’s picture

@daffie as I recall we used new functions in all 3 last issues, but I will check asap

I think it should wait for #3192585: Fix up topics to use new help_topic_link function as making reroll here is harder

andypost’s picture

I did a check (git grep 'render_var(url' in help_topics dir) and only help topics link remains there

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new147.04 KB

Here's re-roll/rebase

jhodgdon’s picture

Status: Needs review » Needs work

I have have carefully reviewed about half of the patch, and it's all looking good as far as creating the same output as the previous code. Thanks for the reroll/rebase!

I have some concern though, about translatability. As an example, in one of the forum topics:

+{% set index_link_text %}{% trans %}forums page{% endtrans %}{% endset %}
+{% set index_link = render_var(help_route_link(index_link_text, 'forum.index')) %}
 ...
+  <li>{% trans %}Starting from the {{ index_link }}, navigate to the forum that currently contains the topic.{% endtrans %}</li>

This reads OK in English. But in other languages, the word "the" changes depending on the gender and count of the following noun. There is no way for a translator translating the string that says "Starting from the {{ index_link }}" to know what the next noun is.

So I think a few of the variable names need to be changed in some of the topics, to make it clearer to translators what the link text would be. Maybe something like this:

+{% set forums_page_text %}{% trans %}forums page{% endtrans %}{% endset %}
+{% set forums_page = render_var(help_route_link(forums_page_text, 'forum.index')) %}
 ...
+  <li>{% trans %}Starting from the {{ forums_page }}, navigate to the forum that currently contains the topic.{% endtrans %}</li>

That seems a bit better?

I don't think that the variable names need updates for the topics where the link is part of "In the Manage administrative menu, navigate to X > Y > Z", because in those cases the link text is the name of the page being navigated to, and there is no ambiguity in how to translate the surrounding words. So, here's a list of topics that I think need attention for variable names because the link is embedded in a sentence as in the example above:

  • content_moderation.changing_states.html.twig -- "Users with the {{ content_moderation_permissions_link}} can change workflow states." and "(such as the {{ content_link}} administration page for content items)"
  • content_moderation.configuring_workflows.html.twig -- "Users with the {{ workflows_permissions_link }} permission" and "The permissions are listed under the {{ content_moderation_permissions_link }} section"
  • core.content_structure.html.twig -- "and other topics listed on the main {{ help_link }}"
  • forum.configuring.html.twig -- "Add links to the main {{ index_link }} page"
  • forum.locking.html.twig -- "Starting from the {{ index_link }}"
  • forum.moving.html.twig -- "Starting from the {{ index_link }}"
  • forum.starting.html.twig -- "Starting from the {{ index_link }}"
  • search.overview.html.twig -- "When users visit the main {{ search_link }}"

My suggestion is to use a variable name that exactly matches the link text, or as close as possible.

I am unsure whether this could be a problem for the help_topic_link replacements we did on the other issue?

longwave’s picture

Having to split up the link text and the surrounding sentence does feel like a step backwards.

I might be a bit late here but is there no way of writing this more naturally as HTML, and then wrapping it in something that will filter any links inside?

Or is it possible to use Twig syntax like Starting from the {% link('forum.index') %}forums page{% endlink %}?

longwave’s picture

Or can we use a filter: Starting from the {{ 'forum index' | link('forum.index') }} where link() returns either a formatted link or the original text if the route is inaccessible?

andypost’s picture

Filters and other ways been debated in #3090659: Make a way for help topics to generate links only if they work and are accessible

And here we are, the cornerstone is fixes to potx project and parsers for text

Re #24 looks good idea but it needs more mentions, at least in Russian current text is perfectly translatable, but for Asian languages it could be problem

jhodgdon’s picture

In French and Spanish, this one:

"Starting from the {{ index_link }}"

I think would be problematic. The text that is going in there is "forums page", which would be something like "pagina de foros" (or something like that) in Spanish. "pagina" is feminine, so it should be "la pagina de foros". But "index_link" doesn't tell a Spanish translator that "la" should be used for the and not "el", because they don't know that the main noun in the link text is "page". If the noun there was actually "link" (which is suggested by "index_link"), "enlace" is masculine, and "the" would be "el" in this case.

Wouldn't you have a similar problem in Russian?

andypost’s picture

There's not a-the difference in Russian so as all link texts are the same (список форумов - or kind of) and its usages (identical I bet) - only 2 strings to translate (probably needs to make sure all forum topics using same string to translate)

andypost’s picture

I used to recall #148145: "Forums" title is not localized but too late outside (3am) to find details today

jhodgdon’s picture

So, the items in #24 are different from the other links, in that the link text has been integrated into a sentence. In the other cases, the links are part of navigation instructions, like "Navigate to Structure > Content types" with "Content types" being a link. Those are the ones that I think we need to be careful of, because if the sentence has things like "a", "an", "the" in English, those would be problematic to translate without knowing what the exact words are that would be in the sentence as link text.

We had some of those in the topic URL function patch too, and we changed the sentences around so they always said something like "see the Doing Something topic" (with the exact topic name). Maybe we should do something like that here, so the link text is always the name of the page and is capitalized?

So those Forum ones could all say something like "Starting from the Forums page", with only "Forums" being the link (and that is the page title). That would be easier to translate I think, because the translators would see "Starting from the {{ forums_link }} page" and that is OK to translate because "page" is the main noun.

So the items in #24 would become something like:

  • content_moderation.changing_states.html.twig -- "Users with the Whatever permission can change workflow states" [take out the link] and "(such as the {{ content_link}} administration page for content items)" [that one can stay as it is I think]
  • content_moderation.configuring_workflows.html.twig -- "Users with the Whatever permission" [take out the link] and "The permissions are listed under the {{ content_moderation_permissions_link }} section" [this one is OK]
  • core.content_structure.html.twig -- "and other topics listed on the main {{ help_link }} page"
  • forum.configuring.html.twig -- "Add links to the main {{ forums_link }} page"
  • forum.locking.html.twig -- "Starting from the {{ forums_link }} page"
  • forum.moving.html.twig -- "Starting from the {{ forums_link }} page"
  • forum.starting.html.twig -- "Starting from the {{ forums_link }} page"
  • search.overview.html.twig -- "When users visit the main {{ search_link }} page"
longwave’s picture

StatusFileSize
new1.09 KB

As an alternative approach, a proof of concept for post-filtering the help topics markup to remove links that the user doesn't have access to. The regex approach isn't perfect but seems to work well enough to perhaps explore this further using better code? This way, we don't have to alter the help topics content at all in this issue.

jhodgdon’s picture

Thanks for #32... but we are trying to avoid doing complicated preg search and replace operations in Help Topics, because these are hard to maintain and are considered technical debt by the core maintainers. So I don't think this solution is something we can get committed. We've had tons of pushback on things like that throughout the lifetime of the Help Topics project, and I don't think we want to derail it now. You could open another issue if you want to discuss it further. For now I think on this issue, we need to go with the solution that was already approved on #3090659: Make a way for help topics to generate links only if they work and are accessible after much discussion.

So back to the question of translatable text. I am not asking for a huge change in #24/31. I just think a small change to what we put in the link text and variable names will make it slightly easier for people to translate. We may not completely eliminate the problem, but I think we can make it better. Also, using the page name exactly as the link text (which is another translatable chunk) will make it easier to translate, because it is already translated elsewhere.

andypost’s picture

I think it could use to add special locale context for the string and allow extend (via distinct help context

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

amber himes matz’s picture

Assigned: Unassigned » amber himes matz

I'm working on updating the patch in #23 with the suggestions in #31.

amber himes matz’s picture

Still working on reviewing and creating a new patch. I noticed that #3264945 was merged into 9.4.x and I've merged in those latest changes into my local branch.

amber himes matz’s picture

Assigned: amber himes matz » Unassigned
Status: Needs work » Needs review
StatusFileSize
new192.5 KB

Here's a new patch complete with review of #23 that also incorporates the suggestions in #31. I went through every single help topic (not just the ones in the patch).

- All render_var(url)) calls were replaced with help_topic_route(). (This was already done in the patch in #23.)
- The patch in #23 is pretty old and wouldn't apply or re-base, so I had to manually apply every change. This was a good, but tedious way to review everything and apply @jhodgdon's suggestions to the variable and sentence structure (to make them friendlier to translators).
- All variable names using help_topic_route use the link name (which is usually, but not always the page title), except in cases where additional context is needed for clarity. (Like in the forum topics where every page is called Forums, but they all go to different places.)
- All links to module permissions now consistently use the "fragment" route. The permission itself isn't linked, but a jump-link to the section on the permissions page is, along with pretty consistent text: "The permissions are found under the {{ node_permissions_link }} section."
- Sneaked in a minor typo fix or two and fixed some indentation.

This is ready for review and I hope is ready to go. (Although re-roll might be necessary. Let's see.)

andypost’s picture

Aggregator topic moved to module, so patch needs reroll

amber himes matz’s picture

StatusFileSize
new192.5 KB

Here's an updated patch that applies cleanly to 9.4.x as of now, that takes into account aggregator's help topics moving to core/modules/aggregator/help_topics.

Thanks @andypost.

amber himes matz’s picture

StatusFileSize
new192.47 KB

Color module also moved its help topic to its own help_topics directory in prep for its removal in Drupal 10. Updated patch to account for this and it now applies cleanly to 9.4.x as of now.

Status: Needs review » Needs work

The last submitted patch, 41: moved-agg-and-color-3215784-40.patch, failed testing. View results

amber himes matz’s picture

Status: Needs work » Needs review

The failing test is a random fail, I think. I ran the failing test locally and it passed, so submitting a re-test.

Status: Needs review » Needs work

The last submitted patch, 41: moved-agg-and-color-3215784-40.patch, failed testing. View results

andypost’s picture

Status: Needs work » Reviewed & tested by the community

Pat h applies, as I see changes should solve #31

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review, +Needs followup

@andypost suggested I take a look at this given the information in #28 for example.

The lack of interdiffs makes it pretty difficult to review for a specific issue for translators' needs, especially with a 200K (!) patch. @Amber Himes Matz, can you please provide interdiffs with your changes in the future when using a patch workflow?

Once we have those interdiffs (or the series of changes represented as an MR), we can try to evaluate whether translators would have sufficient context for the translation placeholders.

xjm’s picture

Oops, forgot my last paragraph:

I'm also somewhat concerned about the entire approach. Normally, when we have links in strings, we make the URL itself the only placeholder, so that the user can translate the sentence naturally including the link text that will be correct in context for their language. In a lot of cases the link text itself will need grammatical changes based on the sentence surrounding it.

At the least, we should have a followup about that, but maybe this issue is the place to discuss it since this is apparently a new API or pattern?

andypost’s picture

amber himes matz’s picture

Assigned: Unassigned » amber himes matz
Status: Needs review » Needs work

Thank you for the feedback.

I'm working on providing an interdiff between @andypost's patch in #23 and mine in #37. My plan is:

1. Re-roll the patch in #23 so that it applies.
2. Re-roll the patch in #37 so that it applies.
3. Interdiff these 2 re-rolled patches.

This will make my recent "moved-..." patches obsolete and we can better compare the 2 approaches.

amber himes matz’s picture

Assigned: amber himes matz » Unassigned
Status: Needs work » Needs review
StatusFileSize
new147.08 KB
new19.46 KB
new2.43 KB
new1.12 KB
new169.73 KB

Ok, here's what I've got here:

1. A re-roll of @andypost's patch in comment #23 so that we can have it for comparison (file: reroll-3215784-23.patch).
2. A diff of the re-roll of #23 (file: reroll-diff-23-50.txt)
3. A diff of patches in #38 and #40 (file: diff_38-40.txt).
4. A diff of patches in #40 and #41 (file: diff_40-41.txt).
5. An interdiff of #23 and #41 (file: interdiff_23-41.txt).

The mix of diffs and interdiffs is because of their ability/inability to apply cleanly.

NOTE: The main comparison should be between reroll-3215784-23.patch and the patch in #41 (See attached interdiff_23-41.txt).

Please let me know if I overlooked anything.

Status: Needs review » Needs work

The last submitted patch, 50: reroll-3215784-23.patch, failed testing. View results

xjm’s picture

Oh one other thing; have we addressed @longwave's feedback on the DX in #26?

xjm’s picture

Just taking a couple notes for review and more research. (I am not familiar with the history of this issue and previous related ones, so need to look into it more.)

  1. +++ b/core/modules/aggregator/help_topics/aggregator.creating.html.twig
    @@ -5,15 +5,15 @@
    +  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate back to <em>Configuration</em> &gt; <em>Web services</em> &gt; {{ aggregator_link }}. You should see the new feed or feeds that you added in the list under <em>Feed overview</em>.{% endtrans %}</li>
    

    This is the most common pattern: The link placeholder is part of a breadcrumb trail represented with angle brackets.

  2. +++ b/core/modules/help_topics/help_topics/editor.overview.html.twig
    @@ -8,17 +8,17 @@
    -  <li>{% trans %}Return to {{ filter_overview_topic }} to complete the text format configuration, and be sure to save the text format.{% endtrans %}</li>
    +  <li>{% trans %}Return to {{ filter_overview_topic }} and complete the text format configuration steps; be sure to save the text format.{% endtrans %}</li>
    

    Here is one example of the link being in the middle of the sentence, in a position where in many languages the object of the preposition "to" would need to be in a different case, and therefore the link text might change. I'm not sure if that gets applied to abstract concepts like webpages or proper page titles, but wanted to make note of it.

  3. +++ b/core/modules/help_topics/help_topics/forum.configuring.html.twig
    @@ -8,19 +8,18 @@
    -  <li>{% trans %}Add links to the main <em>{{ index_link }}</em> page (path: <em>/forum</em>), and optionally to individual forum pages, to navigation menus on your site, so that users can find the forums.{% endtrans %}</li>
    +  <li>{% trans %}Add links to the main {{ forums_index_link }} page (path: <em>/forum</em>), and optionally to individual forum pages, to navigation menus on your site, so that users can find the forums.{% endtrans %}</li>
    

    Another example of a link mid-sentence.

  4. +++ b/core/modules/help_topics/help_topics/forum.locking.html.twig
    @@ -3,13 +3,13 @@
    -  <li>{% trans %}Starting from the {{ index_link }}, navigate to the forum that currently contains the topic.{% endtrans %}</li>
    +  <li>{% trans %}Starting from the {{ forums_index_link }} page, navigate to the forum that currently contains the topic.{% endtrans %}</li>
    

    Another example mid-sentence, this time with the link as an object of "from".

  5. +++ b/core/modules/help_topics/help_topics/forum.starting.html.twig
    @@ -3,13 +3,13 @@
    -  <li>{% trans %}Starting from the {{ index_link }}, navigate to the forum where you want to post the topic.{% endtrans %}</li>
    +  <li>{% trans %}Starting from the {{ forums_index_link }} page, navigate to the forum where you want to post the topic.{% endtrans %}</li>
    

    Another example.

  6. +++ b/core/modules/help_topics/help_topics/help.help_topic_search.html.twig
    @@ -18,10 +18,10 @@
    -  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>{{ extend_link }}</em>. Verify that the Search, Help, Help Topics, and Block modules are installed (or install them if they are not already installed).{% endtrans %}</li>
    +  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to {{ extend_link }}. Verify that the Search, Help, Help Topics, and Block modules are installed (or install them if they are not already installed).{% endtrans %}</li>
    

    Another example.

  7. +++ a/core/modules/tracker/help_topics/tracker.tracking_changed_content.html.twig
    @@ -5,12 +5,11 @@
    +  <li>{% trans %}<a href="{{ recent }}"><em>Recent content</em></a>: Shows the content that has been most recently added, updated, or commented on.{% endtrans %}</li>
    -  <li>{% trans %}<em>{{ recent_link }}</em>: Shows the content that has been most recently added, updated, or commented on.{% endtrans %}</li>
    

    Is the diff backwards here?

    This seems different from the other links in the patch and is an example of how I think translation should actually be handled, with the URL as the placeholder and the <a> tag's text left up to the translator.

  8. +++ b/core/modules/help_topics/help_topics/action.overview.html.twig
    @@ -5,14 +5,12 @@ related:
    -<p>{% trans %}Simple actions do not require configuration. They are automatically available to be executed, and are always listed as available on the {{ actions_page }}.{% endtrans %}</p>
    +<p>{% trans %}Simple actions do not require configuration. They are automatically available to be executed, and are always listed as available on the {{ actions_link }} page.{% endtrans %}</p>
    

    Another example where the link is clearly in the sentence. Also not sure here if changing "page" to "link" is actually an improvement?

  9. +++ b/core/modules/help_topics/help_topics/comment.configuring.html.twig
    @@ -6,16 +6,16 @@ related:
    +<p>{% trans %}In order to follow these steps, the Field UI module must be installed. You'll need the Comment module's <em>Administer comments and comment settings</em> permission, in order to change comment settings for a comment field. The permission is listed under the {{ comment_permissions_link }} section. You'll also need to have the appropriate permission for adding fields to the entity type or subtype that the comments are attached to. For example, to add a comment field to content items provided by the Node module, you would need the Node module's <em>Administer content types</em> permission. See permissions listed under the {{ node_permissions_link }} section to configure permissions for content types.{% endtrans %}</p>
    

    Another example of it being mid-sentence, this time as a modifier for "section".

  10. +++ b/core/modules/help_topics/help_topics/comment.moderating.html.twig
    @@ -5,28 +5,24 @@ related:
    -  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Content</em> &gt; <em>{{ comment_published_link }}</em>. A list of all comments is shown.{% endtrans %}</li>
    +  <li>{% trans %}In the <em>Manage</em> administrative menu, navigate to <em>Content</em> &gt; {{ comments_link }}. A list of all comments is shown.{% endtrans %}</li>
    

    Again looks like the diff might be backwards? There was more information in the old link text than the new one.

gábor hojtsy’s picture

So from #3090659: Make a way for help topics to generate links only if they work and are accessible, this feature was added because (a) the user may not have access to the other page, so we don't want it to be a link (b) the help topic or page being linked to may not exist, so we don't want it to be a link. That means that we need to either have a link or not a link yet still keep the internal text.

I personally think that pulling the labels out of the sentences is fine so long as we are linking to pages in the menu tree and stuff like that with explicit titles. Translation will not be as perfect as it would be with inline text, but reusing the menu item label will make translation potentially faster.

An alternate solution would be to implement the link tag as a twig tag, something like (entirely made up):

-  <li>{% trans %}In the Manage administrative menu, navigate to <a href="{{ appearance }}">Appearance</a>.{% endtrans %}</li>
+  <li>{% trans %}In the Manage administrative menu, navigate to {% topic_link render_var(url('system.themes_page')) %}Appearance{% end_topic_link %}.{% endtrans %}</li>

But this would be quite different in terms of underlying twig integration implementation and also require changes in potx I think in how it understands the translatable string (somehow transform the %topic_link% twig tag for the sake of translation lookup).

Why was this ruled out in prior discussions?

amber himes matz’s picture

Status: Needs work » Needs review

1. Re: @xjm in #47

I'm also somewhat concerned about the entire approach. Normally, when we have links in strings, we make the URL itself the only placeholder, so that the user can translate the sentence naturally including the link text that will be correct in context for their language. In a lot of cases the link text itself will need grammatical changes based on the sentence surrounding it.

The related issue, #3192585: Fix up topics to use new help_topic_link function, has already been fixed and the approach already discussed at length and decided up on in #3090659: Make a way for help topics to generate links only if they work and are accessible. I wasn't personally involved in those discussions so I would refer you to the comments there.

What I tried to do in this issue is:

- Rename variables in a writer-friendly way
- Use the page's title as the link text as much as possible (with a few exceptions; see #38)
- Restructure sentences so that a page title value makes sense (hopefully)

From a help topics user's perspective, I think the variable names are easier to understand and make the help topic code easier to read. That's why I was in favor of the suggestion, even though I don't really know whether this helps or hinders translators. If it at least doesn't hinder them in general, I would like to advocate that we move forward with my patch in #41.

2. Re: @xjm in #52

Oh one other thing; have we addressed @longwave's feedback on the DX in #26?

I don't mean to be dismissive, but @andypost responded in #27 (already discussed/decided in #3090659: Make a way for help topics to generate links only if they work and are accessible).

3. Re: #53, questions 7 and 10 where the diff appears to be backwards.

What this appears to be is a diff of @andypost's changes in #23 compared with what was already committed in HEAD. I didn't make any changes in those cases, so there is no difference between the patch in #23 and #41 in those places. So, yeah, the diff isn't accurately showing a difference between mine and @andypost's patches. Sorry for the confusion!

4. Re: @xjm in #53, question 8

I agree that the pattern doesn't work very well here, and the change to "page" isn't really an improvement. I like the idea of using a consistent naming convention for variables in help topics, but I wouldn't want to ironically sacrifice clarity. I'd call it a nit, but if we think it's important, I won't argue.

5. Re: @Gábor Hojtsy in #54

An alternate solution would be to implement the link tag as a twig tag, something like (entirely made up):

-  <li>{% trans %}In the Manage administrative menu, navigate to <a href="{{ appearance }}">Appearance</a>.{% endtrans %}</li>
+  <li>{% trans %}In the Manage administrative menu, navigate to {% topic_link render_var(url('system.themes_page')) %}Appearance{% end_topic_link %}.{% endtrans %}</li>

But this would be quite different in terms of underlying twig integration implementation and also require changes in potx I think in how it understands the translatable string (somehow transform the %topic_link% twig tag for the sake of translation lookup).

Why was this ruled out in prior discussions?

(If I'm understanding your question correctly...) Because we don't have a way in Twig to check to see if a module is installed (for example) before providing a link, like we have in hook_help() using PHP. We don't want to disrupt the user by providing 404s/403s etc, we just want to either give them a helpful link if one is available given their site's current configuration, or provide text if no link is available. This was discussed/decided up on at length in #3090659: Make a way for help topics to generate links only if they work and are accessible.

6. Any other questions I can try to answer?

Since the function itself has already been discussed and decided upon in #3090659: Make a way for help topics to generate links only if they work and are accessible and already partially implemented in #3192585: Fix up topics to use new help_topic_link function, I think it would be the most helpful to settle the question of whether my changes to the patch in #23 (in my patch in #41) make a translator's work more difficult (not less)?

Settling that, we can ensure that the patch we want to commit has undergone a final review.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Issue tags: -Needs followup
StatusFileSize
new147.07 KB
new140.74 KB

The follow-up is #3219923: Add tests to enforce correct use of help_topic_link and help_route_link functions

Moreover this is a last blocker to make help topics stable

Rerolled for 9 vs 10 as aggregator and color are removed in 10

andypost’s picture

StatusFileSize
new2.29 KB
new146.96 KB

fix merge error, tests should be green

The last submitted patch, 57: 3215784-57-10.patch, failed testing. View results

The last submitted patch, 57: 3215784-57-9.patch, failed testing. View results

andypost’s picture

StatusFileSize
new140.63 KB

10.0.x patch

gábor hojtsy’s picture

@andypost asked me to provide feedback.

As I indicated above I think as long as the short link text snippets that are now out of the larger translatable text are menu items and not snippets of text it should be fine with the current approach. ie “modules are typically installed on the <a>Extend</a> page” instead of “modules are typically <a>installed on the Extend page<a>”. More extensive text snippet links like in this later example usually make for easier clicking targets and potentially better accessibility (when read out), but the snippet “installed on the Extend page” would stand alone for translators.

amber himes matz’s picture

Assigned: Unassigned » amber himes matz
amber himes matz’s picture

Issue summary: View changes

Updating issue summary to clarify the context and scope of this issue for reviewers.

amber himes matz’s picture

Status: Needs review » Needs work

I'm working on updating the patch in #61 to get it to apply to 9.5 (nevermind, I see that the 9.5 patch is in a previous comment) and to also make sure any outstanding feedback has been addressed.

amber himes matz’s picture

Status: Needs work » Needs review

Thank you @Gábor Hojtsy for your input. I think this means the Needs subsystem maintainer review tag can be removed, @xjm?

Thank you @andypost for the new patches for 9 and 10 in comments #58 (9.5.x) and #61 (10.0.x).

Reviewers/Committer: These patches appear to be a re-roll update of @andypost's original patch in #23, before the tangent on changing variable names for translator's sake, etc.

Thank you (@andypost) for this because this is exactly what I was going to do next to get this task moving forward again. This means that the discussion around the approach and subsequent patch in comments #24 and following have been resolved to: let's just refactor the code to use the new function and move on, without adding the extra scope of variable name changing, link text changing, etc.

Things I checked for in my review:

- Instances of render_var(url)) that pointed to non-help-topic links were refactored to use the help_route_link function.
- The link text was the same as the menu item when possible (and that the link text matched the menu item over the page title in cases where they differed -- so that instructions could be followed). Exceptions to this are the forum.locking, forum.moving, forum.starting, and search.overview topics where the link texts remain the same as they were before; also, links to sections or text on the Permissions page where there isn't a menu link per se.
- Code formatting and spelling
- Help topics standards

Things I didn't check for:

- Copy edits or any other changes to help topics outside of the refactoring that was done to use the new help_route_link function.

Since this is @andypost's patch and I don't believe it contains any of my changes (i.e. in #41), I think I can RTBC this but I'm not 100% sure. So I'll just say +1 to RTBC the patches in #58 and #61. (I'll ask advice in Slack to see if I can RTBC this.)

I believe is the last blocker to help topics stability, so it would be great to get this in so that Help Topics could be stable in Drupal 10!

amber himes matz’s picture

Assigned: amber himes matz » Unassigned
xjm’s picture

Status: Needs review » Needs work

BTW, I don't agree with dismissing feedback from the multilingual maintainer (or a release manger who happens to have a linguistics background plus knowledge of a dozen languages 😛) just because #3090659: Make a way for help topics to generate links only if they work and are accessible and #3192585: Fix up topics to use new help_topic_link function were already fixed. I think that's part of what's been blocking here -- those previous issues should have also had subsystem review for the translation impact from the start, Gábor's feedback specifically, before we got so far down this road. We can't change the past, though, so let's consider this a lesson for next time. 🙂

I'm still a bit put off by the notion of making translations potentially have grammatical errors and regressing from the true multilingual support that we get with t(), but I agree with Gábor that using only the proper titles of pages and menu links mitigates the problem somewhat. In a lot of languages, you can sort of delineate a proper title of something (like a page or menu link) from the structure of the rest of the sentence, e.g. if it's in quotation marks. If we're relying on that to ensure translatability, though, it becomes very important for every specific string to be constructed carefully (whereas with t() you can follow a few simple rules and give the translator the latitude they need to form correct and idiomatic sentences).

If we're relying on that -- I'm wondering if we should split this patch. The 200K is killer. Can we separate fixes for thing like breadcrumbs, which are going to be fine regardless of language, from fixes where the page is linked in a sentence? I'm sorry to ask for that rescope so late, but I think it'd really go a long way toward giving us the close review we need of the more complicated cases, while letting us commit the bulk of the simpler changes without having to think carefully about every single hunk.

xjm’s picture

When we split the patch, the standalone links issue(s) won't need subsystem maintainer review anymore, but the one with the sentences should still have it so we can read the whole patch. Also tagging for RM review of the rescope (plus that will also make sure that I get a chance to look at it again before commit to spot-check). Thanks!

xjm’s picture

amber himes matz’s picture

Title: Fix up topics to use new help_route_link function » [META] Fix up topics to use new help_route_link function
Issue summary: View changes

This issue is undergoing re-scoping and is now a META issue. No further patches should be uploaded to this issue. Please see the child issues in the issue summary (being created as I write this) to complete work on patches or review.

amber himes matz’s picture

Issue summary: View changes
Status: Needs work » Active

Added links to child issues in issue summary.

andypost’s picture

Category: Task » Plan

Thank you a lot, split looks great 👍

smustgrave’s picture

All the child issues are in review now

amber himes matz’s picture

I'm doing reviews on these. I realize there is some inconsistency in whether to apply <em></em> tags to the final breadcrumb link. The Help Topics Standards and the example in the issue summaries show not to italicize it. But many of the links that we're updating with the new function were italicized. Instead of nit-picking on italics in these issues, I just want to focus on converting the links to use help_topic_link() and move any nit-picks about italics to #3121340: Fix up minor copy problems in help topics, which I feel is a more appropriate place to do style guide/standards updates for all help topics.

amber himes matz’s picture

Issue summary: View changes

Updated issue summary with tasks to create follow-ups.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Status: Fixed » Closed (fixed)

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

andypost’s picture

Last one in!