Review/fix/delete existing help topics

On #3065148: We need less ambiguous help topic labels we also discussed that topic titles need to be unambiguous for translations. If we follow the standards we have set out here, they will be, but I'm adding a note to the issue summary about it anyway. (That issue has been closed as a duplicate of this one.)

Also getting the latest standards from the parent issue template.

We've migrated the help topic standards to https://www.drupal.org/docs/develop/documenting-your-project/help-topic-... so updating issue summary.

I have some time to work on this in the next few days.

Here is a patch.
- I removed a few topics that were empty. If we have content we can add them then.
- I added some "stub" topics that we need to write.
- I moved topics to more appropriate locations (module directories). Tried to move some to core/help_topics but that didn't work (see bug report).
- I removed the writing good help topic. We have a help topic standards page on drupal.org that tells how to write good topics, but this topic is not really needed by site admins. I added one note to that page that wasn't there from the topic.

On #3076348: Refactor help topic tests so they do not use production topics, we mostly refactored the tests so that they did not depend on production topics. But not the part in the Functional HelpTopicsTest that was testing that if you put topics in the help_topics directory inside core/module/help_topics, and they are intended for modules that are not enabled, the topics are not shown. (That is the test fail from the previous patch.)

To make the test pass, the best thing to do is add the code/test patch that was supposed to be on #3025577: Move help topics to core or the correct modules into this patch. So, here's an interdiff and code change (1-line code change and a few lines removed from a test), added to the patch that was in the previous comment.

updating title/summary

I think probably all of the test fails were due to a discovery exception:

core.ui_components.html.twig file name should begin with 'help_topics'

Due to this code change:

-        // Only the Help Topics module can provide help for other extensions.
-        // @todo https://www.drupal.org/project/drupal/issues/3072312 Remove
-        //   help_topics special case once Help Topics is stable and core
-        //   modules can provide their own help topics.
-        if ($provider !== 'help_topics' && $provider !== $file_name_provider) {
+        if ($provider !== $file_name_provider) {

But this is not currently working due to this related issue.

So I think we need to fix that one first, and this one should be postponed for the moment.

Sorry that was a bit confusing.

We don't want the core help topics in the core/modules/help_topics/help_topics directory. We want them to be under core/help_topics. Which is why that line of code in the comment above was added to the patch in #7.

But due to #3079810: core/help_topics directory does not work, actually moving the help topics to that core directory is also not working. So we need to:
- fix that issue
- move the core topics out of core/modules/help_topics/help_topics and into core/help_topics.

I asked in Slack and found out from @xjm that all of the code *and topics* related to the Help Topics module need to stay under core/modules/help_topics until we are Stable.

So, I reopened #3025577: Move help topics to core or the correct modules (which had been part of this issue), and let's make this issue just about review/fix/delete topics and leave the move until later. As such, it does not need to be postponed. Updating summary.

In light of #17, here is a reroll of the previous patch (#11), which does the following:

a) Moves all the help topics back to core/modules/help_topics/help_topics.

b) Removes the code change from HelpTopicDiscovery and instead leaves it unchanged:

        // Only the Help Topics module can provide help for other extensions.
-        // @todo https://www.drupal.org/project/drupal/issues/3072312 Remove
-        //   help_topics special case once Help Topics is stable and core
-        //   modules can provide their own help topics.
-        if ($provider !== 'help_topics' && $provider !== $file_name_provider) {
+        if ($provider !== $file_name_provider) {

we still need this code, because everything is staying in core/modules/help_topics/help_topics for now.

I haven't made any other changes. I didn't think an interdiff was useful... Let's see what the bot thinks.

OK! That went well. We are down to one test fail:

Topic user.security_account_settings Twig file has all of its text translated

Hm...

That is from this section of the file:

<dl>
  <dt>[site:name]</dt>
  <dd>{% trans %}The name of your web site.{% endtrans %}</dd>
  <dt>[site:url]</dt>
  <dd>{% trans %}The URL of your web site.{% endtrans %}</dd>
...

This is a list of the variables you can use in setting up automated email messages. I actually don't think those should be translated. So maybe we need a way to put exceptions into the syntax checking test? Trying this...

Well, that didn't work. I'll need to get onto the machine I actually use for development and figure this out more. Also probably if I make an addition like this to the syntax test, the test cases should be updated to put in a case like this with not-to-be-translated text to verify it works, and also a Twig comment.

Still need to do #22. Meanwhile, I added a summary of what the patch will do and/or does to the issue summary, because the patch itself is a bit confusing (looks like it is deleting files when it is not necessarily doing so, because they changed names and also got edited).

OK, a few more updates:
- Removed the "Writing good help" topic, as discussed on comment #5 (that removal got reverted by mistake in one of the other patches).
- Fixed some files that failed the syntax tests locally.
- Fixed the syntax test so it actually works for Twig comments and for designating parts of a file to be OK to be untranslated.
- Added these items to the test topics so that the test will continue to work for them hopefully.

Test passes locally now. Probably time for the topics text to be reviewed (assuming the bot agrees)?

I had second thoughts about this patch. I think I will rewrite that one topic so that it doesn't need the "designate parts of a file to be OK to be untranslated" thing, and take that out of this patch.

Here's a new patch, which rewrites one topic and removes the test update that added that functionality (see previous comment). Updating issue summary to reflect this simpler patch. Syntax test still passes locally.

Note: If you want to review this patch (thanks!), check the issue summary for a list of topics that are in this patch. The patch file is itself a bit confusing, with some topics looking like they were deleted/added instead of modified (due to large changes and new file names). Anyway... the thing to do is probably to review all the topics that are visible now on admin/help, and the ones they link to.

Note to potential committers: It would probably be nice to get this backported to 8.8, because it will improve the UX of the (hopefully beta) experimental Help Topics module by making better topics available when people first see it in a release. Hm... Although I think help_topics is cut out of 8.8 at the moment. Not sure where the module that would get put back in will come from... Anyway I'll add a note to the issue summary.

I talked to Amber about this patch today. We decided it would be good to rename the "stub" pages so they start with "Coming soon", to make it clearer to users of Help Topics that this is a work in progress.

Also adding links to the issue summary to the issues where those particular stub topics will be worked on.

We have many examples of using HTML A tags inside translated strings. That should not be a problem.

The interdiffs look great! I do have one concern:
The following Drupal 8 User Guide chapters provide guidance...
Actually, these are not chapters but topics within User Guide chapters... But maybe rather than linking to 5 topics all within the security chapter of the user guide, we should just link to the whole chapter? https://www.drupal.org/docs/user_guide/en/security-chapter.html That way if we add new security topics there, going forward, they would still be included... Seems better than making 5 links to me. Thoughts?

A note about my assertion about A tags within translated strings -- I went to localize.drupal.org and searched for strings containing "https:" in the translation sources. There were 2 pages of results in Drupal Core 8.7.8, so this is definitely the right pattern. Here are a couple of examples:

Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/docs/8/install">installation handbook</a>, or contact your hosting provider.

The Aggregator module requires the <a href="https://secure.php.net/manual/en/curl.setup.php">PHP cURL library</a>. For more information, see the <a href="https://www.drupal.org/requirements/php/curl">online information on installing the PHP cURL extension</a>.

etc.

Actually though, there is a problem. Take a look at #36. I think the endtrans needs to be outside the end of the A tag, not inside the A tag! Either that or the start trans tag needs to be inside the A tag, so it is only wrapping the link text.

#41 looks good to me. +1 for RTBC and for getting this backported to 8.8 if possible.

+1 for a follow-up issue that tests that the translated strings have valid HTML (and for a test case that doesn't, or that has a missing closing tag) to make sure that the test is valid. And I think regarding #40/42 what we should do is add a test that verifies that strings within the Twig trans tags pass locale_string_is_safe() [which is the same function in D7 and D8].

So, for these two things, I thought 1 issue was probably enough, and I made:
#3090257: Add additional tests for help topic syntax
and added this to the Roadmap issue.

Good points!

So I think we should revise the hierarchy so the Coming Soon pages are not top level -- just put them somewhere. We already have another issue about fixing up the hierarchy at the end, so we can do it as we go and/or review the topics at the end and not worry about it too much right now. #2687107: Reorganize topics into sensible outline, and/or write more topics

Regarding the links... We definitely want (I think?) the navigation to be consistent, and this format mirrors what we're doing in the User Guide -- but there we can't have links (it's not within a Drupal site). So... How about if we change the standards for Help Topics so that the final item in the navigation should be a link? Let's try that -- if we like it, we can update the standards.

I'm on the wrong machine here, so I'll add this comment and then make a patch and some screenshots, most likely before you two wake up (insomnia to the rescue!). :)

OK, here's the new patch.

I decided you're right, we probably don't need to have the stub pages there at all. There were no in-topic links, just thinking "We are talking about Blocks here, so we need to link to the eventual Blocks overview page". So... I went ahead and removed them.

The admin/help Help Topics section now looks like this:

I also added links to the navigation, where possible. So the basic site settings topic now looks like this:

As a note, as far as I know we don't have a viable way right now to make a link that is contingent on a module (and its routes) being defined, the way we did in hook_help [at least I don't think so? Do we need an issue?]. So I couldn't make every navigation thing into a link. But I put in what I could.

If we decide we like this... we should change our Help Topic Standards to say to make the last part of a navigation description into a link, but only if the module that provides the link is a dependency of the module the topic is in. Thoughts?

Doh! I thought I had clicked all the links, but obviously not! +1 for careful testing -- you rock!

I created an issue about the conditional URLs, and added it to the Roadmap:
#3090659: Make a way for help topics to generate links only if they work and are accessible

I've also updated the Help Topic Standards to say the last element of the navigation should be a link if possible. If we decide that is no good, we can always revert the change.

I am +1 for setting this back to RTBC.