Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Note: The File and Image module help is soon going to be expecting that the System module help includes an explanation of private vs. public file storage, and how to configure this. When this help text is reviewed/updated, can you make sure file storage is covered?

surendramohan’s picture

Assigned: Unassigned » surendramohan

Self-assigning: Working on it.

surendramohan’s picture

Patch description:
Added help entry for public and private file system.

lostkangaroo’s picture

Status: Active » Needs review
lostkangaroo’s picture

Status: Needs review » Needs work

With a quick glance I noticed that the string tokens were using @ rather then !. These will need to be fixed as well as updating any url calls following the guidelines listed on this page https://drupal.org/node/632280. Once these are fixed I would be happy to do a full review for you.

surendramohan’s picture

Status: Needs work » Needs review
FileSize
13.54 KB

I appreciate your suggestions.
I have updated all sections of hook_help matching D8 coding standards.
Attached patch for your reference.

lostkangaroo’s picture

Status: Needs review » Postponed

There looks to be a blocker for this dealing with module management so marking this postponed until #2035079: [PP-3] Figure out what to do with the install/uninstall modules page can be resolved. This will have an impact on the managing modules section which is not reviewed below.

A few nit picks unfortunately, most of which date back several versions of Drupal.

  1. Two white space errors are present in the patch
  2. In "Configuring private and public file system" a the should start the sentence as it does in all other sections.
  3. In "About" some of the listed items caching, enabling and disabling modules and themes, might be better to combine into a sub list of the longer features list. Such as managing system cache, modules and themes. This will help also to bring the about section into line with the blocking issue above and other recent module management changes.
  4. Caching in several instances might be better replaced with system cache in locations where contexts suggest an object rather then the act of caching. (Merely a suggestion)
  5. In "Performing system maintenance" the text You can set up cron job by visiting Cron configuration page should better define what you are able to do on the cron configuration page.
  6. In "Configuring private and public file system" the first sentence seems to be a touch on the complicated side. I would consider revising using more concise terms and simpler structure.
  7. Need to replace the depreciated module_exists() call with \Drupal::moduleHandler()->moduleExists('update') in the admin/modules case.
  8. A bonus would be to replace the global $base_url used to resolve the update script path.

You certainly picked a good one and for that you are appreciated.

lostkangaroo’s picture

#2109553: Add route to resolve update.php path allows the use of routes now to replace $base_url in resolving update.php links and removes the dependency on using the global.

surendramohan’s picture

Thanks a lot for your valuable feedback and suggestions (which is always appreciated at my end).
I will figure out the changes and update you with the latest patch version shortly.

ifrik’s picture

In the issue queue #2031177: Improve help for file module it was proposed to move a section about public and private files into the system help:

      $output .= '<dt>' . t('Storing files ') . '</dt>';
      $output .= '<dd>' . t('Files that are uploaded through a file field are stored in a directory that is publicly accessible over the web. When public files are listed, direct links to the files are used, and anyone who knows the URL of that file can download it. The public folder is defined in the settings.php file. Consult the <a href="!file-system">File system</a> settings for information about that directory. Files can be stored in a protected folder by setting a <em>Private file system path</em> in the <a href="!file-system">File system</a> settings. Files in the private directory are not accessible directly through the web server. This increases the server load and download time, since the site must resolve the path for each file download request', array('!file-system' => \Drupal::url('system.file_system_settings'))) . '</dd>';
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.

jhodgdon’s picture

Another thing to add to the "this is in system module" help: The System module provides the System Help block, which displays help at the top of admin pages. It also adds this help to system dialogs.

batigolix’s picture

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

Priority: Normal » Major
Status: Postponed » Needs work

I don't think this should be postponed.

Instead of having one critical parent issue I have been asked to change the status of each child issue.

This one seems Major to me. The System module has a lot of stuff that is poorly or not documented right now.

jhodgdon’s picture

Regarding stuff above...
- The Help block is back in the Help module now.
- We still need info about the file system.
- Image toolkit is also part of System module. Suggested help, which I was just about to add to the Image module on #2091337: Update hook_help for Image module but this is really in System:

      $output .= '<dt>' . t('Configuring the image toolkit') . '</dt>';
      $output .= '<dd>' . t('On the <a href="!toolkit">Image toolkit page</a>, you can select and configure the PHP toolkit used to manipulate images. Drupal core provides the GD2 toolkit; other toolkits may be provided by contributed modules.', array('!toolkit' => \Drupal::url('system.image_toolkit_settings')) . '</dd>';
jhodgdon’s picture

Status: Needs work » Needs review
FileSize
15.92 KB

OK... In the interest of getting the hook_help() patches DONE, I decided to make a new patch for this one. The old patch was long outdated, so I pretty much had to start over. No interdiff, sorry.

Some notes:

a) I only worked on the main help topic page. The changes in the rest of the hook_help() [the page help sections] were just to replace @url with !url. So... We should eventually take a look at this page-level help and revise it; however, that is a separate issue. See #2265533: Review page-level help and make sure it's conceptual

b) I tried to make all of the help text as clear and concise as possible, so nearly all of it has been rewritten. I also changed the order of the Uses topics slightly, trying to put the most important ones near the top and put related topics near each other.

c) As a technicality... Most of the functionality of the System module is now that it provides a UI for functionality that is part of Drupal Core. This is reflected in the About section -- it really isn't true any more that the System module provides a lot of base functionality -- mostly it's just the UI for configuring that functionality, as well as the admin/* menu structure (Structure, Configure, etc.).

ifrik’s picture

Status: Needs review » Needs work

The link to the online documentation links to a page with the expected path, but the page is about setting up cron and caching in Drupal 6.
We could either move the content of that site, and use https://www.drupal.org/documentation/modules/system for the help for the system module, or make a different page.

I notice that "Drupal" is used several times. Did I miss a policy change about that?

For the rest all the text looks good. If we decide to move the D6 cron documentation to a different page then this could be RTBC'ed

jhodgdon’s picture

Status: Needs work » Needs review

That is the right link to the online documentation. The page starts:

The system module provides system-wide defaults for running jobs at particular times, storing (caching) web pages to improve efficiency, and performing other essential tasks. The module also keeps track of various preferences you give for how you want your system to behave.

See, it's about the System module... :) it probably needs updating for Drupal 7/8.

I don't know of a policy about not mentioning the word Drupal in the help... do we have one?

gvso’s picture

Patch in #16 seems to be fine for me, RTBC +1

jhodgdon’s picture

Status: Needs review » Needs work

Oh, you're right about not using "Drupal":

"Use 'Site', not 'Drupal'. Referring to Drupal by name complicates distributions and users may not know the site is running on Drupal."
This is from https://www.drupal.org/node/604342

So this needs a new patch.

gvso’s picture

@ifrik @jhodgdon I do understand your point but how can we change that? I don't think that "The site comes with a number of core modules" will sound better than "Drupal comes with a number of core modules".

ifrik’s picture

Unfortunately it is not about what sounds better, but about what works for users.
This help texts are part of core that can - and is - used in different distributions, so therefore some users don't have "a Drupal site" - but what they have in any case will be "your site" or "the system".
We have been consistent about that in other cases as well.
The only times where we used "Drupal" is when it refers to something like "drupal.org" or "Drupal's translation server" because that is still called Drupal, even when you use a different distribution.

jhodgdon’s picture

So, the existing patch says:

Drupal comes with a number of core modules; each module provides a discrete set of features and may be installed or uninstalled depending on the needs of the site.

We can change this to something like this:

Depending on which distribution or installation profile you choose when you install your site, several modules are installed and others are provided but not installed. Each module provides a discrete set of features; modules may be installed or uninstalled depending on the needs of the site.

We'll need to make a similar change in the Themes section too. The current patch says:

Drupal comes packaged with several core themes, and additional contributed themes are available at the ...

We can change this to something like:

Depending on which distribution or installation profile you choose when you install your site, a default theme is installed, and possibly a different theme for administration pages. Other themes are provided but not installed, and additional contributed themes are available...

Thoughts?

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
16.83 KB
15.97 KB

OK. Here's a new patch:
- Rerolled (the last patch didnt' apply). The interdiff is after the reroll.
- Changed "Drupal" references as discussed above, except where they referred to "the Drupal community" or "Drupal.org".
- Updated drupal.org links to be https://www.drupal.org instead of http://drupal.org

Status: Needs review » Needs work

The last submitted patch, 25: 2091363-system-help-25.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
16.84 KB
2.66 KB

Whoops. Route change. Fixing patch.

ifrik’s picture

Status: Needs review » Reviewed & tested by the community

All links work and have the correct labels.
The text reads well.
As far as I see everything that is controlled by the system module is covered.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Excellent work!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed afbd12c on 8.0.x
    Issue #2091363 by jhodgdon, surendramohan, ifrik, lostkangaroo: Update...

Status: Fixed » Closed (fixed)

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