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> > <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> > {{ 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
- [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. - Fix up the rest of the topics in core/modules/help_topics/help_topics to use the new help_route_link function.
Child issues
- #3307691: Update breadcrumbs in ban, block, block_content, book, config, config_translation help topics to use help_route_link function
- #3307692: Update breadcrumbs in contact, content_translation, cron, editor, field_ui, filter help topics to use help_route_link function
- #3307694: Update internal links in forum help topics to use help_route_link_function
- #3307695: Update sentence-embedded internal links in content_moderation, core.content_structure, search.overview help topics to use help_route_link function
- #3307696: Update breadcrumbs in help, image, language, layout_builder, locale, media, and menu_ui help topics to use help_route_link function
- #3307697: Update breadcrumbs in migrate_drupal_ui, responsive_image, search.configuring, search.index help topics to use help_route_link function
- #3307698: Update breadcrumbs in statistics and system help topics to use help_route_link function
- #3307699: Update breadcrumbs in user and views_ui help topics to use help_route_link function
- #3307700: Update breadcrumbs in aggregator, color, and tracker help topics to use help_route_link function
Remaining tasks
- Complete work on all child issues.
- 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.
- 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.
| Comment | File | Size | Author |
|---|---|---|---|
| #61 | 3215784-59-10.patch | 140.63 KB | andypost |
| #58 | 3215784-58-9.patch | 146.96 KB | andypost |
| #58 | interdiff.txt | 2.29 KB | andypost |
| #50 | diff_40-41.txt | 1.12 KB | amber himes matz |
| #50 | diff_38-40.txt | 2.43 KB | amber himes matz |
Comments
Comment #2
andypostComment #3
andypostHere'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 topicsComment #5
andypostI was wrong, here's a fixed patch, now it will have collision with #3192585: Fix up topics to use new help_topic_link function
Comment #6
andypostvalid patch
Comment #8
andypostFix last link
Comment #9
andypostAnd one more
Comment #11
jhodgdonI 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)?
Comment #12
andypostIt will conflict, but I started to think about extracting "mentioned routes" to front-matter key like
It could allow to integrate with tours module by keeping steps of tour assigned to affected routes and elements
Comment #13
jhodgdonInteresting 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.
Comment #14
larowlanI 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
Comment #15
andypostFiled follow-up for tours #3219057: Integrate help_topics with tours
Comment #16
jhodgdonMaking sure that the test part of this issue (which we could split into its own issue) also addresses the help_topic_link function.
Comment #17
jhodgdonI spun off the tests into their own issue: #3219923: Add tests to enforce correct use of help_topic_link and help_route_link functions
Comment #18
jhodgdonUpdating issue summary to take out tests, as per previous comment.
Comment #19
daffie commentedThe 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?
Comment #20
andypost@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
Comment #21
andypostI did a check (
git grep 'render_var(url'in help_topics dir) and only help topics link remains thereComment #22
daffie commented#3192585: Fix up topics to use new help_topic_link function has landed.
Comment #23
andypostHere's re-roll/rebase
Comment #24
jhodgdonI 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:
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:
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:
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?
Comment #25
longwaveHaving 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 %}?Comment #26
longwaveOr 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?Comment #27
andypostFilters 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
Comment #28
jhodgdonIn 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?
Comment #29
andypostThere'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)
Comment #30
andypostI used to recall #148145: "Forums" title is not localized but too late outside (3am) to find details today
Comment #31
jhodgdonSo, 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:
Comment #32
longwaveAs 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.
Comment #33
jhodgdonThanks 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.
Comment #34
andypostI think it could use to add special locale context for the string and allow extend (via distinct
helpcontextComment #36
amber himes matzI'm working on updating the patch in #23 with the suggestions in #31.
Comment #37
amber himes matzStill 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.
Comment #38
amber himes matzHere'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.)
Comment #39
andypostAggregator topic moved to module, so patch needs reroll
Comment #40
amber himes matzHere'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.
Comment #41
amber himes matzColor 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.
Comment #43
amber himes matzThe failing test is a random fail, I think. I ran the failing test locally and it passed, so submitting a re-test.
Comment #45
andypostPat h applies, as I see changes should solve #31
Comment #46
xjm@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.
Comment #47
xjmOops, 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?
Comment #48
andypostI bet the follow-up is #3219923: Add tests to enforce correct use of help_topic_link and help_route_link functions
Comment #49
amber himes matzThank 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.
Comment #50
amber himes matzOk, 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.
Comment #52
xjmOh one other thing; have we addressed @longwave's feedback on the DX in #26?
Comment #53
xjmJust 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.)
This is the most common pattern: The link placeholder is part of a breadcrumb trail represented with angle brackets.
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.
Another example of a link mid-sentence.
Another example mid-sentence, this time with the link as an object of "from".
Another example.
Another example.
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.Another example where the link is clearly in the sentence. Also not sure here if changing "page" to "link" is actually an improvement?
Another example of it being mid-sentence, this time as a modifier for "section".
Again looks like the diff might be backwards? There was more information in the old link text than the new one.
Comment #54
gábor hojtsySo 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):
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?
Comment #55
amber himes matz1. Re: @xjm in #47
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
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
(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.
Comment #57
andypostThe 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
Comment #58
andypostfix merge error, tests should be green
Comment #61
andypost10.0.x patch
Comment #62
gábor hojtsy@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.Comment #63
amber himes matzComment #64
amber himes matzUpdating issue summary to clarify the context and scope of this issue for reviewers.
Comment #65
amber himes matzI'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.Comment #66
amber himes matzThank 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!
Comment #67
amber himes matzComment #68
xjmBTW, 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 witht()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.
Comment #69
xjmWhen 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!
Comment #70
xjmRelevant references for scoping this:
Comment #71
amber himes matzThis 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.
Comment #72
amber himes matzAdded links to child issues in issue summary.
Comment #73
andypostThank you a lot, split looks great 👍
Comment #74
smustgrave commentedAll the child issues are in review now
Comment #75
amber himes matzI'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.Comment #76
amber himes matzUpdated issue summary with tasks to create follow-ups.
Comment #78
andypostOnly 1 issue needs review and one is RTBC
PS: commited #3307699: Update breadcrumbs in user and views_ui help topics to use help_route_link function
Comment #79
andypostCommited #3307698: Update breadcrumbs in statistics and system help topics to use help_route_link function and the last one left #3307695: Update sentence-embedded internal links in content_moderation, core.content_structure, search.overview help topics to use help_route_link function
Closing meta
Comment #81
andypostLast one in!