Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wzoom’s picture

Taking it... DrupalCon Prague

wzoom’s picture

Status: Active » Needs review
FileSize
6.54 KB

Fixed 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?

jhodgdon’s picture

Component: documentation » update.module

I 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.

batigolix’s picture

Status: Needs review » Needs work

a) should we remove the second reference to the on line docs in the 2nd point of the Uses section?

More detailed instructions can be found in the online documentation for Update Manager module.

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

wzoom’s picture

Status: Needs work » Needs review
FileSize
6.37 KB
6.37 KB

Ok, I removed the 2nd notice about online docs.

batigolix’s picture

Title: Update hook_help for Update module » Update hook_help for Update Manager module

Looks all fine to me.

Let's wait for confirmation from the maintainers about the "Install new theme" link.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks 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.

batigolix’s picture

Status: Needs work » Needs review
FileSize
6.33 KB

Patch 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

jhodgdon’s picture

Status: Needs review » Postponed

I 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.

InternetDevels’s picture

@jhodgdon, thanks for recommendation.
a) Added.
b) Done.
c) Renamed to "available updates".
d) Fixed in the https://drupal.org/node/2121725 issue.

jhodgdon’s picture

Status: Needs review » Postponed

We 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.

jhodgdon’s picture

We 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.

batigolix’s picture

Issue tags: +Documentation, +sprint
batigolix’s picture

This 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.

batigolix’s picture

Status: Postponed » Needs review
FileSize
6.44 KB

This patch incorporates the changes from #10.

I adapted the changes manually without doing a re-roll.

jhodgdon’s picture

Status: Needs review » Postponed

We 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.

jhodgdon’s picture

Instead 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).

ifrik’s picture

Status: Postponed » Needs work

#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.

ifrik’s picture

Status: Needs work » Needs review
FileSize
6.78 KB
6.63 KB

Updated the text, and changed the links into arrays.

jhodgdon’s picture

Status: Needs review » Needs work

This 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.

ifrik’s picture

  • Added the information about other modules using Update Manager to the about section
  • Fixed wording and commas

As 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.

ifrik’s picture

Status: Needs work » Needs review
ifrik’s picture

Issue tags: +drupaldevdays
jhodgdon’s picture

Status: Needs review » Needs work

Looking 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.

ifrik’s picture

Thanks,

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.

ifrik’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Oops, 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.

ifrik’s picture

Status: Needs work » Needs review
FileSize
1.74 KB
7.2 KB

Thanks,
I've made the changes according to comment #27

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK, looks good!

  • xjm committed 4f783ea on 8.0.x
    Issue #2091431 by ifrik, wzoom, batigolix, InternetDevels, jhodgdon:...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

This patch definitely improves the help text; nice work! I find it easier to follow now.

+++ b/core/modules/update/update.module
@@ -73,37 +73,21 @@ function update_help($route_name, RouteMatchInterface $route_match) {
array('!update' => 'http://drupal.org/documentation/modules/update', '!modules' => \Drupal::url('system.modules_list'))...

So here (and elsewhere in the patch), we are reverting the PHP 5.4+ array syntax (with []) back to array(). 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!

Status: Fixed » Closed (fixed)

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