Review the hook_help for Internal page cache module

Also in the page_cache.info.yml file, the name of the module should be:

Internal Page Cache

This needs to be fixed in the page_cache_help() function in page_cache.module too.

Also the "Uses" bullet point 'Configuring Internal page cache' should not have I but i on internal.

Here is a patch.

The System help only refers to the aggregation of CSS and Java script files. Maybe that use could be rewritten to say that both the aggregation and the internal caching can be configured on the Performance page if the Interal Page Cache module is installed.

As far as I understand, CSS and JS aggregation is still handled by the System module.

In any case the Performance page there needs to check whether the module is enabled, otherwise the site breaks.

system_help() now refers to the Internal Page Cache module in the About section without a link, since this module actually has no UI of its own.

YML looks good.

A few things I think should be fixed in the hook_help:

a) For authenticated users, pages need to be assembled for each user individually, but anonymous users all get the exact same version of each page.

==> I don't think "exact same" is great... and this is a bit .. I don't know, I didn't think it read well. How about:

Because authenticated users have different roles and permissions, the same URL may result in different pages for different users, while all anonymous users have the same permissions, and pages should appear the same for everyone. Therefore, pages are not cached for authenticated users, only for anonymous users.

b) Configuring internal page cache [uses header]
Needs "the" ==> Configuring the internal page cache

c) On the Performance page, you can configure how long browsers and proxies may cache pages, that setting is also respected by the Internal Page Cache module.

This is a comma splice. You either need to put a conjunction like "and" after the comma, or make it a ; instead. I think a ; would be better here.

d) If the cache setting for browsers and proxies is on Performance due to the system module, then maybe we don't want to remove the help about it from the System module help? or maybe we need to mention this?

d) If the cache setting for browsers and proxies is on Performance due to the system module, then maybe we don't want to remove the help about it from the System module help? or maybe we need to mention this?

There is a mention of it the About section of the System module since comment #4, are you OK with that?

I rephrased a), since in my opinion caching has more to do with sessions than roles and permissions.

This particular line is not good to read. It could be something like this.

+++ b/core/modules/page_cache/page_cache.module
@@ -16,18 +16,14 @@ function page_cache_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dd>' . t('Unlike authenticated users, anonymous users usually share the same session and set of permissions. Therefore, the same URL results in the same page for all anonymous users. This is why pages can be cached for anonymous users, whereas they will have to be rebuilt for every authenticated user.') . '</dd>';

$output .= '

';

Mmmh, looks like I did not take ifrik's modifications into account before working on the patch. I will do better by next week, hopefully with a better explanation about caching for anonymous users.

• Reintegrated ifrik's modifications in #7.
• Reintroduced a brief explanation about the Internal Page Cache module in system_help(), following jhodgdon's advice in #12 (d).
• Wrote a simpler explanation about page caching in page_cache_help().

Here's the fresh screenshot > forgot to apply the patch when taking the first screenshot :)

This looks pretty good!

However, the change in the System module is not quite OK. We have a standard that says all Uses headers should be -ing verbs, so changing it from "Managing the cache" to "Performance" is not right. Maybe it should be "Configuring performance" or something like that?

Also in that section
<a href="!bandwith-optimization">aggregate</a>
This is not a good link text for accessibility. The link goes to the Performance page. The link text should be "Performance page". So the text needs rewriting -- it should say something like "On the Performance page, you can ...".

Also in that section, you should make Internal Page Cache module into a link (you'll need to check whether the module is installed using a moduleExists() call, see other hook_help for examples of this).

I am working on the documentation change.

I'm assigned this to myself to work on it.

@jhodgdon.

I think system help page should have links to the areas that what they are talking about it. So I wrote patch that contains links to the. In addition, this patch contain #22 suggestions.

I would reroll it.

@jhodgdon.

I think system help page should have links to the areas that what they are talking about it. So I wrote patch that contains links to the. In addition, this patch contain #22 suggestions.

Please ignore the #25 patch. I upload the wrong patch. Here the correct patch.

I think system help page should have links to the areas that what they are talking about it. So I wrote patch that contains links to the. In addition, this patch contain #22 suggestions.

darol100: the current issue is mainly about the Internal Page Cache module. I think it would be better to discuss this matter in a new one.

@FMB,

So should we remove the links from the System hook_help ? And open it on a different issue ? I guess we probably would have to re-roll this patch ?

Hm. Some changes to the System module help may be fine, such as if you wanted to make a link to the internal page cache module.

But most or all of the changes in this patch are definitely (a) out of scope for this issue and (b) not in line with the standards we have for Help pages. We do not want the headers in the Uses section to be links.

Please reverse these changes, and if there is a change to make to the System module help that is in scope of this issue (documenting the internal page cache), you can make it in this patch.

This looks great, thanks!

+++ b/core/modules/system/system.module
@@ -89,8 +89,8 @@ function system_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dt>' . t('Handling performance') . '</dt>';

My only suggestion is here: I think that this heading should probably be "Configuring for performance" or something like that -- the verb should be "Configuring" not "Handling", since that's our standard way to say "setting up the site" or "managing settings" in docs and UI text (see https://www.drupal.org/node/604342 ).

+++ b/core/modules/system/system.module
@@ -89,8 +89,8 @@ function system_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dd>' . t('On the <a href="!performance-page">perfomance page</a>, the site can be configured to aggregate CSS and JavaScript files, making the total request size smaller. Note that, for small to medium-sized websites, the <a href="!page-cache">Internal Page Cache module</a> should be installed so that pages are efficiently cached and re-used.', array('!performance-page' => \Drupal::url('system.performance_settings'), '!page-cache' => (\Drupal::moduleHandler()->moduleExists('page_cache')) ? \Drupal::url('help.page', array('name' => 'page_cache')) : '#')) . '</dd>';

Here is a typo, it should said performance instead of "perfomance".

Here is an updated version of the patch with #38 and #39 corrections.

Wrong Interdiff... sorry

Good catch, and thanks for reviewing and the new patch darol100!

So... you prompted me to think "Oh no, I missed some typos!" -- I figured I'd better give this a really really careful read-over, and (sorry!) I found three very small things to fix and then I think we're done:

a) In the new line in system.module, the link to "Performance page" is changed to lower-case "performance page". It should be capitalized, since the page is capitalized. That's here:

t('On the <a href="!performance-page">performance page</a>, the site can be configured...

b) punctuation, just after that:

...small to medium-sized websites...

Should be:

small- to medium-sized websites

The reason is that it's "small-sized to medium-sized", presumably...

c) "re-used" should be "reused". No hyphen. That's also in system.module in that same line. Oh, it's also in page_cache.module.

Here is an updated version of the patch with #42 corrections.

Great, thanks! There's still one "re-used" in page_cache.module though:

Pages requested by anonymous users are stored the first time they are requested and then are re-used.

Fix that and I think we're done.

@jhodgdon, sorry about that. Here is the patch with the page_cache changes.

Thanks! Looks good now.

Added beta eval to issue summary (this is just help docs).

Committed 5c30cf2 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.