Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Background:
This issue is part of the task to update the hook_help texts of the Drupal 8 modules:
#1908570: [meta] Update or create hook_help() texts for D8 core modules
Tasks:
- review / write the hook_help text according to help guidelines
Comment | File | Size | Author |
---|---|---|---|
#37 | path-module-hook-help-2091353-37.patch | 1.94 KB | amitgoyal |
#30 | Screen Shot 2014-05-23 at 2.41.29 AM.png | 84.94 KB | gnuget |
#30 | Screen Shot 2014-05-23 at 2.39.57 AM.png | 56.62 KB | gnuget |
#30 | Screen Shot 2014-05-23 at 2.38.35 AM.png | 80.86 KB | gnuget |
#26 | path-module-hook-help-2091353-26.patch | 3.83 KB | mrjmd |
Comments
Comment #1
babruix CreditAttribution: babruix commentedComment #2
babruix CreditAttribution: babruix commentedInitial patch changes tokens and uses \Drupal::url
Not sure about changes in module and path system, should some information be changed?
Comment #4
babruix CreditAttribution: babruix commentedComment #5
jhodgdonSomeone needs to go through the help that is here and see if what it says still matches the Drupal 8 user interface. If you would like to do that before you make a new patch, that would be helpful.
Other changes needed:
a) All drupal.org links should be changed to https.
b) Links are still not using the correct formatting using routes instead of paths. See
https://drupal.org/node/632280#url-note
Thanks! And sorry for not reviewing this sooner -- I'm still behind from the great Prague help sprint.
Comment #6
batigolixPatch fixes a) & b) from #5
The text matches to current UI which seems not have undergone major changes for D8
Setting component to path.module to let maintainers chime in
I'm wondering if the array('fragment' => 'module-path') piece in the permissions link is actually supposed to work in combination with overlay. The resulting url is http://d8.local/#overlay=admin/people/permissions%23module-path
I'm trying this with overlay but the browser does not navigate to this anchor ...
Comment #7
jhodgdonYeah, fragments do not work with the overlay. However, they can be tested by turning off the overlay, and they're helpful for people who don't use the overlay.
Anyway, the patch in #6 looks fine. Can someone give it a manual test to see that:
a) All the links go to where they should be going
b) The names of pages/sections mentioned in the help text match the actual names of the pages in the administrative UI... I guess the only one mentioned is on content editing it says "URL path settings" -- can it be verified that this is still what it says?
Thanks!
Comment #8
jhodgdon6: path-module-hook-help-2091353-4.patch queued for re-testing.
Comment #10
jhodgdon6: path-module-hook-help-2091353-4.patch queued for re-testing.
Comment #11
batigolixNeed test. See #6
Comment #12
jhodgdonI gave this a manual test today and it all looks great except for one conceptual problem.
The "Creating" uses item says that you can create aliases when you create content. This is true for Nodes, but one of the examples it gives is for a user alias, and I am not seeing the ability to create an alias for a user page when I create the user page. With only the core User and Path modules, I think if you wanted to create an alias for user/3 you would need to do it from the alias list, not while you are creating the user.
And the field where you can add a URL alias on a Taxonomy term is not called the same thing. It just says "URL alias" instead of "URL path settings".
So I think this is a bit misleading. I'm not sure what the best way to fix it would be, and I'm not sure why the URL alias is not available for users and looks different for taxonomy terms either. Any comments from the Path maintainers?
Comment #13
batigolixThis patch attempts to fix the problem raised in #12. The style is not very refined but I think it is clear and not misleading anymore. Suggestions for improvement are always welcome.
I don't think the labeling is different. In the case of the Node it is the "vertical tab label" (form element 'type'= '#details' ?) that is named "URL path settings". The actual input field for both taxo term and node are labeled URL alias.
Comment #14
jhodgdonBetter -- I think that is much clearer.
I missed this in the last review, whoops: The link to the online docs needs updating to our standard wording (still says "handbook").
I'm also wondering, since all the field/section names on the taxonomy/node edit pages are in "Sentence case", if the text would be clearer if they were in quotes? Such as
... under the section URL path settings in the field URL alias ...
vs.
... under the section "URL path settings" in the field "URL alias"...
?
Comment #15
batigolixOk. fixed remarks from #14
Comment #16
jhodgdonLooks good to me, thanks!
Needs a manual test:
- Verify that all the links work
- Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.
Comment #17
Juterpillar CreditAttribution: Juterpillar commentedHave tested this on simplytest.me and verified that it passes html validation and spelling/punctuation is error free.
Here's the screenshot with the patch applied.
Comment #18
jhodgdonThanks for testing! Moving back to Docs component so I see it next time I'm doing commits.
Comment #19
jhodgdonA major change to the Path module was just committed:
#1980822: Support any entity with path.module
so I think we should review this help again and make sure it still matches the UI. I'll also click "retest" just in case.
Comment #20
jhodgdon15: path-module-hook-help-2091353-15.patch queued for re-testing.
Comment #21
jhodgdonThe UI for this module may not have changed yet. See #2201051: Convert path.module form alters to a field widget
Comment #22
jhodgdonLet's postpone this until #2201051: Convert path.module form alters to a field widget is finished.
Comment #23
jhodgdonWe just had a change to hook_help, on this issue: #2183113: Update hook_help signature to use route_name instead of path.
Here is the change record: https://drupal.org/node/2250345
This patch will need a reroll for this change.
Also, un-postponing since the issue it was postponed on has been taken care of.
Comment #24
gnugetComment #25
mrjmd CreditAttribution: mrjmd commentedCurrently working on this reroll
Comment #26
mrjmd CreditAttribution: mrjmd commentedRerolled.
Comment #27
mrjmd CreditAttribution: mrjmd commentedComment #28
mrjmd CreditAttribution: mrjmd commentedComment #29
jhodgdonThank you for the reroll!
So since the UI for the Path module changed (see comments above), we need someone to test this help manually again:
- Verify that all the links work
=====> Especially this: Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.
Comment #30
gnugetI just did a my manual review.
This is the list of links with a screenshot of what is displayed when you click on them or where point them if the links display an external site:
- Pathauto: https://drupal.org/project/pathauto
- online documentation for the Path module: https://drupal.org/documentation/modules/path
- URL aliases:
- Create and edit URL aliases:
- list of all aliases:
I have a question, the links which are pointing to an external site (Pathauto, online documentation for the Path module) shouldn't them open in a new tab/window?
Comment #31
jhodgdonWe are not currently opening links from help in other windows.
@gnuget: I could not tell from your review in #30: did you find any problems, or did you think everything was fine? If everything is fine, please set the issue status to "Reviewed and tested by the community". If there is a problem, set it to "needs work" and explain what the problem is. Thanks!
Comment #32
gnugetHi.
I'm not sure why the images aren't displayed in my comment :-|
I didn't find any problems, i will change the status of this issue.
Thank you.
Comment #33
jhodgdonThanks! This is ready to go in then.
RE images - spaces do not work in image names for whatever reason. Bug in the contrib module that does this.
Comment #34
Dries CreditAttribution: Dries commentedWhat is the reason for changing "@aliases" into "!aliases"?
Comment #35
jhodgdonAll of the hook_help() implementations are being updated in this way for URLs. We discussed it a while back on the parent issue. We don't need URLs that are already generated with url() or a similar function/method to be further sanitized by check_plain(), so we're using !name rather than @name to put them in t() and similar methods.
Comment #37
amitgoyal CreditAttribution: amitgoyal commented@jhodgdon - The patch in #26 no longer applies in current core because all the suggested changes are already in core. Somebody has already committed these changes in core.
Although there is one small issue in the core Path module. As per the Help text standard at https://drupal.org/node/632280, the wording and link should be like "For more information, see the online documentation for the
Path module
."
Whereas currently in core, it is like "For more information,
see the online documentation for the Path module
."
Please see the attached patch which simply makes the link on "Path module" wording rather "see the online documentation for the Path module".
Comment #38
jhodgdonNo, this change is incorrect. The edit you made to the help standards page is incorrect and the current Path module text is correct. I will update the standards page.
Comment #39
amitgoyal CreditAttribution: amitgoyal commentedThanks @jhodgdon for the feedback and correcting it!