Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 |
---|---|---|---|
#28 | drupal8update-help-text-2091431-28.patch | 7.2 KB | ifrik |
#28 | interdiff-25-28.txt | 1.74 KB | ifrik |
#25 | drupal8update-help-text-2091431-25.patch | 7.21 KB | ifrik |
#25 | interdiff-21-25.txt | 6.5 KB | ifrik |
#21 | drupal8update-help-text-2091431-21.patch | 7.04 KB | ifrik |
Comments
Comment #1
wzoom CreditAttribution: wzoom commentedTaking it... DrupalCon Prague
Comment #2
wzoom CreditAttribution: wzoom commentedFixed url() -> Drupal::url().
Also changed mention about access rights (near FTP/SSH).
I think the "Install new theme" link is missing from the top of the themes page (/admin/appearance/install). But it is actually mentioned in the Help. Either there should be the button (which is not), or the sentence should be fixed (so that the button is not mentioned there).
But I don't know what is the right solution for that. Could you help me?
Comment #3
jhodgdonI do not know what the right answer is. Probably we should change the component here to the update module and hopefully the maintainers can answer the question.
Comment #4
batigolixa) should we remove the second reference to the on line docs in the 2nd point of the Uses section?
This reference is already present in the about section conform the guidelines.
b) In my local D8 version the Install new theme buttons is also missing. I think it is because of this bug #2102357: “Install new theme” action link appears on /admin, which seems almost fixed. So we can assume that this link/button will be back in the near future.
I verified that all links work
Comment #5
wzoom CreditAttribution: wzoom commentedOk, I removed the 2nd notice about online docs.
Comment #6
batigolixLooks all fine to me.
Let's wait for confirmation from the maintainers about the "Install new theme" link.
Comment #7
jhodgdonThanks for the patches! There are some more things that need to be fixed in this help text.
a) The links to the online docs are still not following our standard template, and all links to drupal.org should be changed to https.
b) We also need to think generally about link text accessibility in this help text. Link text should tell someone what page they are linking to. So for instance, we could expand the link to the available updates report so that "report of available updates" is the link text, and that would give people more information about the page. This is important because people using screen readers often navigate a page by jumping from link to link, and the screen reader reads the link text and/or link title attribute. If the link text is cryptic, they have to try to back up and read the sentence to figure out what the link is.
Another example: there's a link whose text is just "cron". That link probably needs a title attribute to tell people where they'll go if they click it.
An example of good link text is "Update Manager settings", which tells you it is going to the update manager settings page.
c) We need to make sure what is in the help matches what you see in the Drupal UI. For instance, the Modules and Themes pages are actually called Extend and Appearance, so that is how the help text should refer to them.
Comment #8
batigolixPatch addresses all issues from #7.
I removed the link from cron, as it was only there as an explanation of the term "cron" and I do not think it is necessary to add some phrase like "for more information about cron, consult the Cron documentation"
I'm not so happy with the term "the Extend page" for referring to the modules list
Comment #9
jhodgdonI don't like "Extend page" either, but that is what it is called, so we need to call it that in the help.
Anyway... I think this still needs some work:
a) Technically you cannot actually disable modules from the Extend page any more. All you can do is uninstall them from the Uninstall tab of the Extend page. So that needs to be changed in the help.
Actually though... Isn't it kind of obvious that if you do not want the functionality a module is providing, you should disable/uninstall it? Maybe we don't need this at all?
b) A little nitpick: I think we should say that you can change settings "on" a page, not "at" a page.
c) "report of available updates" -- again, can we make sure the help text mirrors the UI text? This comment still applies all over the help. We want everwhere you link to an admin page or mention an admin page, for the help to use the actual page title. It is confusing if you call it something other than what someone would see in the UI.
d) When I go to the Update page, what I'm actually seeing is the ability to install a new module/theme, not a list of updates as it says in this help.
I think we should actually postpone this issue until the UI of update module is cleaned up. It's a mess right now. See
#2121725: UI badness in update.module (menus, tabs, etc.)
and probably other issues.
Comment #10
InternetDevels CreditAttribution: InternetDevels commented@jhodgdon, thanks for recommendation.
a) Added.
b) Done.
c) Renamed to "available updates".
d) Fixed in the https://drupal.org/node/2121725 issue.
Comment #11
jhodgdonWe need to postpone this issue until the UI for the Update Manager module is fixed, because there is no way to be sure what the help should say until it is settled.
Comment #12
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.
Comment #13
batigolixComment #14
batigolixThis actual change is covered in the issue #2294129: Switch hook_help() to use RouteMatch instead of Request
I ll try re-doing the changes included in the path from #10 without attempting a re-roll.
Comment #15
batigolixThis patch incorporates the changes from #10.
I adapted the changes manually without doing a re-roll.
Comment #16
jhodgdonWe still need to postpone this issue on #2121725: UI badness in update.module (menus, tabs, etc.), which is attempting to fix the UI of the Update module so it actually makes sense. No point in documenting a UI that doesn't really make sense.
Comment #17
jhodgdonInstead of having one critical parent issue I have been asked to change the status of each child issue.
This doesn't seem to be Major or Critical to me, and it still needs to be postponed on that other issue (it is pretty much impossible to document a UI that doesn't make sense).
Comment #18
ifrik#2121725: UI badness in update.module (menus, tabs, etc.) has been closed as duplicate of #2362205: admin/theme/install|update should point to admin/appearance/update|install which is now closed, so this issue can now be worked on.
I'm taking it during DevDays.
Comment #19
ifrikUpdated the text, and changed the links into arrays.
Comment #20
jhodgdonThis looks pretty good! A few questions/comments:
a) Through recent work in the D8MI initiative, it has come to my attention that the Update Manager module is also used by other modules that gather information on the web. For instance, the Interface Translation module will download translations for your installed modules when you install a new language or a new module, but this only works if you have the Update Manager installed. Also it will check for updated translations, also using the Update Manager module, when other updates are scanned.
So I think we should mention this somehow... Right now the beginning of this help says:
The Update Manager module periodically checks for new versions of your site's software (including contributed modules and themes) ...
so I think maybe we should say something like:
The Update Manager module periodically checks for new versions of your site's software (including contributed modules and themes) ... The Update Manager system is also used by some other modules to manage updates and downloads; for example, the Interface Translation module uses the Update Manager to download translations of user interfaces from the localization server.
Then maybe when it says:
If desired, you may disable the Update Manager module from the Extend page.
We should add something like:
... ; if you do so, functionality that depends on the Update Manager system will not work.
b)
"The Available updates report displays core, core modules and contrib modules ..."
===> Needs serial comma before "and" here.
===> contrib => contributed
Also doesn't it also report on themes, and does it really separate out core and core modules?
c)
"You can configure the frequency with which checks for updates are performed during cron runs ..."
===> this seems awkward to me. Maybe:
You can configure the frequency of update checks, which are performed during cron runs,
d)
"At the top of the Extend page and the Appearance page you will see a link to update to new releases. ..."
===> ... you will see links to update...
e)
" This will direct you to the Update page where all available updates are listed,"
===> There are two update pages, one for modules and one for themes? So this is not quite right and I don't think it should be linked directly to the Update page for modules?
f) Similarly, the following section about installing new modules and themes... I do not see this link, for one thing, and again there are two separate ones for modules and themes.
Comment #21
ifrikAs far as I see it, the "Update" links on both the Extend and Appearance page link to the same page giving updates - but with two different pathes.
Comment #22
ifrikComment #23
ifrikComment #24
jhodgdonLooking good! A few typos/comments:
a)
"the Interface Translation module uses the Update Manager to download translation from the localization server," ==> download translations
b)
"For example, the Interface Translation module uses the Update Manager to download translation from the localization server, and anonymous usage statistics are sent to Drupal.org. "
==> Um. The first part of this sentence doesn't go with the second.
I think we need to say something like:
Note that whenever the Update Manager system is used, anonymous usage statistics are sent to Drupal.org.
c) I also think that the sentence "The Update Manager system is also used by some other modules ..." should be combined with the "For example, " that follows. So it would say "... to manage updates and downloads; for example, ...".
d) Do we normally say "through the administration interface" or "through the administrative interface"? Probably should be administrative? Not sure what is normal in all the help pages.
e)
"are links to update to new releases. This will direct you to the Update page ..."
"This" here is referring to "links". It must be "These" not "This".
f)
"If you haven't got sufficient access rights to your web server..."
In American English, this isn't very good grammar, unlike I think in British English. :) Should be "If you don't have sufficient"
g)
"or by clicking the Install new module/theme link"
Should be "links", because there is one on the Extend page and one on the Appearance page.
Comment #25
ifrikThanks,
I've changed the wording in the About section and rewrote the section about the Update page.
Changed the titles of the second and third use, avoiding the "admin interface" question by simply naming the pages directly.
Comment #26
ifrikComment #27
jhodgdonOops, a couple more things:
a)
"The Available updates report displays core, core modules, contributed modules, and themes for which there are new releases..."
So actually it displays Core, as well as contributed modules and themes. It doesn't display core modules separate from "core". So I think this should say:
... displays core, contributed modules, and contributed themes for which...
b)
"and whether notifications are sent at the Update Manager settings page."
==> on the Update Manager settings page [not at]
The rest looks great to me! I like how you reworded the update/install sections.
Comment #28
ifrikThanks,
I've made the changes according to comment #27
Comment #29
jhodgdonOK, looks good!
Comment #31
xjmThis patch definitely improves the help text; nice work! I find it easier to follow now.
So here (and elsewhere in the patch), we are reverting the PHP 5.4+ array syntax (with
[]
) back toarray()
. This is unnecessary and I personally prefer the square bracket syntax for readability, but either is acceptable while #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 is still under discussion, so this is fine.This issue only improves documentation, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x. Thank you for the patch and thorough reviews!