Problem/Motivation

In the Ideas queue, on #2592487: [meta] Help system overhaul, we are proposing adding the currently-sandbox Help Topics module to Core as an Experimental module.

The main new features this adds are:
- Ability for modules, themes, and distributions to distribute arbitrary numbers of help topics (instead of just one and just for modules). This allows topics to be task-based and user-centered rather than organized by module.
- Because help topics are config entities, they can have dependencies (such as one of module A's help topics depending on also having module B installed).
- The help topics can be arranged hierarchically and have cross-links: topics can have a parent topic, plus a list of "related topics", and topic X can also say "I'm related to topic Y" so that if topic Y exists, X will be listed as related on that topic (allowing contrib topics to list themselves on core topics, for example).
- The admin/help page lists top-level topics only (for less clutter).
- Administrators can edit topics and create their own (to make a help system specific to their site).
- Help topics bodies are HTML, with a text format, so they can be edited much like Node module content items.
- Each help topic is broken up into paragraph-sized chunks, and stored as translatable configuration (so they can be translated on localize.drupal.org by paragraph). See also #1885192: Field locales_source.source is not suitable for long texts and huge config objects -- this is not only desirable but necessary -- long topics definitely need to be broken up into smaller strings.
- No PHP knowledge is required to author help topics.
- The existing hook_help() system and Tour module are not replaced by this system, since they complement each other.

Proposed resolution

a) Approve the Plan on #2592487: [meta] Help system overhaul so that this becomes an official Initiative. Note: There are alternative proposals, so we need to finalize that decision.

b) Add the Help Topics module to Core, initially as an Experimental alpha module (this issue). To do that, we need to resolve these issues and get them into the patch here:
- [DONE!] #2942643: Remove the Preview functionality
- #2943919: Make sure route tokens have cacheable metadata
- #2943923: Connect help topic view page to admin page

c) Once this is in Core as an alpha experimental module, we'll open a Meta issue to get this to Beta quality (which will be necessary to keep it as a live Experimental module in a full Drupal release). Sub-issues that we currently have for that phase (which are currently filed against the Sandbox module, but will be moved to Core once we have the module in there and an issue component):
- #2942644: Config export contains CR characters
- #2942645: Translation UI must not allow changing HTML structure
- #2664830: Add search capability
- #2923918: Theme autocomplete is not ordered
- #2943917: Don't use Entity::url()
- #2943920: Use newer API for getting module info
- #2943921: Move body set/get to a unit test

d) To get this module to full release, we will additionally need to do this:
- #2687107: Write some core help topics

e) And there are a couple of issues that we may or may not eventually need:
- #2927346: CKeditor buttons for DL lists
- #2665784: Add a token for images and a way to manage images

Remaining tasks (and how to help)

On this issue, the task is to get the patch approved for this to be an experimental module (see Proposed Resolution for list of issues to be resolved). We are simultaneously updating the sandbox project so that it stays in sync with the patch here. So, you can review/test either the code in the patch here, or the code in the sandbox repository.

For this issue to be completed, the tasks are:

(0) Prerequisite: Agree on whether this is the best approach -- see #2592487: [meta] Help system overhaul

(a) Module tested, including usability testing. Usability demos resulted in a number of issues being fixed. See Proposed Resolution section above for remaining issues that need to be resolved.

(b) Code in patch reviewed. No known issues at this time, but the code needs a final review.

(c) [Done] Choose name for module -- See
#2921120: Change name of the module
The name "Help Topics" chosen on that issue has been incorporated into the patch here.

(d) Patch finalized with feedback from above items
(patch is being kept updated on each commit to the Sandbox repository, so all Fixed issues are reflected in this patch)

(d) Patch committed

We are using individual issues in the sandbox project to track what needs to be done to improve the code and module. Issue list:
https://www.drupal.org/project/issues/2369943
If you review the code or test out the module, please file issues in the sandbox project if you find things that can be improved. Or you can comment here and someone else can turn them into issues.

Testing notes: After applying this patch and installing the Help Topics (help_topics) module, you can go to
admin/help
and see the current (fairly small) list of top-level help topics (there's an issue about writing more: #2687107: Write some core help topics). Click on one, and you'll see related topics. Also, you can try out links within the text of some topics that will take you to related administrative pages, or to other help topics. The other thing to test is the UI for editing and creating new help topics:
admin/config/development/help

User interface changes

A new Experimental module providing configurable help topics is added, along with a few new help topics. It has an administrative UI for authoring and translating help topics.

The only change to existing UIs is that this module adds a section to the admin/help page that lists the top-level topics, and a section to admin/config to reach the admin UI for authoring help.

API changes

No changes to existing APIs.

Defines a new config entity type (for the help topic).

Data model changes

No changes to existing data models.

The new help topic config entity defines a new schema for saving the help topics to config YAML.

People to credit on this patch

This patch was made by the following people, who contributed patches/tests/reviews on either
#2351991: Add a config entity for a configurable, topic-based help system
or on various Sandbox module issues at https://www.drupal.org/project/issues/2369943, or at Usability group meetings, or here on this issue:
alexpott
amateescu
andypost
benjifisher
catch
ckrina
Fabianx
Gábor Hojtsy
gnuget
jhodgdon
jibran
larowlan
MariaDenysyuk
miro_dietiker
Mixologic
sandboxpl
SenthilMohith
tim.plunkett
tstoeckler
vijaycs85
webchick
webflo
yo30
yoroy

CommentFileSizeAuthor
#78 2920309-78.patch140.17 KBjhodgdon
#72 interdiff.txt2.25 KBjhodgdon
#72 2920309-72.patch142.02 KBjhodgdon
#71 2920309-71.patch141.93 KBjhodgdon
#70 2920309-69.patch142.2 KBjhodgdon
#66 2920309-66.patch139.63 KBjhodgdon
#65 2920309-65.patch137.78 KBjhodgdon
#63 2920309-63.patch136.92 KBjhodgdon
#62 2920309-61.patch132.89 KBjhodgdon
#61 2920309-61.patch131.19 KBjhodgdon
#58 2920309-58.patch162.63 KBjhodgdon
#57 2920309-57.patch162.09 KBjhodgdon
#54 2920309-54.patch159.83 KBjhodgdon
#49 2920309-49.patch160.01 KBjhodgdon
#48 2920309-48.patch157.92 KBjhodgdon
#47 2920309-47.patch158.83 KBjhodgdon
#46 2920309-46.patch157.98 KBjhodgdon
#45 2920309-45.patch157.85 KBjhodgdon
#43 2920309-43.patch157.84 KBjhodgdon
#36 2920309-36.patch155.66 KBjhodgdon
#35 2920309-35.patch155.63 KBjhodgdon
#31 2920309-31.patch154.61 KBjhodgdon
#28 2920309-28.patch154.65 KBjhodgdon
#26 2920309-26.patch156.14 KBjhodgdon
#25 2920309-25.patch157.1 KBjhodgdon
#23 2920309-23.patch156.5 KBjhodgdon
#20 2920309-20.patch156.44 KBjhodgdon
#7 2920309-7.patch156.51 KBjhodgdon
#6 interdiff.txt1.89 KBjhodgdon
#6 2920309-6.patch160.92 KBjhodgdon
#2 2920309.patch159.94 KBjhodgdon
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jhodgdon created an issue. See original summary.

jhodgdon’s picture

Status: Active » Needs review
FileSize
159.94 KB

Here's an initial patch. The only difference between this and the existing sandbox module https://www.drupal.org/sandbox/jhodgdon/2369943 is that it's in package Core (Experimental) for the admin/modules page.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Postponed

Updating summary with link to sandbox module. Also marking this Postponed because I think we need to get the Ideas issue to "Approved Plan" before we really proceed on making this happen.

Amber Himes Matz’s picture

Thanks @jhodgdon!

jhodgdon’s picture

I'm looking into the two test fails (which I don't yet understand).

Also to do on this patch is to add to MAINTAINERS.txt.

jhodgdon’s picture

Status: Postponed » Needs review
FileSize
160.92 KB
1.89 KB

It looks like the two test failures are due to (a) not adding an entry in compser.json and (b) needing to re-export the two filter format config YAML files. I think so anyway. Let's try this again... new patch and interdiff. Also includes MAINTAINERS.txt addition.

jhodgdon’s picture

FileSize
156.51 KB
+++ b/core/composer.json
--- /dev/null
+++ b/core/modules/config_help/README.txt

Note for the next patch: this README file is from the sandbox, doesn't belong in a Core module, so this file should be removed.

OK, here's a new patch without the README file. That's the only change so I didn't make an interdiff, and I also set it to Do Not Test because the previous patch is already being tested. Save a few test bot cycles...

jhodgdon’s picture

Status: Needs review » Postponed

OK, we have a path that passes tests! I am going to Postpone this again until we get the Plan issue adopted...

Amber Himes Matz’s picture

Ok, so should I wait to review the patch until our Plan issue is approved?

larowlan’s picture

Just wanted to mention Mobiledoc as a format

https://bustle.github.io/mobiledoc-kit/demo/

https://github.com/bustle/mobiledoc-kit/blob/master/MOBILEDOC.md

Would be better if we used an existing standard.

Would also mean people could use an editor to generate docs

jhodgdon’s picture

I agree a standard format might be better... There are all kinds of formats for documentation, including AsciiDoc (which we're using on the User Guide project)... And they come and go. Is Mobiledoc being used widely? How long has it been around? Is it secure? I had never heard of it.

Also, there is a UI for editing the help, I'll just point out.

Also, remember that we have a requirement that whatever we use be translatable on localize.drupal.org, which means that it needs to be able to be parsed into paragraph-sized chunks that are recognized as translatable strings in the YAML schema... will this work?

Anyway, I tried the Mobiledoc demo... Here's a sample of Mobiledoc output that the editor created:

{ "version": "0.3.1", "atoms": [], "cards": [], "markups": [], "sections": [ [ 1, "h1", [ [ 0, [], 0, "Title goes here" ] ] ], [ 1, "h2", [ [ 0, [], 0, "Section title here" ] ] ], [ 1, "p", [ [ 0, [], 0, "This is a paragraph." ] ] ], [ 3, "ul", [ [ [ 0, [], 0, "A bullet list item" ] ], [ [ 0, [], 0, "another one" ] ] ] ], [ 3, "ol", [ [ [ 0, [], 0, "A numbered list item" ] ], [ [ 0, [], 0, "Second item" ] ], [] ] ], [ 1, "p", [] ] ] }

It is JSON, but I'm not convinced that it's workable as YAML that can be parsed into translatable strings by the POTX extractor and schema-ized... any thoughts?

jhodgdon’s picture

I also took a look at the code and documentation for Mobildoc and it has some other problems:
- Still a "beta" version. I cannot tell if it is stable.
- It has been under development for a couple of years but it's hard to tell if anyone is using it besides the original developer. I would be in favor of adopting a standard, but not in favor of adopting something that isn't really a standard.
- I also don't think the JS code for the editor and UI of this is localized or translatable, from what I can tell... That would make it very difficult to use in Drupal.

So... nice idea, but I think maybe if we want to adopt a project so that we are using a standard, this one isn't the best to do it with?

andypost’s picture

Initial code review - looks very solid

- some places using deprecated entity manager
- entity query should be taken from storage handler now
- some nitpics

  1. +++ b/core/modules/config_help/config_help.module
    @@ -0,0 +1,76 @@
    +      // @todo hook_help() currently expects a string return value. See
    +      // https://www.drupal.org/node/2665672
    +      return \Drupal::service('renderer')->render($build);
    

    Already fixed, CR https://www.drupal.org/node/2669988

  2. +++ b/core/modules/config_help/src/Controller/AutocompleteController.php
    @@ -0,0 +1,228 @@
    +   * @var \Drupal\Core\Entity\EntityManagerInterface
    ...
    +   * @param \Drupal\Core\Entity\EntityManagerInterface
    ...
    +      $container->get('entity.query')->get('help_topic'),
    +      $container->get('entity.manager'),
    ...
    +      $group = $this->helpEntityQuery->orConditionGroup()
    ...
    +      $ids = $this->helpEntityQuery
    ...
    +        $loaded = $this->entityManager->getStorage('help_topic')->loadMultiple($ids);
    

    EM better to replace with entity_type.manager and get storage from it - query also could be retrieved from storage handler https://www.drupal.org/node/2849874

  3. +++ b/core/modules/config_help/src/Entity/HelpTopic.php
    @@ -0,0 +1,331 @@
    + *   id = "help_topic",
    + *   label = @Translation("Help topic"),
    + *   config_prefix = "topic",
    + *   handlers = {
    

    probably it could use route_provider to generate routes

  4. +++ b/core/modules/config_help/src/Entity/HelpTopic.php
    @@ -0,0 +1,331 @@
    +   * @var array
    ...
    +  protected $body;
    

    looks it needs default value - empty array

  5. +++ b/core/modules/config_help/src/Entity/HelpTopic.php
    @@ -0,0 +1,331 @@
    +  public function serialize() {
    ...
    +    return serialize(['_config_entity_serialize' => $data]);
    ...
    +  public function unserialize($serialized) {
    ...
    +    $this->__construct($data['values'], $data['entity_type_id']);
    +    $this->enforceIsNew($data['is_new']);
    

    Probably it needs config_export annotation to define properties to export

  6. +++ b/core/modules/config_help/src/Form/HelpDeleteForm.php
    @@ -0,0 +1,52 @@
    +class HelpDeleteForm extends EntityConfirmFormBase {
    

    There's \Drupal\Core\Entity\EntityDeleteForm

  7. +++ b/core/modules/config_help/src/Form/HelpTopicForm.php
    @@ -0,0 +1,444 @@
    +      $container->get('entity.manager')->getStorage('help_topic'),
    

    ETM

  8. +++ b/core/modules/config_help/src/HelpAccessControlHandler.php
    @@ -0,0 +1,49 @@
    +    if (!$entity->isLocked() && $operation == 'unlock') {
    +          return AccessResult::forbidden();
    ...
    +    if ($entity->isLocked() && $operation == 'lock') {
    +      return AccessResult::forbidden();
    ...
    +    if ($entity->isLocked() && $operation != 'unlock') {
    +      return AccessResult::forbidden();
    

    does it need cache metadata?

  9. +++ b/core/modules/config_help/src/HelpListBuilder.php
    @@ -0,0 +1,102 @@
    +class HelpListBuilder extends ConfigEntityListBuilder {
    

    May use \Drupal\Core\Config\Entity\DraggableListBuilder cos topics sorted by weight

  10. +++ b/core/modules/config_help/src/HelpViewBuilder.php
    @@ -0,0 +1,166 @@
    +      $container->get('entity.manager'),
    ...
    +      $container->get('entity.query'),
    ...
    +        $this->queryFactory->get('help_topic')
    ...
    +      $storage = $this->entityManager->getStorage('help_topic');
    

    ETM & storage will replace both

  11. +++ b/core/modules/config_help/src/TextSectionPluginCollection.php
    @@ -0,0 +1,41 @@
    +    $body = $help_topic->getBody();
    +    if (!empty($body)) {
    

    if ($body) should be enough

andypost’s picture

I filed few issues to sandbox https://www.drupal.org/project/issues/2369943

First patch #2920479: Get rid of rendering in hook_help

PS: Would be great to enable automated testing for sandbox to make sure nothing broken

Amber Himes Matz’s picture

Thanks @andypost for the reviews and new issues opened in the sandbox! I plan to take a closer look later today, unless someone beats me to it.

jhodgdon’s picture

THANK YOU andypost for the review and issues too! I will review them all tomorrow (was busy today), commit things to the sandbox, and make a new patch here. What a great thing it is to finally have some people interested and helping on this!!

jhodgdon’s picture

Oh, and I don't think you can enable automated testing on a sandbox module unfortunately. At least, there is no Automated Testing tab when I visit the project page like there is on my full contrib projects. :(

jhodgdon’s picture

Amber: Sorry! I got so excited about having actual code/patch reviews! I don't want to step on toes or deprive you of the opportunity to get into maintainership -- let me know (perhaps by assigning issue to yourself) if there are some of the patches/issues you'd like to review and/or make patches for. :)

Amber Himes Matz’s picture

Jennifer: Not at all! I'm very glad you're able to jump in. I'm very much in watch-and-learn mode but will jump in and assign tickets to myself where I can. Thanks!

jhodgdon’s picture

Issue summary: View changes
FileSize
156.44 KB

OK, here we go! What I'm going to do is try to keep the sandbox and the patches here in sync. So, the first thing I did is commit the changes from comment #6 here to the sandbox.

Going forward, I'll follow andypost's very good idea, and make sure each change to this patch here is made into an issue on the sandbox module. Then we can discuss/patch/review/fix there, and when each fix is done, update the patch here to keep things in sync and also run the automated tests in Core to make sure nothing was broken. Too bad sandboxes cannot have test bot running, oh well, but there are only 5 tests for config_help, and they take just a few minutes total to run, so it's not too hard to run them manually.

So, to run the tests manually... Assuming you are using this core patch and not the sandbox module, and you have PHPUnit set up to run as outlined on https://www.drupal.org/docs/8/phpunit/running-phpunit-tests -- and see also #2867042: Running any tests which extended from BrowserTestBase getting permission denied because at least I ran into this problem... Anyway, you can run these 5 commands to run the 5 tests, from the core directory of your Drupal install:

../vendor/bin/phpunit modules/config_help/tests/src/Functional/HelpTopicAdminTest.php
../vendor/bin/phpunit modules/config_help/tests/src/Functional/HelpTopicTranslateTest.php
../vendor/bin/phpunit modules/config_help/tests/src/Functional/HelpTopicTest.php
../vendor/bin/phpunit modules/config_help/tests/src/Kernel/HelpTopicTokensTest.php
../vendor/bin/phpunit modules/config_help/tests/src/Kernel/TextSectionRenderingTest.php

Anyway. All that said, here's the first Core patch update based on this strategy. The interdiff is the patch from #2920479: Get rid of rendering in hook_help -- https://www.drupal.org/files/issues/2920479-2.patch -- which I am also committing to the sandbox module. The tests passed for me locally.

Also I'm making a list of people to make sure to credit for the patch on this issue -- added to the bottom of the issue summary -- anyone who contributed patches or reviews on the sandbox or the original Core issue when I first tried to get this into Core. If I've missed someone, please remind me!

jhodgdon’s picture

Status: Postponed » Needs review

Having this in Postponed status is annoying, tests don't run, so I'm leaving it at Needs Review.

jhodgdon’s picture

I am creating some more issues in the Sandbox module for the review in #13... will mark each of them as having this issue as Related...
https://www.drupal.org/project/issues/2369943

jhodgdon’s picture

FileSize
156.5 KB

I think everything in the review in #13 now has an issue in the Sandbox issue queue.

And I've taken care of the coding standards stuff on #2920498-6: Fix coding standards issues. I decided just to commit it to the sandbox, as it's all pretty straightforward, and tests pass locally.

See patch there https://www.drupal.org/files/issues/2920498-6.patch for interdiff. Here's a new core patch with these changes. We can look at the testbot output and see if there are any remaining coding standards fixes needed after the tests run here...

jhodgdon’s picture

@andypost -- if you didn't see it already, can you take a look at
#2920847-2: Access denied returns need cache metadata
Pertains to one of your review items in comment #13. Thanks!

jhodgdon’s picture

FileSize
157.1 KB

So far so good on the coding standards fixes.

Here's another issue that I've patched on the Sandbox:
#2920839: See if we can use a route provider
The patch is on comment #3: https://www.drupal.org/files/issues/2920839-4.patch

which serves as the interdiff for this new Core patch.

jhodgdon’s picture

FileSize
156.14 KB

Next step: eliminate the custom delete form on
#2920843: See if we can use EntityDeleteForm instead of HelpDeleteForm
Patch is on comment #3 -- https://www.drupal.org/files/issues/2920843-3.patch -- also serves as interdiff for this patch.

jhodgdon’s picture

So @andypost... thanks again for your review above! I've fixed several of the issues.

I could use some advice/reviews on
#2920841: Fix serialization of help topics
#2920847: Access denied returns need cache metadata
Thanks!

jhodgdon’s picture

Issue summary: View changes
FileSize
154.65 KB

A few issues on the Sandbox have gotten to RTBC and I've just committed them to the Sandbox, so updating this patch too:

a) #2920841: Fix serialization of help topics -- patch in #8 is https://www.drupal.org/files/issues/2920841-8.patch

b) #2920847: Access denied returns need cache metadata -- patch in #4 is https://www.drupal.org/files/issues/2920847-4.patch

c) #2920839: See if we can use a route provider -- update to move the class to a different namespace, new patch added is in #6 https://www.drupal.org/files/issues/2920839-6.patch

d) #2920498: Fix coding standards issues -- patch in #8 is https://www.drupal.org/files/issues/2920498-8.patch

Also adding amateescu to the credit list in the summary, for help on the above patches.

Phew! Here's the new patch.

Status: Needs review » Needs work

The last submitted patch, 28: 2920309-28.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jhodgdon’s picture

It looks like #2920841: Fix serialization of help topics broke a Core config test. I'll comment there.

There's also a coding standards problem introduced on the same patch. I'll comment there on that as well.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
154.61 KB

I think I have the two problems fixed. Simple patch on #2920841-11: Fix serialization of help topics https://www.drupal.org/files/issues/2920841-11.patch

That's the interdiff, here's the new patch.

webflo’s picture

The module looks great, i could not found much :)

  1. +++ b/core/modules/config_help/config/schema/config_help.schema.yml
    @@ -0,0 +1,41 @@
    +      sequence:
    +        - type: string
    ...
    +      sequence:
    +        - type: string
    ...
    +      sequence:
    +        - type: config_help_text
    

    The format for defining a sequence is already deprecated. See https://www.drupal.org/node/2442603 for more information.

  2. +++ b/core/modules/config_help/config_help.routing.yml
    @@ -0,0 +1,20 @@
    +  path: '/config_help/autocomplete_topic'
    ...
    +  path: '/config_help/autocomplete_module'
    ...
    +  path: '/config_help/autocomplete_theme'
    

    Core paths use hyphen instead of underscore mostly. I know, its not a user-facing url but still its better for consistency.

  3. +++ b/core/modules/config_help/src/Entity/HelpTopic.php
    @@ -0,0 +1,308 @@
    +    $this->set('related', $topics);
    ...
    +    $this->set('list_on', $topics);
    

    ::set returns $this.

jhodgdon’s picture

Issue summary: View changes

Thanks for the review! (adding your name to the contributors list in the issue summary also)

I've created issues for the points you've made in the Sandbox module issue queue (we're doing individual fixes there and then updating this patch once discussion/review/etc. has completed on each issue):
#2921105: Update config schema for deprecated sequence/map format
#2921107: Switch paths to use - instead of _
#2921109: Simplify a couple of lines in HelpTopic

andypost’s picture

I filed #2921120: Change name of the module

IMO we need better naming and probably split functionality between help & help_ui modules

Also module could have integration to core's tour module

jhodgdon’s picture

I'm not sure about integrating with Tour. Tour could definitely link to any help topics that are published, and since you can put links in help topics, and admin pages linked to can have tours, in that sense they are "integrated". What type of integration were you thinking of? Anyway, good idea to have a discussion about the name and whether to separate out the UI for managing topics.

Meanwhile, I'm updating the patch here:

#2921014-6: Add a Delete tab to View/Edit/Translate -- patch is at https://www.drupal.org/files/issues/2921014-6.patch
That's also the interdiff for this patch.

Amber Himes Matz’s picture

You are all amazing! I will also take a look at this latest patch on my flight which I'm about to board.

jhodgdon’s picture

Several people indicated either here or on #2592487: [meta] Help system overhaul that they were interested in helping with this project... Here are some ways you can help!

I would like to encourage everyone following this issue to help us brainstorm a better name for the module on:
#2921120: Change name of the module

Also, anyone looking for a coding project, we still have this one open that no one has assigned to themselves yet:
#2921010: Make a test for export/import

Another way to help would be to try applying the patch here, test out the module, and see what you think. After installing, you can go to
admin/help
and see the current (fairly small) list of help topics (there's an issue about writing more: #2687107: Write some core help topics). And then you can go to
admin/config/development/help
and try out the UI for editing and creating new help topics. File issues in the sandbox project at https://www.drupal.org/project/issues/2369943 if you see things that should/could be improved. (As an added bonus, if you write one of the topics listed in the issue above on writing more topics, you could export your new topic using the core Configuration Manager module, and attach the YML file to that issue so we can add the topic to the module.)

Or, you could review the code in the latest patch here and see if there are things that could be made better. Or look in the sandbox module issue queue https://www.drupal.org/project/issues/2369943 and see if there are patches that need review. There's one right now: #2921013: Replace all deprecated assertions in tests

Thanks!

webchick’s picture

Since this is introducing a major new component, we should get sign-off from one of the framework manager committers, methinks.

This also has major UX implications (in a good way!), so would be great to get a demo on one of the upcoming UX calls with a wider team of designers + UX people. They happen Tuesdays at noon Pacific in the #ux channel on Drupal Slack. (See full list of core meetings at https://calendar.google.com/calendar/embed?src=happypunch.com_eq0e09s0kv...)

jhodgdon’s picture

OK, I should be able to attend next Tuesday's UX meeting. Do I need to do something special to get on the UX meeting agenda, or just show up?

larowlan’s picture

Issue tags: +DrupalSouth 2017

Tagging as there is potential for folks to work on this next week at the DS sprint

jhodgdon’s picture

Issue summary: View changes

That's great @larowlan! I'm updating the issue summary with essentially #38 (how to help), and also made a better list of Remaining Tasks.

jhodgdon’s picture

FileSize
157.84 KB

Status: Needs review » Needs work

The last submitted patch, 43: 2920309-43.patch, failed testing. View results

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
157.85 KB

Whoops. Patch glitch. The interdiff above was what was supposed to be in the patch, but line didn't get into the actual patch. Artifact of doing the patching in two places at once...

--- a/core/modules/config_help/tests/src/Functional/HelpTopicTranslateTest.php
+++ b/core/modules/config_help/tests/src/Functional/HelpTopicTranslateTest.php
@@ -80,7 +80,7 @@ public function testHelpTranslation() {
 
     // Visit the page in English and verify.
     $this->drupalGet('admin/help-topic/help_test');
-    $session->assertSession();
+    $session = $this->assertSession();

Here's a fixed patch.

jhodgdon’s picture

jhodgdon’s picture

FileSize
158.83 KB

Another patch update. andypost fixed #2921150: Add a test for autocompletes, and refactor to use Core classes. Patch (which is interdiff for this issue) is in #30: https://www.drupal.org/files/issues/2921150-30.patch

jhodgdon’s picture

FileSize
157.92 KB

Another patch update: with help from amateescu and andypost, fixed a lingering serialization problem in the HelpTopic entity, by making the body really be a plugin collection instead of sort of being one. Config schema changed in the process.

Patch is on #2922758-21: Fix entity serialization in edit form, but see comments starting on #15 for discussion. Patch file (interdiff for this patch): https://www.drupal.org/files/issues/2922758-fix-collection-21.patch

jhodgdon’s picture

FileSize
160.01 KB

Here's another patch update, from @andypost and myself on #2923139: Move some functionality into the TextSectionPluginCollection. This is just code refactoring, with zero effect on functionality.
The Sandbox patch is on comment #11 and is the interdiff for this patch: https://www.drupal.org/files/issues/2923139-11.patch

xjm’s picture

Nice work here. I think being able to add help through the UI could definitely be valuable.

I have some concerns about the implementation if it's proposed for core. Currently the module basically re-creates markup in a much more verbose, Drupal-specific format in yaml. I'm not totally sure I see the advantage of that over storing it in a more standard form of markup, markdown, or whatever.

Also, has allowing the creation of help through existing content creation workflows been considered? It could be something as simple as a text format. It could be created and edited in the WYSIWYG. Etc.

I know there's already a big patch here from the contributed module, but it'd be good to step back a bit and discuss other ways we might implement this feature.

xjm’s picture

Regarding #50, it might be good to have product manager signoff on this actually being a feature that we want in core before refactoring the module or anything. #2592487: [meta] Help system overhaul doesn't have a product manager's explicit followup from the UX meeting and I can't really watch an hour-long recording to see what they said. :)

yoroy’s picture

Status: Needs review » Needs work
Issue tags: -Needs usability review

We reviewed and discussed this extensively during last weeks UX call.

Making it possible to add topical or project specific help/documentation to a Drupal install is a great idea and will be a welcome addition to core. The idea was already approved, still is :)

But I think that the proposed user interface model for how to create that documentation is unnecessarily complex. I spent the last week thinking about it and I don’t see enough benefits to this particular approach. To recap: the proposed UI follows a pattern similar to that used in the Paragraphs contributed module: add discrete chunks of content to compose a full page.

As I understand it, this “chunking model” was primarily chosen to make it possible to translate help content in bite-sized chunks through localize.drupal.org. While that may come in handy in some cases, this means that for creating the original help content, we would:

  1. Introduce a completely new mode for creating content, there’s no equivalent for it elsewhere in core.
  2. Introduces a lot of friction to the writing process: you have to add a chunk for each individual header, list item, paragraph (an even more fine grained use of “Paragraphs” than Paragraphs itself)
  3. Related: this also forces you to think about the structure of your content first or at least during creation. I would rather let people enter a draft and refine, structure afterwards. The drag-drop functionality mitigates that a bit but:
  4. Add a very clunky user interface: too much UI “chrome” around too little content

All in all, I think the optimisation for bite-sized translatability through localize.drupal.org is not the right trade-off here. Multi-lingual help is definitely something we should support, but I don’t think it should be the leading consideration for choosing the UI for entering the initial content.

I expect that a lot of the value of this module lies in enabling customer project specific documentation. Again, we should support multi-lingual, but how many project specific docs for multi-lingual sites will be managed trough localize.drupal.org?

So in short: what would the version that uses a single text area look like? I think that would be the better place to start. Use an existing, more familiar model for creating content (text area + wysiwyg editor), reducing the amount of code and complexity we’d add to both the core code base and the user experience.

Adding this feature for topical docs is a great and core worthy idea so I hope we can figure out and agree on a simplified approach.

Suggested next steps:

  • Figure out and agree on a simplified approach :)
  • Improve the other UI issues we identified
  • And then it would probably be useful to open a new issue that outlines the plan and the plan only, so that we can focus the discussion on the details of making this land in core
jhodgdon’s picture

Regarding the idea being approved... The ideas issue is: #2592487: [meta] Help system overhaul and it has an outline for the plan there (the issue summary has the plan).

Regarding a simplified UX for entry, and a simplified schema for storage, I think before we do that we should talk to the Internationalization folks and see if they are OK with that idea.... We were told before that it wasn't. I understand the objection in #52, but I don't think we want to make it impossible for the internationalization teams to translate the help topics. The problem is that very long pieces of text require a completely different workflow for translation/editing/approval than short pieces of text (see for example, the User Guide project, which has 10 groups working on translation and it is a SLOW process!).

So this is a conflict between Usability and Internationalization, as far as I can tell... Can we figure out a compromise idea that works for both?

[EDIT: See two comments down -- spun off the UX for entry into its own issue #2926651: Better UI needed for entering body text please discuss there and not here, thanks!]

jhodgdon’s picture

FileSize
159.83 KB

Meanwhile, another issue on the Sandbox has been addressed, so making a new Core patch here to keep things in sync.
#2920478: Get rid of deprecated entity manager & entity query in HelpViewBuilder & HelpListBuilder classes -- patch is on #26 (and is the interdiff for this patch) https://www.drupal.org/files/issues/2920478-evb-only.patch

jhodgdon’s picture

I spun #52 off into its own issue:
#2926651: Better UI needed for entering body text
Meanwhile, I am going to set this issue back to Needs Review so that patches can be tested as we work on fixing the issues in the Sandbox project queue.

Also, it would really be nice if we could get approval for the Idea on #2592487: [meta] Help system overhaul to make it into an official Initiative. Or if that Idea cannot be approved for some reason, or if the issue summary there needs to be redone, could someone please comment there on what the problem is? Thanks!

jhodgdon’s picture

Status: Needs work » Needs review
jhodgdon’s picture

FileSize
162.09 KB

Another patch update: fixed #2921010: Make a test for export/import. Patch is on #5 and that's the interdiff for this patch:
https://www.drupal.org/files/issues/2921010.patch

jhodgdon’s picture

FileSize
162.63 KB

Another patch update: Fixed some usability-team-identified issues with the Add/Edit screen (not including the "chunked" stuff) on #2923560: Improve usability of the edit screen. Patch (which is interdiff for this patch) is on #3 https://www.drupal.org/files/issues/2923560-edit-form-3.patch

jhodgdon’s picture

Does anyone watching this issue have experience writing CKEditor plugins, and want to help in this effort? If so, see
#2927346: CKeditor buttons for DL lists

jhodgdon’s picture

Adding note from andypost on #2592487-73: [meta] Help system overhaul about chunking to the summary.

jhodgdon’s picture

FileSize
131.19 KB

I just made a commit on #2926651: Better UI needed for entering body text in the Sandbox module, so updating the patch here. The interdiff is the patch on comment #20 there:
https://www.drupal.org/files/issues/2926651-tests-pass-20.patch
except that for the Core patch, I'm not including the config_help.install file (that is there for users of the Sandbox module to update to the new config schema in this patch, but since this is not in Core yet, it doesn't need the hook_updateN() yet).

This patch replaces the unwanted "chunking" in the edit UI, but maintains translatability by chunking the body behind the scenes.

jhodgdon’s picture

Issue summary: View changes
FileSize
132.89 KB

New patch! Two interdiffs (commits on the Sandbox module), and two new contributors added to commit message in issue summary:

a) #2923558: Confirm message should include a link to the created/edited topic -- patch is on #16 -- https://www.drupal.org/files/issues/2923558-16.patch

b) #2931587: Fix 3 coding standards problems -- patch is on #7 -- https://www.drupal.org/files/issues/config_help-coding_standards-2369943...

jhodgdon’s picture

jhodgdon’s picture

Issue summary: View changes

I was wondering if we could get this to RTBC before the 8.5 alpha, which is sometime this week... so I took a look at existing issues...

There are two usability issues that are at Needs Review that I think should get into this patch before we get it into Drupal Core:
#2923559: Add a preview button
#2923557: After editing, return to View page if that is where you started
I could just commit these two to the sandbox and update the patch here, but if someone wants to review them, that would probably be nicer.

There's one code issue that would probably block this getting into Core in 8.5.x. This one needs a patch update:
#2932213: Replace drupal_set_message() with Messenger

And finally, we need to decide on a name for the module:
#2921120: Change name of the module
If we're going to change the shortname, we should definitely do it before we get this into Core.

There are a few other issues, but none of them would block this patch, I think. Anyway... I don't think we can make the 8.5 alpha deadline, due to needing to decide on a name. But let's try to get this done soon!

Meanwhile, updating the issue summary with this information.

jhodgdon’s picture

Issue summary: View changes
FileSize
137.78 KB

New patch, takes care of #2923557: After editing, return to View page if that is where you started. Interdiff is patch on #14: https://www.drupal.org/files/issues/2923557-14.patch

Updating summary to remove this as blocker, and add a few more people to contributor list.

jhodgdon’s picture

Issue summary: View changes
FileSize
139.63 KB

New patch, takes care of #2923559: Add a preview button. Interdiff is patch on #9 - https://www.drupal.org/files/issues/2923559-9-with-tests.patch

Updating summary to remove this as blocker.

jhodgdon’s picture

Issue summary: View changes

Updating summary to list follow-up issues for after this patch is hopefully committed.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhodgdon’s picture

Issue summary: View changes

New patch, fixes #2932213: Replace drupal_set_message() with Messenger. Interdiff is the patch on #20 on that issue: https://www.drupal.org/files/issues/2932213-20.patch

Updating issue summary to remove this as a blocker. Only blocker remaining is
#2921120: Change name of the module
which could use some feedback if anyone here is interested in what this module's name should be!

jhodgdon’s picture

FileSize
142.2 KB

Doh, forgot the patch upload.

jhodgdon’s picture

Title: Add experimental module for Configurable Help » Add experimental module for Help Topics
Issue summary: View changes
FileSize
141.93 KB

Here is the first real candidate patch -- with a name change to "Help Topics" from
#2921120: Change name of the module

The interdiff for this patch is essentially the sandbox patch on that module, except there are two files there that are not in the core patch (README and the .install file), and two files in the Core patch that are not in the sandbox patch (MAINTAINERS.txt and composer.json, each of which has a pretty obvious name change in it as compared to the previous patch). I tried making an interdiff file but since every file changed names, it wasn't actually useful.

Updating summary and issue title...

jhodgdon’s picture

FileSize
142.02 KB
2.25 KB

One more patch, to fix 3 coding standards violations...

jhodgdon’s picture

I did another demo of this module at the UX meeting today.
Issues that were identified:
- yoroy was not sure that the preview functionality was useful and/or implemented correctly, so we might want to back that out of this patch.
- When using the HTML editor to edit a topic, and then exporting it via config export, there are some blocks in the body that just contain character 13 (carriage return). This was the first time I had seen this, but it was pervasive.
- Translation editing is problematic. If you change the HTML markup, the translation doesn't work correctly, because behind the scenes, the translation wouldn't correspond to the original (because the stored configuration is broken up into blocks).

I may have forgotten something... I will create issues for these in the Sandbox issue queue at https://www.drupal.org/project/issues/2369943 and update this patch when they're addressed.

yoroy’s picture

Issue tags: +Usability

Thanks again @jhodgdon for the walkthrough.

Indeed, with the new WYSIWYG-in-1-body-field approach to creating the content, there's no need for a separate preview functionality anymore. Lets remove it from the initial patch.

From ux point of view I think we need another close look at the user interface texts. The form section for "show this topic elsewhere/show other topics here" and the dependencies feature are useful but a bit cryptic at first glance. This is fine-tuning though. I still think the feature itself is looking good and a worthwhile addition. I suggest we get it committed as an alpha experimental module in 8.6 sooner rather than later.

jhodgdon’s picture

OK. Here is a list of all the open issues on the Sandbox project, and whether I think they should be pre-commit or post-commit. See if you agree -- hopefully the titles are self-explanatory enough...

Before initial commit (of experimental alpha module):
- #2942643: Remove the Preview functionality

Necessary for beta-quality:
- #2942644: Config export contains CR characters
- #2942645: Translation UI must not allow changing HTML structure
- #2664830: Add search capability
- #2923918: Theme autocomplete is not ordered

Necessary for full release:
- #2687107: Write some core help topics

Maybe eventually (not sure whether or not we need these):
- #2927346: CKeditor buttons for DL lists
- #2665784: Add a token for images and a way to manage images

jhodgdon’s picture

Issue summary: View changes

Well, barring feedback from the Usability or Product Manager groups, I am adding previous comment to the issue summary, and will shortly make a patch without the Preview functionality so we can get to RTBC here hopefully! Also adding a few more credits from the usability group meetings.

jhodgdon’s picture

Issue summary: View changes

forgot one

jhodgdon’s picture

Issue summary: View changes
FileSize
140.17 KB

OK, here's a patch without the Preview functionality. Reviews welcome! Updating summary. The interdiff is the patch on #2942643: Remove the Preview functionality
https://www.drupal.org/files/issues/2942643-remove-preview.patch
which is just the reverse of the patch where we added Preview on #2923559: Add a preview button.

dawehner’s picture

Let me clarify first some general question about this system. I think it is weird that we have to reinvent a documentation system. It is somehow hard to believe that there is not an existing translatable system out there we could leverage. I understand that our features we want are not trivial, so yeah I'll review the patch without thinking about that particular problem in mind.

I really don't want to discourage this module, but I also don't want to hide my thoughts about it :(

  1. +++ b/core/modules/help_topics/config/install/help_topics.topic.config_basic.yml
    @@ -0,0 +1,44 @@
    +  -
    +    text: 'Site name, slogan, and email address'
    +    prefix_tags: '<dl><dt>'
    +    suffix_tags: '</dt>'
    ...
    +    text: 'On the <a href="[route:url:entity.date_format.collection]"><em>Date and time formats</em></a> page, which you can reach in the main <em>Manage</em> administrative menu, by navigating to <em>Configuration</em> &gt; <em>Regional and language</em> &gt; <em>Date and time formats</em>.'
    +    prefix_tags: '<dd>'
    +    suffix_tags: '</dd></dl>'
    

    I'm curious, did you thought about nesting elements?

  2. +++ b/core/modules/help_topics/help_topics.tokens.inc
    @@ -0,0 +1,81 @@
    +        $replacements[$original] = $topic->url('canonical');
    

    I though $entity->url() is discouraged ...

  3. +++ b/core/modules/help_topics/help_topics.tokens.inc
    @@ -0,0 +1,81 @@
    +      try {
    +        $url = Url::fromRoute($route_name)->toString();
    +      }
    

    It feels like we would need to care about cacheable metadata here.

  4. +++ b/core/modules/help_topics/src/Controller/AutocompleteController.php
    @@ -0,0 +1,214 @@
    +    $installed = array_keys($this->moduleHandler->getModuleList());
    +    $info = system_get_info('module');
    +    $modules = [];
    +    foreach ($installed as $extension) {
    +      $modules[$extension] = $info[$extension]['name'];
    +    }
    

    🔧 With \Drupal\Core\Extension\ExtensionList::getAllAvailableInfo we have a nicer api for that now :)

  5. +++ b/core/modules/help_topics/src/Entity/HelpTopic.php
    @@ -0,0 +1,474 @@
    +class HelpTopic extends ConfigEntityBase implements HelpTopicInterface {
    

    As a side owner I disliked having to add migrations, so it is confusing for me to deal with help configuration entities.

    Also see my comment about config entities vs. plugins.

  6. +++ b/core/modules/help_topics/src/Entity/HelpTopic.php
    @@ -0,0 +1,474 @@
    +  public function getBody() {
    ...
    +  public function setBody($body) {
    

    It is a bit confusing that we don't expose the individual paragraphs. It feels like it could be super powerful to have them available without creating them manually again.

  7. +++ b/core/modules/help_topics/src/Entity/HelpTopic.php
    @@ -0,0 +1,474 @@
    +  /**
    +   * Splits a DOM node recursively into chunks.
    +   *
    +   * @param \DOMDocument $dom
    +   *   The DOM document this came from.
    +   * @param \DOMNode $node
    +   *   DOM node to split into chunks.
    +   * @param string $prefix
    +   *   (optional) Prefix tags to put on first chunk.
    +   * @param string $suffix
    +   *   (optional) Suffix tags to put on last chunk.
    +   *
    +   * @return array
    +   *   Array of chunks from $node. Each chunk is an array with elements:
    +   *   - text: Text content of the chunk, which may contain HTML.
    +   *   - prefix_tags: HTML tags that go before the text content.
    +   *   - suffix_tags: HTML tags that go after the text content.
    +   *   The intent is that if you concatenate all the chunks' prefix_tags,
    +   *   text, and suffix_tags, you will end up with the original HTML.
    +   */
    +  protected function chunkDomNode(\DOMDocument $dom, \DOMNode $node, $prefix = '', $suffix = '') {
    

    ❓Is there a need for this domain logic to live inside the configuration? For me this sonds like a nice helper method on some additional class you could directly use here.

  8. +++ b/core/modules/help_topics/src/Form/HelpLockForm.php
    @@ -0,0 +1,94 @@
    +
    +/**
    + * Provides a form for confirming locking of a help topic.
    + */
    +class HelpLockForm extends EntityConfirmFormBase {
    

    Do we need locking of help topics? It feels like this could be just a permission issue for editing these help topics.

  9. +++ b/core/modules/help_topics/src/Form/HelpTopicForm.php
    @@ -0,0 +1,314 @@
    +
    +    // Make sure that the HTML in the body can at least be loaded/parsed.
    +    if (!Html::load($form_state->getValue('body')['value'])) {
    +      $form_state->setErrorByName('body', $this->t('Body HTML is malformed'));
    +    }
    

    Nice test!

  10. +++ b/core/modules/help_topics/tests/src/Functional/HelpTopicTest.php
    @@ -0,0 +1,347 @@
    +   */
    +  public function testTopicBodyGetSet() {
    +    $topic = HelpTopic::create();
    +
    +    $body = '' .
    +      '<p class="testing123" id="something" style="color:red;">A paragraph with attributes</p>' .
    +      '<h2>A heading</h2>' .
    +      '<h3>A sub-heading</h3>' .
    +      '<ul><li>Bullet 1</li><li>Bullet 2</li></ul>' .
    +      '<ol><li>Number 1</li><li>Number 2</li></ol>' .
    +      '<table><thead><tr><th>Col 1</th><th>Col 2</th></tr></thead>' .
    +      '<tbody><tr><td>Data 1</td><td>Data 2</td></tr></tbody></table>' .
    +      '<dl><dt>Item 1</dt><dd>Definition 1</dd><dt>Item 2</dt><dd>Definition 2</dd></dl>';
    +
    +    // Make sure after set/get, the body is unchanged.
    +    $topic->setBody($body);
    +    $body_out = $topic->getBody($body);
    +    $this->assertEqual($body, $body_out);
    +
    +    // Make sure that if we strip out tags, all remaining text is in the 'text'
    +    // parts of the chunked body array.
    +    $body_plain = strip_tags($body);
    +    $body_chunked = $topic->toArray()['body'];
    +    $chunked_text = '';
    +    foreach ($body_chunked as $item) {
    +      $chunked_text .= $item['text'];
    +    }
    +    $this->assertEqual($body_plain, $chunked_text);
    +  }
    

    This stuff feels like a nice unit test

For myself I tried to compare the approach of using config entities to the one outlined in #2820166: Flexible plugin based help system a while ago.

The main new features this adds are:
- Ability for modules, themes, and distributions to distribute arbitrary numbers of help topics (instead of just one and just for modules). This allows topics to be task-based and user-centered rather than organized by module.

N things of a kind is a central part of plugins.

- Because help topics are config entities, they can have dependencies (such as one of module A's help topics depending on also having module B installed).

Plugins can have some sort of simple dependencies. Migrate has some special code to add more complex ones, so plugins would fullfill this featureset.

- The help topics can be arranged hierarchically and have cross-links: topics can have a parent topic, plus a list of "related topics", and topic X can also say "I'm related to topic Y" so that if topic Y exists, X will be listed as related on that topic (allowing contrib topics to list themselves on core topics, for example).

All this information could be put into the twig file/plugin information.

- The admin/help page lists top-level topics only (for less clutter).

That would be enabled by filtering the plugin definitions.

- Administrators can edit topics and create their own (to make a help system specific to their site).

Editing existing ones is not directly supported, but we have experience with exposing twig in userinterfaces (views), so we could do that. I'm honestly a bit confused why adding custom documentation to a website is something most sites using Drupal need. This is obviously something to be thought about by the product team.
On the other hand the plugin system provides you the flexibility to add this feature in. Just like migrate in contrib allows you to add additional migrations (i think by using config entities), such a help UI module could leverage the same power of plugins. For me this situation feels really similar.

- Help topics bodies are HTML, with a text format, so they can be edited much like Node module content items.

- Each help topic is broken up into paragraph-sized chunks, and stored as translatable configuration (so they can be translated on localize.drupal.org by paragraph). See also #1885192: Field locales_source.source is not suitable for long texts and huge config objects -- this is not only desirable but necessary -- long topics definitely need to be broken up into smaller strings.

Using twig files and ```translate``` would allow us to also use the normal localization tools.

- No PHP knowledge is required to author help topics.

Nor is twig.

- The existing hook_help() system and Tour module are not replaced by this system, since they complement each other.

jhodgdon’s picture

Thanks very much for the review! I'll look at it closely sometime soon, but let me just say one thing. Regarding the paragraph chunking (item 6 in your review), this module originally did have the user editing via "chunks", but the Usability team hated it and insisted on the editing screen using a regular HTML editor. However, you cannot have large pieces of HTML stored in config entities that are translatable, so that is why it needs to be chunked for storage. And yes the chunking could be moved into another class, but for now it is on the entity since there is no other user for it at the moment.

Amber Himes Matz’s picture

Issue summary: View changes

Just fixing a typo on the issue summary.

Amber Himes Matz’s picture

@dawehner Re: #79

Thank you so much for taking the time to review this patch.

Regarding,

I'm honestly a bit confused why adding custom documentation to a website is something most sites using Drupal need.

A common best practice of documentation is to put the docs as near to the product as possible. In this case, the website being the product and the help system within the site being the documentation.

Commonly, an agency would create docs using a custom module and hook_help or previously Advanced Help module before handing off the site to the client. Further additions to the help documentation (after hand-off) would then rely on a developer or at least someone familiar enough with the code base and copy-pasting skills to add more help files for their site.

This solution provides a UI that both an agency and a client could utilize to create and maintain up-to-date documentation for their site. This is a much needed feature, especially due to a site's oftentimes unique or custom process of entering content on their site.

The audience here is a marketing person or content editor -- not only the developer. And not necessarily someone with access to the codebase to "just" create new Twig files.

The objective is to make site-specific documentation creation and maintenance possible for the site builder, administrator, or content manager, while also making it possible to export the configuration and move it to another instance of the site.

Amber Himes Matz’s picture

Here are my thoughts on the patch in #78:

1. When editing (or creating) a help topic, the breadcrumb is:

Home > Administration > Configuration > Development > Help topics

After submitting that form and viewing the help topic, the breadcrumb changes to

Home > Administration > Help

This makes sense if navigating to a help topic from the main Help page, however, there doesn't seem to be a clear path for the help topic editor to get to the list of Help Topics at /admin/config/development/help or from a help topic view page.

Basically, for the help topic editor, how can we make it easier to navigate to the list of help topics?

2. Regarding the Help text format, the help text for it says:

You can use tokens like [route:url:ROUTE_NAME] and [help_topic:url:MACHINE_NAME] to insert URLs to administrative page routes and to other help topics into the text.

I experimented with putting route and help_topic token into the body field. While it will output the value of a token as plain text, I couldn't find a way to insert the value of these tokens into a link, which is how they would be the most useful. I tried both inserting the token using the WSYWIG link button as well as using HTML (which was rendered as text not HTML). So the instructions either need to be updated with clearer directions on how to create a link using a token or something else needs to be fixed.

jhodgdon’s picture

Issue summary: View changes

Thanks @Amber Himes Matz and @dawehner (again)!

So, I'm going to make a few comments here to address these points, and then spin some of them off into their own issues in the Sandbox project so we can get them fixed.

First, I'd like to address item #5 in comment #79, plus the discussion at the end, about config entities vs. plugins and #2820166: Flexible plugin based help system. I think that you make some very good points, and maybe we should consider going in that direction... but it is a better discussion to have on the Ideas issue, so let's move that over into #2592487: [meta] Help system overhaul. I've put your comments there, as well as Amber's from #81, and added one of my own -- let's continue the discussion. I've also put a note in the issue summary that we need to finally decide on the approach before we commit this.

Next comment I'll look at the specific code/docs comments.

jhodgdon’s picture

OK, now I want to look at the specific review comments from comment #79 and #83... Moved most of them to their own issues in the Sandbox module:

#79 item 1 -- I do not know what this means, sorry?

#79 item 2 -- you are correct, so I filed #2943917: Don't use Entity::url()

#79 item 3 -- filed #2943919: Make sure route tokens have cacheable metadata.

#79 item 4 -- filed #2943920: Use newer API for getting module info

(item 5 was moved to Ideas queue, and item 6 & 7 I addressed in comment #80)

#79 item 9 -- So far the Usability team has been fine with the lock/unlock operations (there have been several demos)

#79 item 10 -- filed #2943921: Move body set/get to a unit test

#83 item 1 -- filed #2943923: Connect help topic view page to admin page

#83 item 2 -- Hm.... I didn't have any problem with this. In the HTML editor, I did this:
a) Typed in the text I wanted for the link
b) Highlighted the link text
c) Clicked the Link button (chains)
d) In the popup for the link URL, typed in [help_topic:url:config_basic] and clicked Save
e) Saved the topic.

This resulted in a link, which when I clicked it took me to the config_basic topic.

So... what did you try, and how can we revise the body hint so that it will be clearer this is what you need to do?

jhodgdon’s picture

Issue summary: View changes

I have triaged the issues I just created into ones I think are definitely pre-commit and ones that I think can be addressed in the alpha-to-beta phase. Updating issue summary in Proposed Resolution section. Open to other ideas if my triage is incorrect...

Berdir’s picture

Didn't review yet, just a random input on the config entity vs plugin discussion. We have plugin derivates, which can be used to dynamcially expose N plugins based on anything dynamic. A common example where that is used a lot is blocks. Each custom block (which is a content entity) is exposed as a block, on the same level as block plugin classes from modules. And the same is done for each menu, which is a config entity.

So if help pages were plugins, then we could have a config entity too that would expose itself as a plugin. That we, we could have both user-managed help pages as well as module-provided help which would be easy to keep up to date as modules are updated, which I think the current config system can't really do without contrib modules.

Amber Himes Matz’s picture

I dug into what was happening with the tokens a bit more. The tokens are being truncated when the topic is edited. They work the 1st time, but not after an edit. I opened Issue #2943974: Route tokens in Help text format get truncated after editing a help topic.

jhodgdon’s picture

RE #87, I put a note on #2592487: [meta] Help system overhaul about your comment as it should be discussed there I think (or in both places).