Add experimental module for Help Topics

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

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 1 November 2017 at 21:12

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

Comment #10

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 1 November 2017 at 21:33

Just wanted to mention Mobiledoc as a format

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

Comment #11

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 November 2017 at 23:19

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?

Comment #12

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 November 2017 at 23:31

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?

Comment #13

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 2 November 2017 at 13:11

2 files were hidden/shown/deleted

File	Size
2920309-6.patch	160.92 KB
8.5.x: PHP 7 & MySQL 5.5 22,503 pass
interdiff.txt	1.89 KB

Initial code review - looks very solid

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

+++ 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

+++ 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

+++ 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

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

looks it needs default value - empty array

+++ 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

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

There's \Drupal\Core\Entity\EntityDeleteForm

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

ETM

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

+++ 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

+++ 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

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

if ($body) should be enough

Comment #14

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 2 November 2017 at 13:56

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

Comment #15

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 2 November 2017 at 19:41

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.

Comment #16

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 November 2017 at 23:59

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!!

Comment #17

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 November 2017 at 00:00

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. :(

Comment #18

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 November 2017 at 02:22

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

Comment #19

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 3 November 2017 at 03:58

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!

Comment #20

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 November 2017 at 15:42

Issue summary:

File	Size
2920309-20.patch	156.44 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

1 file was hidden/shown/deleted

File	Size
2920309-7.patch	156.51 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!

Comment #22

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 November 2017 at 15:52

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

Comment #23

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 November 2017 at 17:01

File	Size
2920309-23.patch	156.5 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

1 file was hidden/shown/deleted

File	Size
2920309-20.patch	156.44 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

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

Comment #24

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 November 2017 at 17:23

@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!

Comment #25

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 November 2017 at 20:53

File	Size
2920309-25.patch	157.1 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

1 file was hidden/shown/deleted

File	Size
2920309-23.patch	156.5 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

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.

Comment #26

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 November 2017 at 21:18

File	Size
2920309-26.patch	156.14 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

1 file was hidden/shown/deleted

File	Size
2920309-25.patch	157.1 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

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.

Comment #27

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 November 2017 at 21:54

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!

Comment #28

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 4 November 2017 at 14:37

Issue summary:

File	Size
2920309-28.patch	154.65 KB
8.5.x: PHP 7 & MySQL 5.5 22,430 pass, 2 fail

1 file was hidden/shown/deleted

File	Size
2920309-26.patch	156.14 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

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.

Comment #30

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 4 November 2017 at 15:50

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.

Comment #31

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 4 November 2017 at 16:36

Status:

Needs work

» Needs review

File	Size
2920309-31.patch	154.61 KB
8.5.x: PHP 7 & MySQL 5.5 22,505 pass

1 file was hidden/shown/deleted

File	Size
2920309-28.patch	154.65 KB
8.5.x: PHP 7 & MySQL 5.5 22,430 pass, 2 fail

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.

Comment #32

webflo CreditAttribution: webflo at UEBERBIT GmbH commented 5 November 2017 at 07:43

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

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

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

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

Comment #33

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 5 November 2017 at 15:32

Issue summary:

+Needs framework manager review, +Needs usability review

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

Comment #34

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 5 November 2017 at 17:11

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

Comment #35

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 5 November 2017 at 21:32

File	Size
2920309-35.patch	155.63 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

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.

Comment #36

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 5 November 2017 at 22:37

File	Size
2920309-36.patch	155.66 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

2 files were hidden/shown/deleted

File	Size
2920309-31.patch	154.61 KB
8.5.x: PHP 7 & MySQL 5.5 22,505 pass
2920309-35.patch	155.63 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

More patch updates -- several fairly small changes from andypost (THANK YOU!!):

#2921107: Switch paths to use - instead of _ patch is in comment #4 -- https://www.drupal.org/files/issues/2921107-4.patch
#2921109: Simplify a couple of lines in HelpTopic patch is in comment #4 -- https://www.drupal.org/files/issues/2921109-4.patch
#2921105: Update config schema for deprecated sequence/map format patch is in comment #4 -- https://www.drupal.org/files/issues/2921105-4.patch
#2921148: Replace string reference to classes patch is in comment #2 -- https://www.drupal.org/files/issues/2921148-2.patch
#2372585: Change order of tabs to View / Edit / Translate patch is in comment #12 -- https://www.drupal.org/files/issues/2372585-11.patch

Comment #38

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 7 November 2017 at 15:14

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: Reorganize topics into sensible outline, and/or write more 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!

Comment #39

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick at Acquia commented 9 November 2017 at 20:13

Issue tags:

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

Comment #40

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 9 November 2017 at 21:05

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?

Comment #41

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 9 November 2017 at 21:48

Issue tags:

+DrupalSouth 2017

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

Comment #42

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 9 November 2017 at 22:12

Issue summary:

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.

Comment #43

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 9 November 2017 at 22:18

File	Size
2920309-43.patch	157.84 KB
8.5.x: PHP 7 & MySQL 5.5 22,541 pass, 2 fail

1 file was hidden/shown/deleted

File	Size
2920309-36.patch	155.66 KB
8.5.x: PHP 7 & MySQL 5.5 22,506 pass

Meanwhile, here's another patch update, for #2921013: Replace all deprecated assertions in tests -- interdiff is https://www.drupal.org/files/issues/2921013-7.patch

Comment #45

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 10 November 2017 at 15:25

Status:

Needs work

» Needs review

File	Size
2920309-45.patch	157.85 KB
8.5.x: PHP 7 & MySQL 5.5 22,542 pass

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.

Comment #46

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 November 2017 at 22:39

File	Size
2920309-46.patch	157.98 KB
8.5.x: PHP 7 & MySQL 5.5 22,542 pass

2 files were hidden/shown/deleted

File	Size
2920309-43.patch	157.84 KB
8.5.x: PHP 7 & MySQL 5.5 22,541 pass, 2 fail
2920309-45.patch	157.85 KB
8.5.x: PHP 7 & MySQL 5.5 22,542 pass

New patch, due to #2922774: Update URL patterns to match config standard -- interdiff/patch is: https://www.drupal.org/files/issues/2922774-5.patch

Comment #47

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 12 November 2017 at 17:27

File	Size
2920309-47.patch	158.83 KB
8.5.x: PHP 7 & MySQL 5.5 22,543 pass

1 file was hidden/shown/deleted

File	Size
2920309-46.patch	157.98 KB
8.5.x: PHP 7 & MySQL 5.5 22,542 pass

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

Comment #48

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 13 November 2017 at 18:16

File	Size
2920309-48.patch	157.92 KB
8.5.x: PHP 7 & MySQL 5.5 22,549 pass

1 file was hidden/shown/deleted

File	Size
2920309-47.patch	158.83 KB
8.5.x: PHP 7 & MySQL 5.5 22,543 pass

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

Comment #49

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 15 November 2017 at 23:07

File	Size
2920309-49.patch	160.01 KB
8.5.x: PHP 7 & MySQL 5.5 22,556 pass

1 file was hidden/shown/deleted

File	Size
2920309-48.patch	157.92 KB
8.5.x: PHP 7 & MySQL 5.5 22,549 pass

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

Comment #50

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 19 November 2017 at 11:01

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.

Comment #51

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 19 November 2017 at 20:11

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

Comment #52

yoroy CreditAttribution: yoroy at Roy Scholten commented 21 November 2017 at 16:56

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:

Introduce a completely new mode for creating content, there’s no equivalent for it elsewhere in core.
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)
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:
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

Comment #53

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 27 November 2017 at 16:22

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!]

Comment #54

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 27 November 2017 at 17:16

File	Size
2920309-54.patch	159.83 KB
8.5.x: PHP 7 & MySQL 5.5 22,906 pass

1 file was hidden/shown/deleted

File	Size
2920309-49.patch	160.01 KB
8.5.x: PHP 7 & MySQL 5.5 22,556 pass

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

Comment #55

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 27 November 2017 at 23:37

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!

Comment #57

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 28 November 2017 at 20:18

File	Size
2920309-57.patch	162.09 KB
8.5.x: PHP 7 & MySQL 5.5 22,907 pass

1 file was hidden/shown/deleted

File	Size
2920309-54.patch	159.83 KB
8.5.x: PHP 7 & MySQL 5.5 22,906 pass

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

Comment #58

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 November 2017 at 20:44

File	Size
2920309-58.patch	162.63 KB
8.5.x: PHP 7 & MySQL 5.5 22,907 pass

1 file was hidden/shown/deleted

File	Size
2920309-57.patch	162.09 KB
8.5.x: PHP 7 & MySQL 5.5 22,907 pass

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

Comment #59

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 6 December 2017 at 20:36

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

Comment #60

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 8 December 2017 at 05:59

Issue summary:	View changes
Related issues:		+#1885192: Field locales_source.source is not suitable for long texts and huge config objects

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

Comment #61

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 18 December 2017 at 19:25

File	Size
2920309-61.patch	131.19 KB
8.5.x: PHP 7 & MySQL 5.5 22,996 pass

1 file was hidden/shown/deleted

File	Size
2920309-58.patch	162.63 KB
8.5.x: PHP 7 & MySQL 5.5 22,907 pass

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.

Comment #62

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 21 December 2017 at 04:20

Issue summary:

File	Size
2920309-61.patch	132.89 KB
8.5.x: PHP 7 & MySQL 5.5 22,999 pass

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

Comment #63

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 2 January 2018 at 18:40

File	Size
2920309-63.patch	136.92 KB
8.5.x: PHP 7 & MySQL 5.5 23,026 pass

2 files were hidden/shown/deleted

File	Size
2920309-61.patch	131.19 KB
8.5.x: PHP 7 & MySQL 5.5 22,996 pass
2920309-61.patch	132.89 KB
8.5.x: PHP 7 & MySQL 5.5 22,999 pass

New patch, with interdiffs (commits on the Sandbox module) from:

a) #2933920: Should pass untranslated messages into logging -- patch is on #4 https://www.drupal.org/files/issues/2933920.patch

b) #2928351: Make sure filter dependency is in the help topic configuration -- patch is on #4 https://www.drupal.org/files/issues/2928351-4.patch

c) #2927976: UI for translating is unusable -- patch is on #6 https://www.drupal.org/files/issues/2927976-6.patch

Comment #64

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 15 January 2018 at 16:50

Issue summary:

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.

Comment #65

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 15 January 2018 at 17:28

Issue summary:

File	Size
2920309-65.patch	137.78 KB
8.5.x: PHP 7 & MySQL 5.5 23,179 pass

1 file was hidden/shown/deleted

File	Size
2920309-63.patch	136.92 KB
8.5.x: PHP 7 & MySQL 5.5 23,026 pass

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.

Comment #66

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 15 January 2018 at 17:56

Issue summary:

File	Size
2920309-66.patch	139.63 KB
8.5.x: PHP 7 & MySQL 5.5 23,179 pass

1 file was hidden/shown/deleted

File	Size
2920309-65.patch	137.78 KB
8.5.x: PHP 7 & MySQL 5.5 23,179 pass

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.

Comment #69

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 17 January 2018 at 23:38

Issue summary:

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!

Comment #70

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 18 January 2018 at 00:07

File	Size
2920309-69.patch	142.2 KB
8.6.x: PHP 7 & MySQL 5.5 23,223 pass

1 file was hidden/shown/deleted

File	Size
2920309-66.patch	139.63 KB
8.5.x: PHP 7 & MySQL 5.5 23,179 pass

Doh, forgot the patch upload.

Comment #71

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 February 2018 at 23:51

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

File	Size
2920309-71.patch	141.93 KB
8.6.x: PHP 7 & MySQL 5.5 23,455 pass

1 file was hidden/shown/deleted

File	Size
2920309-69.patch	142.2 KB
8.6.x: PHP 7 & MySQL 5.5 23,223 pass

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

Comment #72

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 4 February 2018 at 04:46

File	Size
2920309-72.patch	142.02 KB
8.6.x: PHP 7 & MySQL 5.5 23,455 pass
interdiff.txt	2.25 KB

1 file was hidden/shown/deleted

File	Size
2920309-71.patch	141.93 KB
8.6.x: PHP 7 & MySQL 5.5 23,455 pass

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

Comment #73

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 6 February 2018 at 21:37

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.

Comment #74

yoroy CreditAttribution: yoroy at Roy Scholten commented 6 February 2018 at 22:03

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.

Comment #75

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 6 February 2018 at 22:41

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 to help topics
- #2923918: Theme autocomplete is not ordered

Necessary for full release:
- #2687107: Reorganize topics into sensible outline, and/or write more 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

Comment #76

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 7 February 2018 at 17:40

Issue summary:

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.

Comment #78

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 7 February 2018 at 18:10

Issue summary:

File	Size
2920309-78.patch	140.17 KB
8.6.x: PHP 7 & MySQL 5.5 23,499 pass

2 files were hidden/shown/deleted

File	Size
2920309-72.patch	142.02 KB
8.6.x: PHP 7 & MySQL 5.5 23,455 pass
interdiff.txt	2.25 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.

Comment #79

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 9 February 2018 at 19:48

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 :(

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

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

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

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

+++ 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 :)

```
+++ 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.
```
+++ 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.

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

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

+++ 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!

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

Comment #80

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 9 February 2018 at 23:34

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.

Comment #82

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 11 February 2018 at 03:24

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

Comment #83

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 11 February 2018 at 03:43

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.

Comment #84

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 February 2018 at 17:13

Issue summary:

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.

Comment #85

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 February 2018 at 17:35

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?

Comment #87

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 11 February 2018 at 22:51

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.

Comment #88

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 11 February 2018 at 22:57

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.

Comment #89

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 12 February 2018 at 15:13

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

Comment #90

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 February 2018 at 17:48

Issue summary:

File	Size
2920309-90.patch	142.24 KB
8.6.x: PHP 7 & MySQL 5.5 23,585 pass

Here's a new patch, from two fixed issues in the Sandbox project:

a) #2942644: Config export contains CR characters -- interdiff is the patch on #4 https://www.drupal.org/files/issues/2942644-4.patch

b) #2943923: Connect help topic view page to admin page -- interdiff is the patch on #4 https://www.drupal.org/files/issues/2943923.patch

Updating the issue summary to mark these two fixed.

Comment #91

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 March 2018 at 22:01

Issue summary:

File	Size
2920309-91.patch	144.69 KB
8.6.x: PHP 7 & MySQL 5.5 23,493 pass

2 files were hidden/shown/deleted

File	Size
2920309-78.patch	140.17 KB
8.6.x: PHP 7 & MySQL 5.5 23,499 pass
2920309-90.patch	142.24 KB
8.6.x: PHP 7 & MySQL 5.5 23,585 pass

Here's a new patch, which fixes the remaining issue that (according to my triage at least) is needed before we can try to get this committed to Core as an experimental module.

Issue: #2943919: Make sure route tokens have cacheable metadata
Interdiff: patch from #23 on that issue -- https://www.drupal.org/files/issues/2943919-23.patch

Updating issue summary accordingly, and crediting two more people.

EDIT: updated issue link, had wrong node ID in it, sorry.

Comment #93

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 March 2018 at 22:39

Issue summary:

File	Size
2920309-93.patch	145.51 KB
8.6.x: PHP 7 & MySQL 5.5 23,493 pass

1 file was hidden/shown/deleted

File	Size
2920309-91.patch	144.69 KB
8.6.x: PHP 7 & MySQL 5.5 23,493 pass

Here's another patch, which fixes
#2943921: Move body set/get to a unit test
The patch (interdiff for this patch) is on comment #5 https://www.drupal.org/files/issues/2943921.patch

Comment #95

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 7 March 2018 at 19:17

Issue summary:

File	Size
2920309-95.patch	146.24 KB
8.6.x: PHP 7 & MySQL 5.5 23,503 pass

1 file was hidden/shown/deleted

File	Size
2920309-93.patch	145.51 KB
8.6.x: PHP 7 & MySQL 5.5 23,493 pass

Here's another patch, with the fix to #2942645: Translation UI must not allow changing HTML structure
Interdiff is the patch on #6 there: https://www.drupal.org/files/issues/2942645-6.patch
Updating summary to mark that issue as fixed.

Comment #96

benjifisher

he/him or they/them

English

Boston area

CreditAttribution: benjifisher at Isovera for Pegasystems commented 8 March 2018 at 02:30

In #79, @dawehner wrote, "I'm curious, did you [think] about nesting elements?" and in #85, @jhodgdon replied, "#79 item 1 -- I do not know what this means, sorry?"

I think that @dawehner was suggesting that using a prefix like <dl><dt> and later a suffix like </dd></dl> seems fragile, so it might be safer to do something like this:

-
  prefix_tags: '<dl>'
  suffix_tags: '</dl>'
  children:
    -
      prefix_tags: '<dt>'
      suffix_tags: '</dt>'
      text: 'Defined term'
    -
      prefix_tags: '<dd>'
      suffix_tags: '</dd>'
      text: 'Definition of the term'

(Putting the prefix and suffix first is for my own sake. Presumably, the order does not matter.)

Allowing each element to have (optional) prefix_tags and suffix_tags and one of text or children, we can support nested arrays that have similar structure to nested lists. It begins to look a little more like the YAML version of a render array.

I have not given any thought to how hard it would be to parse the HTML in order to produce such a structure.

Comment #97

benjifisher

he/him or they/them

English

Boston area

CreditAttribution: benjifisher at Isovera for Pegasystems commented 8 March 2018 at 04:05

I demonstrated this module at the Boston Drupal meetup yesterday. It got a good reception, generating a lot of interest.

First of all, there was strong agreement with what @Amber Himes Matz said in #82: this would be a valuable tool for agencies and development shops that build complex sites for clients. The module-provided help is useful for site builders, but the site-builders should be the ones writing documentation for the content editors who will be living with the site once it is built.

There is also strong support for (in the words of the issue summary) more task-based and user-centered help.

Module maintainers in the group said they were not concerned about having their help topics edited for individual sites. Similarly, site builders were not concerned that their documentation might be updated by site administrators.

One concern was raised about this approach: if a module updates its help topics, then it is hard to import those updates, especially if those topics have been edited by the site builder or administrator.

There were also some feature requests:

Add permissions, so that a topic can be restricted to certain roles.
Allow help topics to be embedded anywhere on the site, not just on dedicated admin pages.
Allow help topics to include sub-topics, not just link to them.

Part of the idea of (2) and (3) is to encourage module maintainers to write granular, reusable topics that can be assembled to create task-oriented guides.

My own suggestion for implementing (2) and (3) is to expand the token system. In addition to the url tokens, like [help_topic:url:MACHINE_NAME], provide link tokens (to provide an entire <a> element, not just the href attribute) and embed tokens (to provide the entire text of the help topic). Assuming that you are using the Token API, these tokens can be used anywhere. (They can even be used in text fields with the help of the Token Filter module.)

Comment #98

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 9 March 2018 at 14:53

Issue summary:

RE #96... I see what you mean... I don't see the benefit of it though?

RE #97, good ideas! I have created issues in the Sandbox project for these:
#2951560: Add tokens for embed and link of help topics
#2951565: Expand permissions for help topics
If you have any suggestions or clarifications of these features, please add them there. Especially the second one -- I'm not sure how best to implement it.

Also adding these to the issue summary. I decided the new tokens could be post-beta (although it's pretty easy to do so might get done before), and permissions should probably be pre-beta as they might affect the schema, depending on implementation (again, please help me figure that out on the other issue @benjifisher, thanks!).

Comment #100

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 10 March 2018 at 17:05

Issue summary:

File	Size
2920309-100.patch	147.39 KB
8.6.x: PHP 7 & MySQL 5.5 23,508 pass

1 file was hidden/shown/deleted

File	Size
2920309-95.patch	146.24 KB
8.6.x: PHP 7 & MySQL 5.5 23,503 pass

Here is a new patch, with a fix for #2943920: Use newer API for getting module info.
Interdiff is the patch from #11 on that issue https://www.drupal.org/files/issues/2018-03-09/2943920-extension-list-11...
Module is now only compatible with the 8.6.x branch of Drupal Core.

Updating summary to mark that issue fixed.

Comment #102

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 April 2018 at 18:55

Amber presented this help system today at DrupalCon at this session:
https://events.drupal.org/nashville2018/sessions/new-help-system-drupal

@eojthebrave took notes on the ensuing discussion... An edited/consolidated version of his notes and my responses to them:

People really want to the ability to display/access help in context. For example, on the page the user is currently viewing rather than making them click "help" and navigate away to another page
...
For example, a content type created by a module, that has help text, but you can't edit it because it's hard coded. Could you add help text to pages like that that are local changes, but don't have to be comitted as part of a module
...
The desire seems to be, hey, I'm on this page, I want to just click a button and add some help right here

My response: This sounds to me like the Tour module. If a site has Tour and the contrib Tour UI, you can do exactly that -- add help that is shown right in a particular page. I don't think we should add this to the Help Topics module, or try to combine them.

How are related topics ordered? Is there a way for someone to modify that order?

My response: Currently there is just "Is this topic top-level?" and if so, display it alphabetically on admin/help. So, right now, the only thing a site owner can modify is "is the topic top-level".

However, any particular site could conceivably use a Menu to make a hierarchical/ordered list of topics. We could add that as a feature, but given that help topics come from multiple modules/sources, I'm not sure it would make sense or how it would work? Maybe we should just add something to the help topics about making a help system and suggest using regular Menus to make a hierarchy if that is desired? We could presumably also facilitate this by making a Block visibility criterion of "Show or hide this block on all help topic pages". Thoughts?

Someone asked a question about permissions, and wanted to see what was provided,
...
provide per topic permissions so you can assign topics to a role,

We already have an issue (in progress) about this:
#2951565: Expand permissions for help topics
It has a patch; needs some discussion/review/testing to see if it's usable/desirable to put it into this project.

Including inline images ... there's a button to add them now in the editor, but it was unknown what happens currently when you add one. Are they exported, and included with the config?

I'll point out that if you use the default Help text format and its associated CKEditor config, there is no image button. If you switch to Full HTML or another text format, you might have an image button, but it wouldn't put the image into the config. That said, we already have an issue for this feature, which hasn't gone anywhere yet:
#2665784: Add a token for images and a way to manage images

Why not promote the sandbox project to a full project, and let it exist in contrib, instead of trying to move it into core?

I personally think having this in Contrib not Core is a bad idea, but we can continue to discuss on this existing issue:
#2901802: Release as module, please.

Comment #103

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 April 2018 at 19:05

Issue summary:

For item (b), there wasn't yet an issue, so I created
#2960220: Do we need hierarchy/order for help topics?
and am adding this to the issue summary.

Since we don't yet know what we should do about this idea, I've currently put it in category of "May or may not need to resolve". Images are there too. I think everything else that I know of that came out of the Core Conversation is already an issue...

Comment #106

jhedstrom

English

Portland, OR

CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented 13 April 2018 at 16:54

I just spoke with @Amber Himes Matz regarding @Berdir comment in #87:

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.

This approach seems reasonable, as all of the work already done here around config entities will still work (a deriver will be needed to loop through them and expose them as HelpTopic plugins). Modules can also hard-code HelpTopic plugins that will live with the code, and be able to be updated for sites as the module evolves.

Next steps to implement that approach from the current patch would be:

Define a HelpTopic plugin annotation
Create a plugin deriver that loops through all the config-based help topics
Instead of looping through the config entities in our HelpSection plugin, loop through all of the HelpTopic plugins via the plugin manager

Comment #107

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 13 April 2018 at 19:58

I have some questions on #87/#106:

a) Why would we want to do that (make help topics plugins) (i.e., what would be improved vs. the current system, what problems would it solve, and what new problems would it introduce)?

b) How exactly would a module/theme/distribution be able to provide multiple help topics as plugins? I don't understand at all how it would work.

c) What would the authoring experience be like for module/theme/distribution maintainers writing their help topic pages as plugins?

d) Why would we need this functionality, when Tour module doesn't? It's very analogous: Tour also lets modules/themes/distributions provide tours as config, and they have the same problem with sometimes needing updates. A module can already do an update hook to update default config (checking first to see if the original config has been edited), or to add config... or a site owner can use the Config Update module from contrib to manage these types of updates.

Comment #108

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 15 April 2018 at 16:22

To be clear, I just suggested an approach how both plugins and config entities could be combined not that plugins *should* be used. I think it could be interesting, but think that the advantages and disadvantages should be weighted very carefully before making a decision and starting the implementation.

a) The main difference is that plugins are read from the source and just cached, there's no active storage into which things need to be imported. This means that if you update your documentation as a module, all you need to do is a cache clear and then the updates are visible. So it's closer to how hook_help() currently works. What we'd lose is the ability to edit them directly. If we'd want to be able to customize default help provided by modules then we could still do that, for example with a system similar to base field overrides that we use to store configuration customizations for base fields.

b) That's completely up to you/us, the plugin system doesn't care. Migrations were switched from config entities to plugins for example, almost nothing changed except they were moved to a different folder. The same could be done here.

c) Also not really any changes here. They can either write the yml files by hand (which means that testing them becomes easier because it just needs a cache clear), or they could create a config entity in the UI, export it and put it in the plugin folder.

d) As mentioned above, migrate is the counter example, which started as config entities and moved to plugins and one of the main reasons there was DX. Tour never had the chance to make that transition/rethink that design choice as it was put into 8.0 as stable and isn't allowed to change. Tour also doesn't even have a UI in core, and we both know how well the adoption of of tours in general is. Although I doubt that this change would have made a difference there.

Comment #109

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 16 April 2018 at 15:13

Thanks for the clarifications, Berdir! That makes sense: I think you're saying that the same exact YAML could be used for plugins and config entities. The difference would be the file name and/or location. So module developers could use the UI for config entities, export as config, and move the files to the correct location.

So... that's good. The question is whether we want to make a distinction between help topics that are immutable (provided by modules/themes/profiles and then not editable) vs. ones that are (provided by site builders/administrators). I personally think that we don't -- the philosophy is that since a site builder/administrator owns the configuration of the site, the site builder/administrator should also be able to edit the help for the site, so that it reflects the site's specific config/overrides. It sounds like we could make an override system, so that admins could still edit the plugin-provided help.

So, I guess we could do this. I'm not convinced it buys us much. I think what it solves is the "update the help provided by modules/themes/profiles" problem, which is important. But Core really needs to resolve this problem in general anyway, because of views and tours. See also #2957423: Proposal: Configuration Management 2.0 initiative, where this problem (and others) with the config system are being discussed.

Comment #110

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 16 April 2018 at 20:55

If we went the direction of defining a Help Topic plugin type which saved as a config entity, still with an admin UI, it would be very similar to how Blocks work. Where, a module developer could define a block in their module as a plugin, it appears in the block listing and can be placed by the site administrator, but it can't be edited. Additionally, site administrators can add new custom blocks with the UI that are editable.

I think following a pattern like Blocks uses -- a system that is widely used -- as contrasted to following a pattern that Tour uses -- a system that is not widely used at this time -- would gain the Help Topics project more buy-in and use. I think that is the major win: buy-in from module developers.

What I've been hearing is that module maintainers would like the ability to provide an immutable help topic -- it can still be referenced in the "Related Topics" or "Link this topic on" fields, but otherwise it's truly read-only. Providing this option for module developers by defining a Help Topic plugin type would put them more at ease while not eliminating the UI for site builders/admins, which I agree is a very important audience for any in-site help system.

@jhedstrom walked me through where the refactoring could take place and it didn't seem to be a huge change. I can try to take a stab at a patch this week, which will no doubt need cleaning up.

Comment #111

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 16 April 2018 at 21:29

Issue summary:

OK. Let's start by making a separate issue, and continue the discussion (and patching) there:
#2961552: Refactor using a plugin system
I've added this one to the issue summary as "before initial commit".

Comment #113

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 April 2018 at 14:47

Issue summary:

File	Size
2920309-113.patch	148.59 KB
8.6.x: PHP 7 & MySQL 5.5 23,926 pass

1 file was hidden/shown/deleted

File	Size
2920309-100.patch	147.39 KB
8.6.x: PHP 7 & MySQL 5.5 23,508 pass

Here is a new patch, which fixes #2943974: Work-around for route tokens in Help text format getting truncated after editing a help topic by adding a work-around for the token mangling problem. The core issue at the root of the problem remains open.

Interdiff for this is the patch in #13 on that issue: https://www.drupal.org/files/issues/2018-04-20/2943974-13.patch

Updating summary to mark this issue as fixed.

Comment #114

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 April 2018 at 14:59

Issue summary:

-Needs framework manager review

Minor updates to issue summary to clarify a few things. If we get #2961552: Refactor using a plugin system done, we'll need to edit the summary again to summarize the plugin stuff.

Comment #115

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 April 2018 at 16:12

File	Size
interdiff.txt	1.29 KB
2920309-115.patch	148.56 KB
8.6.x: PHP 7 & MySQL 5.5 23,926 pass

1 file was hidden/shown/deleted

File	Size
2920309-113.patch	148.59 KB
8.6.x: PHP 7 & MySQL 5.5 23,926 pass

Test run showed some minor coding standards messages, so here's a new patch and interdiff.

Edit: Note that I committed these on the sandbox project against this issue #2927976: UI for translating is unusable, which is where they came from.

Comment #116

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 April 2018 at 22:15

Amber and I were looking into refactoring this system to use plugins... It is pretty complicated and I'm not sure it will work.

I would really appreciate it if some of the people who are advocating for this using a plugin system...
... by which I mean @Berdir, @jhedstrom, and @dawehner ...
would take a look at #2961552: Refactor using a plugin system, especially comments 5-7 and the summary... Thanks!

Comment #117

RoloDMonkey CreditAttribution: RoloDMonkey at Last Call Media commented 1 May 2018 at 21:08

I think we can remove the "Needs framework manager review" tag from this issue, if I am reading this correctly #2592487-64: [meta] Help system overhaul.

Comment #118

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 May 2018 at 21:17

Issue tags:

Probably true, thanks.

Comment #119

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 July 2018 at 18:45

Issue summary:

+Needs product manager review, +Needs framework manager review, +Needs release manager review

File	Size
2920309-119.patch	184.86 KB
8.6.x: PHP 7 & MySQL 5.5 24,131 pass, 1 fail

2 files were hidden/shown/deleted

File	Size
interdiff.txt	1.29 KB
2920309-115.patch	148.56 KB
8.6.x: PHP 7 & MySQL 5.5 23,926 pass

Phew! Amber and I finished up the sandbox issue #2961552: Refactor using a plugin system, so here is a new patch for Core. I think it is finally ready for a real review, hopefully in time to get this into Core for 8.6.x... but the deadline is in two weeks, so... we'll see. Please review!

The interdiff for this patch is the patch from comment #63 on that sandbox issue: https://www.drupal.org/files/issues/2018-06-28/help_topics-plugin-refact...

I'm also marking the "make this use plugins" issue as fixed in the issue summary, and adding three new To Dos for post-commit to the To Do list in this issue. And crediting a few more people who helped.

Hopefully this will still apply to the latest Core and the tests will pass... if not I will make another patch shortly!

Anyway, please review! Thanks!

Comment #121

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 July 2018 at 20:29

Status:

Needs work

» Needs review

File	Size
interdiff.txt	5.93 KB

1 file was hidden/shown/deleted

File	Size
2920309-119.patch	184.86 KB
8.6.x: PHP 7 & MySQL 5.5 24,131 pass, 1 fail

Hm. Weird failure talking about HTML output. Maybe due to the deprecation notices? Also some coding standards fixes... Here is a new patch and interdiff. The interdiff is relative to the help_topics module and I also committed it to the Sandbox project. Let's see if this works better.

Comment #122

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 July 2018 at 20:30

File	Size
2920309-120.patch	185.27 KB
8.6.x: PHP 7 & MySQL 5.5 CI aborted

patch upload didn't happen?!?

Comment #124

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 July 2018 at 22:06

Status:

Needs work

» Needs review

It looks like all of the Drupal Core tests failed on that one... some kind of glitch? I will hit Retest...

Comment #125

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 July 2018 at 22:12

File	Size
2920309-124.patch	184.8 KB
8.6.x: PHP 7 & MySQL 5.5 24,134 pass

2 files were hidden/shown/deleted

File	Size
interdiff.txt	5.93 KB
2920309-120.patch	185.27 KB
8.6.x: PHP 7 & MySQL 5.5 CI aborted

Oh -- a file (my revised .htaccess) jumped into that patch that shouldn't have been there. Try this one.

Comment #127

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 July 2018 at 23:27

There we go! The patch in #125 passed tests, and with no coding standards flags. Reviews requested -- let's get this done!

Comment #128

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick at Acquia commented 3 July 2018 at 04:57

Issue tags:

Since this is a new experimental module being proposed for core, adding some tags.

To help with the product manager sign-off, any chance one or both of you could come to UX meeting tomorrow (@ 12:30pm Pacific time in #ux on Slack) and do a demo?

Comment #129

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 3 July 2018 at 05:03

@webchick Yes I would be happy to demo the module at the #ux meeting on Tues!

Comment #130

timmillwood

English

🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK

CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented 3 July 2018 at 08:05

I didn't get chance to take a lot at the whole patch, but here are some nitpicks and a query I had:

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,163 @@
+   *   The plugin id. Maps to the {id} placeholder in the help_topics.help_topic route.
...
+    /** @var \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginInterface $help_topic */
...
+        /** @var \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginInterface $topic */

These line needs to be less than 80 characters.

+++ b/core/modules/help_topics/src/Entity/HelpTopic.php
@@ -0,0 +1,415 @@
+ * configuration, you can make it into a plugin instead of a configurable
+ * entity by putting it into the help_topics directory; you'll also need to

Should this be "configuration entity" or "config entity" rather than "configurable entity"?

```
+++ b/core/modules/help_topics/src/Entity/HelpTopic.php
@@ -0,0 +1,415 @@
+ * @ConfigEntityType(
...
+ *     "add-form" = "/admin/config/development/help/add",
+ *     "edit-form" = "/admin/config/development/help/manage/{help_topic}",
+ *     "delete-form" = "/admin/config/development/help/manage/{help_topic}/delete",
```
One slight concern. On all of our non-local versions of the site, which is 4-10 environments, we currently have around 30 sites. We use the config_readonly module (https://www.drupal.org/project/config_readonly), to make sure all config is created locally, exported, and committed to git.

I just want to be 100% sure here that the use case is for *developers* to create/edit/delete the content?

Comment #131

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 3 July 2018 at 17:16

Hi @timmillwood, thanks for reviewing!

Re: 1, ok. Will reformat and submit a new patch after more reviews come in and Jennifer gets back.

Re: 2, "configurable entities", yes the wording should be changed to config or configuration entities.

Re: 3, modules and themes will provide plugins via YAML files saved in a help_topics directory, not config entities. (That was the plugin refactor we just finished.) However, folks with permissions can use a UI to create and save help topics which are saved as entities and can be exported/imported as configuration entities. So there are 2 use cases being addressed, one for devs and one for site admins.

Comment #132

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 3 July 2018 at 23:44

Today I demo'd Help Topics at the UX meeting. Here are some notes, questions, and ideas from that demo from @webchick, @benjifisher, and @diqidoq who were in attendance.

TL;DR: A patch that leaves out the UI (for now) is more likely to get into 8.6.

Summary: The plugins-based help is viable and gets the thumbs-up. The UI portion of the module, now that it is squarely aimed at a non-dev audience, needs some UI improvements that don't require knowledge of things like routes and machine names of topics that could cause confusion. There isn't a lot of re-work required for the UI, but enough open questions to prevent it from getting into 8.6.

Ideas

- [admin/help] Move Topics above the Module Overviews.
- [UI] Provide a link and/or in-line help for writing help topics in the add new help topic form
- [UI] Help text about the help system not ideal mostly because of the help text about the route-based tokens. The UI and its help should be redesigned/reframed for a non-developer audience (or someone using help topics in a capacity as a non-developer).
- [UI] The autocomplete in the fields "Topics to list here" and "List this topic on" should return the title of the help topic instead of the help topic machine name. (This relates to a new objective of making the UI singularly focused on a non-developer audience.)
- [UI] "Help" text format should be the default selection when creating a new help topic.
- [UI] The tokens help text is confusing because it provides too much technical detail and too many options. (Some confusion about the : vs | help text for the body field. This is related to a fix for XSS filter stripping out colons in tokens.)

Questions (and some answers)

Documentation

- What is a recommended workflow for creating new help topics? Use the UI, export to config, and remove the config_entity value, then save to a module or theme's help_topics directory? Or use an existing example of a help topic plugin in help_topic module's help_topics directory?

Who is the target audience?

- This is for module and theme developers to provide topic-based (i.e. concept/task) help for their users and site owners/teams to create in-site help for their folks to understand how their site-specific workflows/processes.

Translations: How will that work?

- The topics should be chunked into paragraph-sized chunks. See the example help topic plugins in this patch in core/modules/help_topics/help_topics. We have a postponed TODO to update POTX to accept these translation strings in the plugin YAML files. (Postponed into after the module is committed.)

Help text format

- What is special about the help text format? What functionality does it support? Is it global or only support help topics? What is the advantage of Help text format if tokens are global to any format? (Maybe it is the help-topic-specific tokens that don't work outside the Help text format? We didn't test that specific case in the demo.)

Support for other formats?

- If I use Markdown filter, will that mess with things? (I.e. Will tokens still work?)

Tokens

- Clarification needed on the colon (:) vs the pipe (|) in the help text for the body. The help text for the tokens was confusing/overly-technical. Didn't seem appropriate for the new audience. (Now that developers would use plugins, the UI should be focused on a non-developer audience.)

- Also the need was expressed for a better UX for creating links to internal paths or using tokens/route names. Can we make it easier for help topic editors to create links to other internal pages or help topics without needing to know the route? For example, if they could paste in the internal path or providing a CKEditor button that will generate a token based on a path...or something.

General notes

- Help topics visible to any module/theme installed on the system (just like Module Overviews)

- Images not supported right now.

Next steps

- Still gathering feedback and reviews this week.

- Likely will re-submit a patch that excludes the UI and moves Help Topics above Module Overviews, while we work on UX improvements and these other questions. The patch would then provide (only) a way for module and theme developers to provide help topics as YAML-based plugins. Which would be fantastic!

Comment #133

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 7 July 2018 at 15:13

Regarding the idea of removing the UI to get this in as an ***experimental module***, I don't think it is a great idea. There is no easy way for a module or theme developer to write help and export it to a plugin config without the UI.

Also, keep in mind it's a UI aimed at site builders and module/theme developers, not casual users. I agree it can/should be improved, but it's not really aimed at the same audience as the UI for node editing.

Anyway. From my point of view, there are two choices:

a) Get this in with the UI as it is, as an experimental module for 8.6, and work on the UI in core patches. The question is: is it good enough to be an experimental module as it is?

b) If the answer to (a) is no, work on the UI issues listed above in the Sandbox and shoot for 8.7. Sounds like that is where we are?

Comment #134

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 7 July 2018 at 15:19

Quick note on #130 (thanks for the review!!!): I think @var comments are frequently more than 80 character lines due to namespaces being too long. Before reformatting those lines, let's check Core and see how they are doing it.

Comment #135

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 7 July 2018 at 18:54

On the other hand (RE #133)... I could be wrong about the UI. To me, this project seems too difficult to use (for module and theme developers) without the UI part. I wouldn't want to try to write the help topics by hand, anyway -- the UI for editing, then export, then remove a few lines to turn it from config to plugin, seems much easier. But it could be like Tour, which also has only a contrib UI (which I also kind of think is a mistake that module developers would need to download Tour UI to have a viable way to author tours, and that it isn't as robust and usable as it would be if it had to get into Core -- we really benefit from the Core usability team...).

But if everyone else thinks the UI is not necessary, and wants to make a patch to put the non-UI part into Core, that is OK with me. I am, after all, not volunteering to maintain the Core module (although I plan to help).

Thoughts?

Comment #136

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick at Acquia commented 8 July 2018 at 05:29

The UI was explained to us as a way for site builders to create site-specific documentation for their content authors. It makes total sense that core would provide a UI to do that, since nearly all sites would benefit from providing such documentation. However, site builders !== people who can pop open a routing.yml file and start understanding it, and the UI needs a lot more work to be usable by someone who doesn't understand routing.yml files (aka, 99% of the Drupal-using population). https://www.youtube.com/edit?o=U&video_id=n4zlDxVVnnQ is a recording of the meeting where we walked through this UI, if you're curious to see where we were coming from.

If it's instead intended for developers to use as a slightly-friendlier means of generating YAML files, then it doesn't really belong as an "80% case" UI in core, and can easily live in contrib as a developer accelerator tool like Devel and Schema API. (If you want the benefit of the core UX team, please feel free to bring contrib stuff to UX meetings; we're happy to review it!)

But either way, if 8.6 is our target, we have a much better shot getting the stuff that is a solid improvement in and of itself (which is the API which provides hierarchical, topic-based help [a UX team priority since roughly 2008], with the added ability for themes as well as modules to provide it) without bogging it down in all of the already-existing UX problems around creating links to things.

Comment #137

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 9 July 2018 at 01:27

Fair enough!

So. The deadline for 8.6 is coming up very soon (July 18). Making this patch isn't a quick/obvious thing. We'll need to:
- Keep some of the tests but remove others (or parts of some) so the non-UI stuff still gets tested.
- Decide whether to keep the config entities in there and just remove the ability to edit/manage them, or remove them entirely?
- Remove the topics that I wrote about how to use the module, as they refer to the UI.
- Remove some doc headers in some files that refer to the UI.

I have zero time available between now and July 18 for doing this, and depending on the timing, I may or may not be on the Internet when it needs to be reviewed -- I'll be going out into the wilderness for a few of the days between now and then -- bad timing. Sorry!

So, any volunteers to figure out something sensible to do (see the four points above) and make a patch? And review it?

Comment #139

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 11 July 2018 at 06:49

Update: I am working on a stripped down patch containing just the API for modules and themes to provide help topic plugins via YAML files. I am almost done; I just have to go through and modify/delete the tests. I should have a patch ready on Wednesday (July 11).

Comment #140

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 July 2018 at 13:40

Sounds good! I can make some time Wed or Fri to review.

We'll still need someone who isn't either of us to get this to RTBC, and I will be unable to help (out in the wilderness) Sat-Tue... so good luck with the deadline (which is 1 week from today)!

Comment #141

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 11 July 2018 at 21:35

File	Size
interdiff-125-to-141.txt	123.01 KB
help_topics-api_only-141.patch	81.01 KB
8.6.x: PHP 7 & MySQL 5.5 24,023 pass, 3 fail

Alright, I've done the best I could here, but I'm stuck on the cache tags testing in Drupal\Tests\help_topics\Functional\HelpTopicTest. I think this is the only blocker, however, it could also mean that cache tags aren't working as expected in the plugins. @jhodgdon would have a bit more insight into this, I think, since she did the cache tags and the tests originally.

I've attached an interdiff from the patch in comment #125 as well as a patch.

The test I left in the module doesn't pass on my local and I don't expect it to pass here either.

This patch supports help topics as YAML plugins only.

Comment #143

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 July 2018 at 23:55

The help_test test module and the tests that go with it were only intended to work as config entities. When you have a config entity and display it, you need to make sure the cache tags say "this page depends on this config entity". As far as I know, that is not true for plugins.

We also need to have a cache tag, whether for entities or plugins, that says "this page depends on the body text format", as that is a config entity and can change.

We also may need cache tags for things that come up in the tokens, as well as the Related Topics section (for instance, on a plugin page that has a related topic that is a config entity, we would need to have a cache tag for that config entity because its title is being displayed as the link text in Related Topics).

So... it looks like you took the entity out of this patch entirely (which is a valid choice). In that case, you also need to take the tests out that are looking for the help topic entities' cache tags.

Also, there are still some references to the entities in the patch, that will need to be removed:

+++ b/core/modules/help_topics/help_topics.info.yml

+configure: entity.help_topic.collection

That configure link needs to go away.

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php

+ * The format of this file is the same as for the help topic entity. See
+ * config/schema/help_topics.schema.yml

that reference has to be removed. There is no schema or entity now.

+++ b/core/modules/help_topics/help_topics.info.yml
@@ -0,0 +1,10 @@
+name: Help Topics
+type: module
+description: 'Displays and configures help topics'

The module no longer configures help topics, so the description is wrong here.

e) Also here:

+++ b/core/modules/help_topics/help_topics.module
@@ -0,0 +1,48 @@
+<?php
+
+/**
+ * @file
+ * Provides configurable help topics.
+ */

f) Also here:

+++ b/core/modules/help_topics/help_topics.permissions.yml
@@ -0,0 +1,2 @@
+view help topics:
+  title: 'View configured help topics'

g) The autocomplete controller should go away for now.

+++ b/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php

+ *   description = @Translation("Topics can be provided by modules, themes, installation profiles, or site administrators, and they may be organized hierarchically. Top-level help topics configured on your site:"),

Shouldn't say "configured" here. And site admins cannot provide topics.

i) in HelpTopicTest:

+    // Verify links for for configurable topics, and order.
...
+    // Verify access to configurable help topic pages.

This is no longer about configurable topics, because you have converted them into plugin topics.

That's all I can see -- I think it's pretty close!

Comment #145

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 July 2018 at 23:58

I hope that made sense about the cache tags... Basically, if all we have is plugins, then the only cache tags you need to look for are the text format, not the topic ID.

EDIT: Oh, and the permissions and user account cache tags we were testing for are probably still valid... Just take out the topic ID cache tag checks and we're probably OK.

Comment #146

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 12 July 2018 at 00:01

Also, I think we still need the other tests in there, just not the Admin test (as that was testing the UI). But the kernel tests should probably go back in? They should work without the entities and UI.

Comment #147

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 12 July 2018 at 01:12

Thanks for the review @jhodgdon! I'm working on the updates now.

Looks like I'll need to refactor HelpTopicKernelTest to test the help topic plugin, instead of the entity, as it does now.

Comment #148

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 12 July 2018 at 03:02

File	Size
interdiff-comment-141-148.txt	23.43 KB
help_topics-api_only-148.patch	77.54 KB
8.6.x: PHP 7 & MySQL 5.5 23,996 pass, 26 fail

After taking another look at HelpTopicKernelTest, it looked really entity-focused and tested functions we don't have in the plugin, so I left it out.

Tests are passing on my local (yay!). I also ran Code Sniffer and fixed a bunch of coding standards and warnings. I did not fix the following:

FILE: core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicDefaultPlugin.php
----------------------------------------------------------------------
46 | ERROR | Type hint "array" missing for $plugin_definition
54 | WARNING | Only string literals should be passed to t() where
| | possible
61 | WARNING | Only string literals should be passed to t() where
| | possible
----------------------------------------------------------------------

Because 1) The type hint "array" is missing in the parent class and if I add the type hint in my class, I get a warning as well. Can't seem to win here.

And 2) because it seems our use case is an exception.

So this patch is ready for another review please!

Comment #149

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 12 July 2018 at 18:22

Issue summary:	View changes
Status:	Needs work	» Needs review

Updated issue summary with latest objective.

Comment #152

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 12 July 2018 at 20:33

File	Size
interdiff-comment-148-152.txt	807 bytes
help_topics-api_only-152.patch	77.53 KB
8.6.x: PHP 7 & MySQL 5.5 24,104 pass

3 files were hidden/shown/deleted

File	Size
help_topics-api_only-141.patch	81.01 KB
8.6.x: PHP 7 & MySQL 5.5 24,023 pass, 3 fail
interdiff-comment-141-148.txt	23.43 KB
help_topics-api_only-148.patch	77.54 KB
8.6.x: PHP 7 & MySQL 5.5 23,996 pass, 26 fail

Ironically, a coding standards fix produced a testing error:

Declaration of Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginManager::processDefinition(array &$definition, $plugin_id) should be compatible with Drupal\Core\Plugin\DefaultPluginManager::processDefinition(&$definition, $plugin_id)

So I removed the array type hint to make it compatible.

Patch and interdiff attached. (Crossing-fingers!)

Comment #153

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 12 July 2018 at 21:32

Status:

Needs work

» Needs review

Tests passing! Reviews needed!

Comment #154

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 13 July 2018 at 01:32

It looks like the existing tests in the patch passed, and there are no Coder flags. Great job! So, I finally had time this evening to give this patch a thorough review. A few thoughts/suggestions -- the first one is substantive, and the rest are nitpicks (but are all quick/easy to fix)... I am leaving this at Needs Review so as not to stand in the way of someone other than me reviewing it -- I cannot set this patch to RTBC (worked on it a lot), and we will need someone independent to review the patch before the 8.6 deadline if it is to get into Core this go-around...

Anyway, I am happy with the patch if these 5 things are fixed... but depending on the timing, I may not be able to review it again before the deadline:

a) The HelpTopicKernelTest had two purposes. One was to test the export/import of the config entity into YAML, and the other was to test the HTML chunking functionality. The latter should still be tested. In HelpTopicKernelTest, the method testTopicBodyGetSet() tests chunking via the set/get methods for the body on the help topic entity, but it could easily be refactored so that it would instead call the chunk/join methods on the HTML chunker class directly. Otherwise, that class has no tests.

b) Nitpick -- in the .module file, in the hook_help() implementation, I think we should take out this sentence:

It is advisable not to edit the YAML files of module- or theme-provided topics, to make updates easier.

Editing any file provided by a module or theme is inadvisable. It think that sentence was originally meant to say that editing config entity topics was inadvisable (which is important to say because generally editing module config is advisable). but I think for plugin-based topics, that sentence is basically saying "don't hack your modules" so we can leave it out.

c) Another nitpick:

+++ b/core/modules/help_topics/help_topics.permissions.yml
@@ -0,0 +1,2 @@
+view help topics:
+  title: 'View configured help topics'

Help topics aren't really "configured" now. I think this should just say "View help topics".

d) In HelpTopicPluginController:

+   * @param string $id
+   *   The plugin id. Maps to the {id} placeholder
+   *     in the help_topics.help_topic route.

This is formatted incorrectly, and "id" as a word should be "ID" instead ("id" is a psychological term, where ID means identifier, in text). So this should be:

+ * @param string $id
+ * The plugin ID. Maps to the {id} placeholder in the
+ * help_topics.help_topic route.

e) Similar to (c), in the HelpTopicSection plugin:

+ *   description = @Translation("Topics can be provided by modules or themes. Top-level help topics configured on your site:"),

shouldn't say "configured".

Comment #155

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 13 July 2018 at 06:42

File	Size
interdiff-comment-152-155.txt	6.88 KB
help_topics-api_only-155.patch	80.37 KB
8.6.x: PHP 7 & MySQL 5.5 24,105 pass
demo.zip	1.68 KB

Thanks for the review @jhodgdon!

I've refactored the HelpTopicKernelTest as you suggested (and it's passing on my local). I hope it is a decent test.

Also fixed other nitpicks -- thank you!

Patch and interdiff attached.

We really need someone other than myself or @jhodgdon to review this patch, as we have been the primary developers on it. 8.6 deadline is almost upon us. Would really love an outside review and RTBC if possible.

If you are familiar with the plugin system, then this a great issue for you to review.

I've also attached a zipped demo module which demonstrates how a module can add a help topic to the system. Install the module as usual and after applying the patch, go to admin/help and see the demo help topic listed! A similar process goes for a theme (help_topics/how_to_do_this.help_topic.yml file(s) in your theme directory).

Comment #156

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 13 July 2018 at 13:57

The interdiff looks almost perfect!

I have a few nitipicks for the new Kernel test:

a) Doc header:

+/**
+ * Tests some core functionality of the Help Topic entity.

Should probably be something like "Tests the \whatever\namespace\HTMLChunker class."

b) Maybe change the name of the test class to HtmlChunkerTest. :)

c) Given that we aren't testing the entity, I think we don't need any of these lines (the 2 coding standards messages in the test results picked up the use statements, but the others it couldn't figure out):

+use Drupal\Component\Utility\Html;
+use Drupal\Core\Serialization\Yaml;
...
+  public static $modules = [
+    'system',
+    'help_topics',
+    'help',
+    'filter',
+  ];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+    $this->installConfig(['help_topics']);
+  }

d) This line appears twice -- the second time can be omitted:

+    $body_chunked = HtmlChunker::chunkHtml($body);

Comment #157

jhedstrom

English

Portland, OR

CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented 13 July 2018 at 16:46

3 files were hidden/shown/deleted

File	Size
interdiff-comment-152-155.txt	6.88 KB
help_topics-api_only-155.patch	80.37 KB
8.6.x: PHP 7 & MySQL 5.5 24,105 pass
demo.zip	1.68 KB

I like the new direction of getting this in in stages. I think this is essentially RTBC.

While giving it a final review, I did notice these issues:

+++ b/core/modules/help_topics/src/HelpBreadcrumbBuilder.php
@@ -0,0 +1,49 @@
+  /**
+   * Constructs the HelpBreadcrumbBuilder.
+   *
+   * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
+   *   The translation service.
+   */
+  public function __construct(TranslationInterface $string_translation) {
+    $this->stringTranslation = $string_translation;
+  }

Isn't this method (and service injection) redundant to StringTranslationTrait? That trait defines the class property (stringTranslation) as well as a get method...

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicDefaultPlugin.php
@@ -0,0 +1,64 @@
+    $this->stringTranslation = $container->get('string_translation');

Ditto here? I think the string translation trait can be used instead.

Comment #158

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 13 July 2018 at 16:59

File	Size
interdiff-comment-155-158.txt	1.97 KB
help_topics-api_only-158.patch	79.92 KB
8.6.x: PHP 7 & MySQL 5.5 24,144 pass

2 files were hidden/shown/deleted

File	Size
interdiff-comment-148-152.txt	807 bytes
help_topics-api_only-152.patch	77.53 KB
8.6.x: PHP 7 & MySQL 5.5 24,104 pass

Thanks for the reviews @jhodgdon and @jhedstrom!

Re: Comment #156 from @jhodgdon:

I've renamed the class to HtmlChunkerTest and made the modifications you mentioned, which all make sense.

Attached is an interdiff and patch. Tests are passing on my local. I'm about to head out into the woods for the weekend, but I'll be back Sunday and can address any further reviews or questions then.

Re: Comment #157 from @jhedstrom:

I think I'm following you here, but it wasn't immediately obvious to me how to fix it. I'm about to head out of town for the weekend, but I will take a closer look at this and fix on Sunday, unless by chance @jhodgdon can fix if she is available today.

Comment #159

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 13 July 2018 at 17:45

Regarding #157, StringTranslationTrait doesn't inject the string translation service. As it says on
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21StringTra...

If the class is capable of injecting services from the container, it should inject the 'string_translation' service and assign it to $this->stringTranslation.

So, I think we're doing the right thing for those two spots. You're supposed to inject the string translation service in your constructor and set it to $this->stringTranslation to use StringTranslationTrait properly.

Interdiff looks great except one nitpick in the test class doc header:

Tests the Drupal\help_topics\HtmlChunker class.

Should be \Drupal\... (in docs, when using namespaces, start with \ always).

Comment #160

jhedstrom

English

Portland, OR

CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented 13 July 2018 at 17:57

@Amber Himes Matz re:

I think I'm following you here, but it wasn't immediately obvious to me how to fix it

In the first instance:

Remove the __construct method entirely
Remove the string_translation argument from help_topics.services.yml for the breadcrumb builder

Since it's already using the StringTranslationTrait, it will still work as expected.

In the second instance, remove this from the __construct method:

$this->stringTranslation = $container->get('string_translation');

and nothing else need be done there since it isn't actually using calls to $this->t().

Note neither of these are really a blocker, just redundant code that would need to be cleaned up later I think. They don't actually break anything :)

Comment #161

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 13 July 2018 at 18:40

@jhedstrom, see my comment #159. It is documented in StringTranslationTrait that you are supposed to inject the string translation service if you can. If that documentation is wrong, we should file an issue to fix it. Otherwise, I think the code to inject the string translation trait should remain?

Comment #162

1. core/modules/views/src/Plugin/Derivative/ViewsEntityArgumentValidator.php

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 16 July 2018 at 19:07

File	Size
interdiff-comment-158-162.txt	501 bytes
help_topics-api_only-162.patch	79.92 KB
8.6.x: PHP 7 & MySQL 5.5 24,159 pass

2 files were hidden/shown/deleted

File	Size
interdiff-comment-155-158.txt	1.97 KB
help_topics-api_only-158.patch	79.92 KB
8.6.x: PHP 7 & MySQL 5.5 24,144 pass

Thanks for the insights, @jhodgdon and @jhedstrom.

I looked for some examples in core of using the 'string_translation' service as well as uses of StringTranslationTrait.

ViewsEntityArgumentValidator has use StringTranslationTrait; injects it via its __construct() and retrieves the 'string_translation' service from the container in its create(). This appears to follow the advice in the API docs to use the trait and inject the string_translation service and assign it to $this->stringTranslation = $string_translation;. (Our code doesn't have a create() method, but it does inject it and assign it in our __construct method.)

2. Another example retrieves the 'string_translation' service from the services container without using the StringTranslationTrait:
core/modules/config_translation/src/ConfigNamesMapper.php.

3. This example does what we do: uses the trait, injects the service, and assigns it to $this->stringTranslation = $string_translation;, which is what the StringTranslationTrait API doc seems to recommend.

4. I found many examples of using the StringTranslationTrait as @jhedstrom described, with `use StringTranslationTrait` and then just using `$this->t()` in other class functions.

a) core/modules/node/src/NodePermissions.php

b) core/modules/workspace/src/WorkspaceManager.php

c) core/modules/taxonomy/src/TermBreadcrumbBuilder.php

So, on the strength of the many examples of just using the trait, I refactored our HelpBreadcrumbBuilder class to just use the trait and not inject the service, as suggested by @jhedstrom in comment #160. However, when I did that, we ended up with 2 "Help" breadcrumbs, i.e. Home » Administration » Help » Help.

Since using the trait by itself introduces a bug and since I did find a core example that uses the trait in the same way @jhodgdon originally did (example #4 above), and since the API doc appears to recommend this method, I'm going to keep it as-is.

The attached patch and interdiff only adds a missing initial backslash for a namespace in a comment, as per @jhodgdon's review in comment #159.

Comment #163

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 16 July 2018 at 19:11

Deadline is looming. Any chance of a final review and maybe even an RTBC?

Comment #164

jhedstrom

English

Portland, OR

CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented 16 July 2018 at 21:18

Since using the trait by itself introduces a bug and since I did find a core example that uses the trait in the same way @jhodgdon originally did (example #4 above), and since the API doc appears to recommend this method, I'm going to keep it as-is.

That works for me! Resolving that can always be a follow-up.

I think this is RTBC myself.

Comment #165

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett at Acquia commented 16 July 2018 at 21:52

+++ b/core/modules/help_topics/help_topics.services.yml
@@ -0,0 +1,9 @@
+      - { name: breadcrumb_builder, priority: 900 }

Follow-up: Please add a comment explaining the choice of 900

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicDefaultPlugin.php
@@ -0,0 +1,64 @@
+    return new TranslatableMarkup(parent::getLabel(), [], []);
...
+    return new TranslatableMarkup(parent::getBody(), [], []);

Nit: What's with the , [], [] part? Aren't those the default parameters?

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginBase.php
@@ -0,0 +1,166 @@
+  public function getProvider() {
+    return $this->pluginDefinition['provider'];
...
+  public function getLabel() {
+    return $this->pluginDefinition['label'];
...
+  public function getBodyFormat() {
+    return $this->pluginDefinition['body_format'];
...
+  public function isTopLevel() {
+    return $this->pluginDefinition['top_level'];
...
+  public function getRelated() {
+    return $this->pluginDefinition['related'];
...
+  public function getListOn() {
+    return $this->pluginDefinition['list_on'];

Nit: These seem a bit redundant, could have used an object-based plugin definition instead.

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,214 @@
+  public function getDefinitions() {
+    // Since this function is called rarely, instantiate the discovery here.
+    // This finds all the help_topic plugins in theme and module directories.
+    $definitions = $this->getDiscovery()->getDefinitions();

Skipping plugin caching seems bad. Any reason not to move the rest of this logic into ::findDefinitions() instead?

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,214 @@
+      $definition['id'] = $plugin_id;

This isn't done already?

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,214 @@
+    // If this plugin was provided by a module that does not exist, remove the
+    // plugin definition.
+    // @todo Address what to do with an invalid plugin.
+    //   https://www.drupal.org/node/2302623
+    foreach ($definitions as $plugin_id => $plugin_definition) {
+      if (!empty($plugin_definition['provider']) && !$this->moduleHandler->moduleExists($plugin_definition['provider']) &&
+        (!$this->themeHandler->themeExists($plugin_definition['provider']))) {
+        unset($definitions[$plugin_id]);
+      }
+    }

This could even be part of ::alterDefinitions()

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,214 @@
+        $label = (string) $topic->getLabel();
+        $topics[$label] = $topic;

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManagerInterface.php
@@ -0,0 +1,45 @@
+   *   Array of all of the help topics that are marked as top-level, sorted
+   *   by topic title.

I see that these are being sorted by title, but must they also be keyed by title? That seems risky.

Comment #166

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 17 July 2018 at 00:19

Issue summary:

interdiff-comment-162-166.txt

File

Size

3.06 KB

help_topics-api_only-166.patch

80.31 KB

8.6.x:
PHP 7 & MySQL 5.5 24,308 pass

8.7.x:
PHP 7 & MySQL 5.5 24,280 pass

Thanks so much for the review @tim.plunkett!

I'll address (your numbered) points as best I can, but some answers I'll have to defer to @jhodgdon.

+++ b/core/modules/help_topics/help_topics.services.yml
@@ -0,0 +1,9 @@
+      - { name: breadcrumb_builder, priority: 900 }

Follow-up: Please add a comment explaining the choice of 900

I don't know. @jhodgdon?

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicDefaultPlugin.php
@@ -0,0 +1,64 @@
+    return new TranslatableMarkup(parent::getLabel(), [], []);
...
+    return new TranslatableMarkup(parent::getBody(), [], []);

Nit: What's with the , [], [] part? Aren't those the default parameters?

Yes, looks like. And the 2nd and 3rd parameter are both optional. I've taken the , [], [] out of both places in this patch.

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginBase.php
@@ -0,0 +1,166 @@
+  public function getProvider() {
+    return $this->pluginDefinition['provider'];
...
+  public function getLabel() {
+    return $this->pluginDefinition['label'];
...
+  public function getBodyFormat() {
+    return $this->pluginDefinition['body_format'];
...
+  public function isTopLevel() {
+    return $this->pluginDefinition['top_level'];
...
+  public function getRelated() {
+    return $this->pluginDefinition['related'];
...
+  public function getListOn() {
+    return $this->pluginDefinition['list_on'];

Nit: These seem a bit redundant, could have used an object-based plugin definition instead.

I'm not sure how exactly to refactor this and in a Slack conversation @tim.plunkett said it was not a blocker but a stylistic choice. So maybe something to clean up later?

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,214 @@
+  public function getDefinitions() {
+    // Since this function is called rarely, instantiate the discovery here.
+    // This finds all the help_topic plugins in theme and module directories.
+    $definitions = $this->getDiscovery()->getDefinitions();

Skipping plugin caching seems bad. Any reason not to move the rest of this logic into ::findDefinitions() instead?

I looked at the parent class in core/lib/Drupal/Core/Plugin/DefaultPluginManager.php and what you said makes sense to me, too. I refactored it and the changes are in the attached interdiff and patch.

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,214 @@
+      $definition['id'] = $plugin_id;

This isn't done already?

Honestly not sure (this was my first stab at plugins). Can you explain further?

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,214 @@
+    // If this plugin was provided by a module that does not exist, remove the
+    // plugin definition.
+    // @todo Address what to do with an invalid plugin.
+    //   https://www.drupal.org/node/2302623
+    foreach ($definitions as $plugin_id => $plugin_definition) {
+      if (!empty($plugin_definition['provider']) && !$this->moduleHandler->moduleExists($plugin_definition['provider']) &&
+        (!$this->themeHandler->themeExists($plugin_definition['provider']))) {
+        unset($definitions[$plugin_id]);
+      }
+    }

This could even be part of ::alterDefinitions()

Ok, I'm not sure I exactly see why, but I did move that code into alterDefinitions and called the parent class as well (which invokes any hooks), since we're not so much overriding the parent alterDefinitions as adding to it. (I think.)

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,214 @@
+        $label = (string) $topic->getLabel();
+        $topics[$label] = $topic;

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManagerInterface.php
@@ -0,0 +1,45 @@
+   *   Array of all of the help topics that are marked as top-level, sorted
+   *   by topic title.

I see that these are being sorted by title, but must they also be keyed by title? That seems risky.

The label is the title of the help topic. Without a title, the topic is useless (no text to link). A label is essentially required. So, I think it's ok to sort and key by label (aka title). Is there some other check that should be in place to guarantee a label?

Attached is a patch and interdiff addressing the concerns as best I could.

Comment #168

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 18 July 2018 at 17:43

OK, looks like we missed the 8.6.x deadline. Too bad! There is no reason to stop working on this, but I'm unsure about how to proceed... Here are my ideas (the plan could be a combination of these):

a) Continue to work on the plugin-only patch on this issue, until we get it into Core (I assume 8.7.x at this point, unless an exception is made, which I doubt will happen). Initially it will probably go in as an Experimental module. [It seems like we are pretty close to having this patch done? Maybe Tim can review Amber's latest patch and we can go from there and see if we can get it done. I will be out of town mostly until the end of July, but can jump back in to help after that if necessary. It seems like Amber is doing a great job so far on responding to the reviews!]

b) Once A is done, rip the plugin parts out of the Sandbox project so that project is only the configurable help topic entity and its UI. Also change the module shortname to config_help or help_ui or something so it is a separate module from the new Core module.

c) Make the usability review findings in comments #132 and #136 into issues (which I think for now should go in the Sandbox project). Work on these issues.

d) Decide whether the UI and config entity module belongs in core or contrib. Contrib-only is a viable option, if the plugin module is in Core I think?

e) Work on getting the Core plugin-only module from experimental to full module. There is a list in the issue summary of things to do. Some of them only apply to the UI and config entities, so we'll need to sort that out.

Thoughts?

Comment #169

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 18 July 2018 at 17:50

RE questions from Tim in #165 that Amber didn't already address:

- I have no idea why I put a priority of 900 on the breadcrumb builder. Or indeed if we need a priority at all, or what it should be. Any guidance?

- Regarding item 3 about the pluginDefinition['whatever'] things -- the other plugin class that is no longer in this patch (the one that is a Deriver for the config entities) does these methods in quite a different way, and also sets up the plugin definition in quite a different way... if you're OK with leaving this as-is, I think it would make our lives easier for the derived-from-config-entity plugins.

Otherwise I agree with what Amber said/did. Let me know if there's anything else I can help with -- I'm somewhat around today and tomorrow, then off for 10 days or so.

I'd also like to point out that we used existing plugin code from the MenuLink module as our model for a lot of this, so many of the decisions about how to do the code came from there. You might want to review the plugin manager for menu links and see if it suffers from some of the same problems as Tim pointed out in #165, and if so, consider refactoring it too.

Comment #170

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett at Acquia commented 18 July 2018 at 18:07

Status:

Needs review

» Reviewed & tested by the community

Oh my, yep. Seems about 90% of everything I pointed out is also present in MenuLinkManager or MenuLinkBase.

With that in mind, I think this is fine. Still has all the committer review tags, but RTBCing nonetheless.

As an experimental module, this could still make it in by the beta deadline, in theory. That's what happened with Layout Builder!

Comment #171

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 18 July 2018 at 18:25

Thank you @jhodgdon and @tim.plunkett for the reviews and clarifications! And for the RTBC! (Yay!!)

Let's wait to see what comes of the product/framework/release manager review(s) before deciding on next steps. FWIW, I think @jhodgdon's plan in #168 makes sense. But let's wait and see for now.

Comment #172

Greg Boggs

he/him

English

Portland Oregon

CreditAttribution: Greg Boggs commented 18 July 2018 at 18:47

I installed the patch and reviewed the content from an end user prospective on Simply Test Me. The help files are well-written and well-organized. It very much feels like something that's always been a part of Drupal.

Comment #173

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 18 July 2018 at 18:58

Thanks for testing Greg! We have plans to write more topics. The ones that are in there now are accurate (I hope!) but the list is incomplete. We have an issue about creating more topics at #2687107: Reorganize topics into sensible outline, and/or write more topics.

Comment #175

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce commented 24 July 2018 at 10:45

This issue was discussed on the recent committer call. One of the things came out of that was that we should target an early commit to 8.7.x. Committing a completely new experimental module during the alpha / beta stage of a release feels rushed.

I'm really happy that the configuration entity has been dropped from the implementation. I can understand the use case for having some help topics configurable managing all help topics via configuration felt very wrong. This issue summary needs an update for the new approach.

One thing that moving way from configuration allows us to ask is: "Is HTML in YAML files is the best way to do this?"

+  -
+    text: 'Date and time formats'
+    prefix_tags: '<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>'

The whole concept of an array with prefix_tags etc… feels like we're optimising for the UI use case which no longer is part of the patch and if a module (in core or contrib) does do the UI case it should be feel to do it how it likes.

And lastly how do we avoid confusion with the existing help module and maybe this should just be a part of that module not a separate thing. We could add it to the help module and mark all the API with @internal until we decide it is ready. That way users won't have to work out the difference. This point needs further discussion with release managers.

Comment #176

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 25 July 2018 at 12:40

That all sounds reasonable. Both Amber and I are out at the moment but we plan to confer the week of Aug 6 and respond to this, update issue summary, etc. Thanks for taking a look!

Comment #177

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 8 August 2018 at 19:12

Issue summary:

Amber and I are back, and we're ready to do whatever needs to be done to get this into Core, if possible!

So, first, to respond to @alexpott's comments in #175:

a) We would be happy to either leave this as a separate experimental module (as the patch in #166 is now), or to move it into the existing core Help module. If we do move it into Help, we should probably also move the HtmlChunker class into core/lib/Component.

This decision sounds like something that the product managers and/or other committers need to decide on. So, if you want us to move it into core Help, let us know and we'll update the patch. If not, we have a patch that I think is ready to go.

b) The question of YAML, HTML, and the "chunking" with the tags separated out... A few thoughts:
- We are using YAML because we implemented this plugin system with YAML discovery, with each topic in a YAML file.
- The other option would be to implement the plugin system so that each topic is its own annotated plugin class, but that seems like overkill, since a help topic is basically/usually (and most simply) some marked-up translatable text.
- The chunking is necessary for translatability. This is explained in the issue summary, but basically for usability by translation groups on on localize.drupal.org and for our translation database in Drupal, translatable UI strings need to be no larger than about a paragraph, and as free from HTML tags as possible.

Given all of that, the YAML-format with broken-up HTML text seems like the best format/implementation. But if someone has a better idea, or if you think we should use annotated plugin classes instead of static YAML files, we can think about that.

c) With this comment, I am also updating the issue summary so it reflects our current plan, which is to (hopefully!) get this plugin-based help topic system into Core, and once it's there, work on writing some more help topics for Core. We are for the moment dropping the UI and config entities. We plan to keep working on it in the contrib/sandbox module and may try to get it into Core eventually, we'll see. It may be fine to just have it in contrib.

d) We will also be making the usability review from #132 and #136 into issues on the contrib/sandbox module sometime soon. I do not think that any of them were about the usability of the plugin system, but will be checking on that as I make issues, and if so, I will update the issue summary here.

Comment #178

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 8 August 2018 at 23:36

Issue summary:

I went through the Usability Review comments #132 and #136.

I created a few issues on the Sandbox project https://www.drupal.org/project/issues/2369943 . But a couple of the findings are related to this Core patch as it is now:

[admin/help] Move Topics above the Module Overviews.
If we want to do that, it needs to get into the Core patch, but I'm wondering if it should wait until we have more topics? So I didn't create an issue yet. I've added it to the issue summary as possible follow-up here though. I don't think it should hold up this patch and I don't think we necessarily want to do it right now, since there are only a few topics currently.
There were some questions about the Help text formats vs. other text formats:
- What is special about the help text format? What functionality does it support? Is it global or only support help topics? What is the advantage of Help text format if tokens are global to any format? (Maybe it is the help-topic-specific tokens that don't work outside the Help text format? We didn't test that specific case in the demo.)
Support for other formats?
- If I use Markdown filter, will that mess with things? (I.e. Will tokens still work?)
Answers: There is nothing particularly special about the help text format. However, since topics have a text format, it is important to make sure the text format exists, so the module provides one to go with its topics (in particular, the "standard" ones that come with Drupal are only part of the Standard install profile, except "plain text", which really wouldn't be helpful.). You can use any text format that you know your module customers will have available. Tokens still work.

Comment #179

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 21 August 2018 at 18:27

There will be a Usability group meeting in just over an hour where we plan to discuss questions about this module with the Drupal Core product managers and other people interested in Usability. Join the #ux channel in Slack to find out details, and see https://www.drupal.org/slack to find out how to join Drupal slack.

I will be doing a brief demo of what Drupal looks like with the patch in #166, and the YAML format for help topics. And I hope to discuss the following questions:

Do we want Help Topics functionality in Core at this time, with the ability for modules/themes to supply topics as YAML plugin files?
Should it be
(a) Added as an Experimental module (because it's maybe not quite stable and/or there aren't many topics)? [patch in #166] or
(b) Added as part of the existing core Help module (because it will make it easier to find the new functionality)? [make a new patch]
Is the proposed YAML format OK (especially regarding the HTML chunking -- we need some kind of chunking for translatable)?
Is the help provided for module/theme developers sufficient?
From the previous Usability meeting demo, should we move the Help Topics section above Module Overviews on admin/help, or perhaps wait until there are more topics?

Comment #180

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 21 August 2018 at 20:13

At today's usability meeting (see previous comment), we concluded the following regarding the questions in the previous comment (the recording will be distributed to people who were unable to attend, so they may have further feedback):

Yes, we should get this into Core...
As an experimental module for now (see issue summary for list of what needs to be done to get to full release; keep as Experimental until then).
Chunking is needed for translation. We would either need to keep the current chunked YAML format (or something similar), or move the chunking ability somehow into POTX and Interface Translation (which seems complicated?) But this decision should be by the Framework Manager team (we already have the "Needs framework manager review" tag).
We should add a section to the hook_help() that talks about how translation of topics is accomplished. Otherwise, the existing help seems to be sufficient.
Yes, move the Help Topics section up above Module Overviews. If someone has enabled the experimental module, it's because they want to see the topics.

So, I'll see if I can get a Framework Manager person to review the chunking. And I'll post a link to the recording of the Usability meeting when it's available.

Comment #181

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 21 August 2018 at 20:23

Recording of usability call: https://www.youtube.com/watch?v=Q63hOhdqnoE

Comment #182

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 21 August 2018 at 20:38

Issue summary:

So @Amber Himes Matz -- what we need to do for the current patch (barring changes to the "chunking" model) is:

a) Move the help topics section to the top of admin/help. Hm... I had thought this was straightforward but I see now that HelpSection plugins do not have a weight to them. So, to do that we would need to add a weight parameter to HelpSection annotation, and then use it in HelpSectionManager to order by weight. I suggest we move this to a separate post-commit issue, because it is a bigger change to the core Help module and probably shouldn't be part of this patch. So, added this to the issue summary as a "get this to release" item.

b) Add a section to the hook_help() for this module that explains how translation works. It should basically say that for contributed modules and themes, the text in the body section of the help topic plugin files will be picked up by the translation extraction system as translatable strings, so that if a site has the Interface Translation (locale) module enabled, this text will be translatable just like other interface text in the site. For custom modules, you would need to attempt to view the topic once on a site in another language for the translation system to pick up the translatable text, and then you could translate it using the Interface Translation module.

[Edit: Added this paragraph] We should look at how the hook_help() for locale explains the Interface Translation system, and/or link to that in our help.

Hope that makes sense... Anyway (b) is the only thing to do. If you want, I can make a go at that text...

Comment #183

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick at Acquia commented 22 August 2018 at 06:22

I'm really sorry to have missed the meeting due to life circumstances, but just for the record, the plan in #180 sounds solid to me, as well. Thanks for presenting!

Comment #184

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 22 August 2018 at 13:43

@jhodgdon

Yeah, I agree with your assessment of a). I had discovered a similar roadblock when I looked into it in July hoping for a "quick fix". I don't think that change should be part of this patch.

For b), yes, please go ahead with that text update when you have time and I'd be happy to review.

Thanks!

Comment #185

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 22 August 2018 at 17:21

Suggested text for hook_help: ... let's see ...

$output .= '<dt>' . t('Translating help topics') . '</dt>';
$output .= '<dl>' . t('The titles and body text of help topics provided by contributed modules and themes is translatable using the <a href=":locale_help">Interface Translation module</a>. Topics provided by custom modules and themes are also translatable if they have been viewed at least once in a non-English language, which triggers putting their translatable text into the translation database.', ':locale_help' => (\Drupal::moduleHandler()->moduleExists('locale')) ? \Drupal::url('help.page', ['name' => 'locale']) : '#']) . '</dl>';

I think this is enough. We should leave explanations of how interface translation works to the Interface Translation module help (which we're linking to here).

Thoughts? Didn't make a patch yet. If you have a clone handy and can put it in a patch, that would be great... mine is a bit messed up right now.

Comment #186

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 22 August 2018 at 18:18

File	Size
interdiff-comment-166-186.txt	1.94 KB
help_topics-api_only-186.patch	3.16 MB
8.7.x: PHP 7.1 & MySQL 5.7
help_topics_hook_help_screenshot.png	314.74 KB

Thanks @jhodgdon! I fixed a few little things. Attached is a interdiff from the last updated patch, a patch, and a screenshot of the Help Topics help page with the change.

Comment #188

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 22 August 2018 at 18:47

File	Size
interdiff-comment-186-188.txt	2.17 KB
help_topics-api_only-188.patch	3.16 MB
8.7.x: PHP 7 & MySQL 5.5 PHP 7.1 & MySQL 5.7

The test failure says "Requeued after CI error" but we found a grammar issue in the new text about translating help topics so I'm uploading a new patch.

Comment #191

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 22 August 2018 at 19:50

File	Size
interdiff-comment-166-191.txt	1.94 KB
help_topics-api_only-191.patch	80.91 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,344 pass

4 files were hidden/shown/deleted

File	Size
interdiff-comment-166-186.txt	1.94 KB
help_topics-api_only-186.patch	3.16 MB
8.7.x: PHP 7.1 & MySQL 5.7
interdiff-comment-186-188.txt	2.17 KB
help_topics-api_only-188.patch	3.16 MB
8.7.x: PHP 7 & MySQL 5.5 PHP 7.1 & MySQL 5.7

The last two patches were a mess. I have fresh install of 8.7.x dev, applied that last patch that passed testing (from comment #166), and then added the text about translating help topics to hook_help. Sorry about the mess!

Comment #192

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 22 August 2018 at 20:03

Status:

Needs work

» Needs review

That looks like a better patch. :)

Comment #193

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 August 2018 at 04:41

Status:	Needs review	» Reviewed & tested by the community
Issue tags:	-Needs product manager review, -Needs release manager review

Tests passed, and both Amber and I reviewed the two-line change to the Help page for the module, so I think we can set it back to RTBC (no code changes since the last RTBC patch).

We have approval from Product Managers so removing that tag. Just waiting on Framework Manager review I guess... I pinged people in #contribute so we'll see.

Comment #194

EclipseGc CreditAttribution: EclipseGc at Acquia commented 23 August 2018 at 17:02

Ok, so I gave the patch a cursory review. I can't say I'm a huge fan of the chunking since it functionally recreates features we already have in our render system today while simultaneously tip toeing around that same render system... that being said I think the yaml abstraction requires something like the HTML Chunker and prevents the need to write a new PHP class for each help doc (yay!). So consider me on board.

I think (and maybe this was justified at some earlier point in this conversation) I'm just wondering why the HTML Chunker creates flat strings instead of converting the yaml definitions into render array markup/prefix/suffix keys and returning an array. Is this because we want to run tokenization just once? I suppose that makes good sense.

On a separate topic, 182.a mentions this issue of help weights. I'm curious if someone could describe this more in depth. Generally weights in plugin definitions is going to be really hard to manage. My initial thought is that an index file(s) somewhere might be a better solution to grouping and weighting "like" topics. If this sounds appealing I'd be happy to discuss it further here or on slack.

Eclipse

Comment #195

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 August 2018 at 18:15

Thanks for the review!

Yes, one flat string is definitely easier for the token replace. It's also good for the UI for editing (which doesn't exist in the core patch; will be in contrib only at least for now) -- you want to take the chunked stuff, paste it all together, edit the HTML in an HTML editor, and then when you're done, chunk it and save.

Regarding weights... I think you are confusing the Help Topic plugins with the Help Page Section plugins? We're talking about weights for the section plugins -- to determine the order of the sections on admin/help, which currently consist of:
- hook_help() module overviews
- Help Topics (this patch)
- Tours
in that order. We want Help Topics to appear first, and currently it's in the middle.

Having weights for these 3 sections on the Help Page Section plugins doesn't seem unreasonable, and I don't anticipate Core having any more sections, or Contrib supplying more than a few (Advanced Help perhaps? But hopefully they would convert to Help Topics maybe? Hopefully?).

Within the Help Topics section, we list the top-level help topics in alphabetical order; my feeling is anything else would make it difficult to scan, so we're not proposing weights for the topics.

Comment #196

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 August 2018 at 19:58

Issue summary:

Created #2994748: Make a way for Help Page Sections to be ordered and adding it to the issue summary, to address the order of sections on admin/help.

Comment #197

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 23 August 2018 at 20:52

Code review

+++ b/core/MAINTAINERS.txt
@@ -232,6 +232,10 @@ Hypertext Application Language (HAL)
+Help Topics
+- Amber Matz 'Amber Himes Matz' https://www.drupal.org/u/amber-himes-matz
+- Andrey Postnikov 'andypost' https://www.drupal.org/u/andypost

yay! great to have two maintainers from day 1

+++ b/core/modules/help_topics/help_topics.tokens.inc
@@ -0,0 +1,95 @@
+    'description' => t('The URL to the route. Provide the route name as route:url:ROUTE_NAME'),

how does one specify route parameters with this token? not supported?

+++ b/core/modules/help_topics/help_topics.tokens.inc
@@ -0,0 +1,95 @@
+  if ($type == 'help_topic') {

nit ===

+++ b/core/modules/help_topics/help_topics.tokens.inc
@@ -0,0 +1,95 @@
+  elseif ($type == 'route') {

we could return in the previous chunk and avoid an elseif here (just an if)

+++ b/core/modules/help_topics/help_topics.tokens.inc
@@ -0,0 +1,95 @@
+      catch (\Exception $e) {
+        // Invalid route or missing parameters or something like that.

should we use a more focused exception class here instead of the pokemon approach?

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,164 @@
+            'url' => Url::fromRoute('help_topics.help_topic', ['id' => $other_id]),
...
+        'url' => Url::fromRoute('help_topics.help_topic', ['id' => $other_id]),

could use $plugin->toUrl() here?

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,232 @@
+  public function getDefinitions() {
+    return parent::getDefinitions();
+  }

not needed?

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,232 @@
+      if (in_array($id, $definition['list_on'])) {

should use the third argument here?

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicTest.php
@@ -0,0 +1,227 @@
+   */

could use inheritdoc here

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicTest.php
@@ -0,0 +1,227 @@
+    $this->verifyHelp(403, FALSE);
...
+    $this->verifyHelp();
+    $this->verifyHelpLinks();

would 'assert' be more appropriate naming than 'verify'?

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicTest.php
@@ -0,0 +1,227 @@
+      if ($response == 200) {
...
+      elseif ($check_tags) {

are these mutually exclusive? should second one just be if? not elseif?

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicTest.php
@@ -0,0 +1,227 @@
+      $session = $this->assertSession();

you can create this outside the loop and save a bit of time, it will update as you navigate

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicTest.php
@@ -0,0 +1,227 @@
+  protected function assertCacheContext($expected_cache_context) {
...
+  protected function assertCacheTag($expected_cache_tag) {

Any reason you can't use AssertPageCacheContextsAndTagsTrait?

Concept review

I have to agree with @alexpott, I'm not very comfortable with inventing a new concept for markup in the yaml/chunking approaches. However I do concede that translation support is difficult with large chunks of HTML.
My first thoughts were, we have a templating engine (twig), why can't we use that?

So as a thought experiment, here's an extract of one of those topics in twig with translation.

<p>{% trans% }The settings for your site are configured on various administrative pages, as follows {% endtrans %}</p>
<dl>
  <dt>{% trans %}Site name, slogan, and email address{% endtrans %}</dt>
  <dd>{%trans %}On the <a href="[route:url:system.site_information_settings]"><em>Basic site settings</em></a> page, which you can reach in the main <em>Manage</em> administrative menu, by navigating to <em>Configuration</em> &gt; <em>System</em> &gt; <em>Basic site settings</em>{% endtrans %}</dd>
</dl>

No this isn't alterable, like the plugins would be, however if we did auto theme hook registration via plugins, it would be simple to modify in the theme.

So what would that leave the plugins to look like?

id: config_basic
label:  'Changing basic site settings'
top_level: true
related: {  }
list_on: {  }
theme: help_topic__config_basic

With the contents in help-topic--config-basic.html.twig

So looking back over the comments, there's some discussion from @dawehner about using twig, but the context is lost. I can't see where his comments are quoting from.

I also note that @Amber Himes Matz has pointed out that 'creating twig files' isn't something a non-technical user can do. But neither is creating these yaml files.

Looking at the comments and history on this issue, is appears that at one point, we allowed admins to create new topics via the UI, which would explain the need to chunk from HTML back to the YAML format. It appears like that functionality is now in contrib?

It also appears that we at one point used config entities for storage, which would also explain the ability for admins to create new ones.

Now however, it seems though we've settled on plugins with yaml discovery. Which means we're back to being only created by developers. Is there a plan to augment this with a derivative discovery based on admins entering topics? Because if so, that would require the chunking.

But without that, I can't see anything in the current patch that uses \Drupal\help_topics\HtmlChunker::chunkHtml which means we're adding a lot of code that isn't being used. Is there a plan to use it in the future (Such as a derivative discovery based on admin-added help topics based on config entities?). If not, I don't think it needs to be added, as its a quite complex piece of code to maintain, with no uses.

The comments above mention the UI functionality exists in the contrib module. In my opinion the chunking (not the joining) should exist there too (in a standalone class). If the plan is to bring that UI to core, we could bring the chunking at the same time.

So if the longer-term plan is to have a UI etc, I'm not sure it makes sense to enforce the yaml format on fixed help topics provided by modules. We could introduce a second plugin type/manager for help topic rendering and have two methods of rendering. With contrib able to extend that further if needed. E.g.

id: config_basic
label:  'Changing basic site settings'
top_level: true
related: {  }
list_on: {  }
renderer: help_topic_twig
renderer_config:
  theme: help_topic__config_basic

And then the UI based ones would be

id: some_plugin
label:  'Some help topic'
top_level: true
related: {  }
list_on: {  }
renderer: help_topic_chunked_html
renderer_config:
  body_format: help_topic
  # chunked body here
  body: ...

And in this scenario, the help_topic_chunked_html topic renderer could live in contrib until we brought the UI in.

I'm playing devil's advocate with a lot of this, so apologies if some of this is off-topic/been covered before. I've tried to ascertain the answers from the conversations above.

I really appreciate the work that has gone into this so far.

Comment #198

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 23 August 2018 at 21:05

renderer: help_topic_twig
renderer_config:
  theme: help_topic__config_basic

Actually, we don't even need this.

Because plugins are classes and getBody is a method, we could put the decision to use twig or chunking inside that.

Then the automated discovery of config entity based plugins would just add a class: value pointing to a different class

Comment #199

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 August 2018 at 21:13

Thanks for the review @larowlan! I'll respond to a few points (most I'm OK with changing, and/or don't need response):

2. Parameters are not supported by the route token, at least currently.

5. I don't think trying to imagine all the exceptions that could happen and catching each one is worth the trouble. In general, and/or in the past, token replacement has been "If you find a token to replace, replace it. If it's invalid or you don't find a replacement, leave the text as it is". So if an exception happens, that's the response: "leave it as it is". I think that is the correct answer?

7. Yeah, that must have been left over from a previous patch when it used to do something else.

8. I don't understand the question here?

11. If the response is 200, then we're doing what's in that elseif() anyway (look a few lines up). So, I think it's fine.

12. Did not know that.

13. Did not know about that trait, sounds useful.

Will discuss the larger questions elsewhere...

Comment #200

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 August 2018 at 21:22

So... Regarding the larger question...

We could definitely have module/theme developers use Twig for topics. It does have some advantages.

One disadvantage is that we do have a (currently, and maybe always) contrib module that lets people edit topics as config. So if a module/theme developer installs this contrib module, they can use an HTML editor (and yes, that is the only usage of the Chunk function, and it is out in contrib and not in the current patch) to author help, export the resulting config (which uses the exact same YAML as the plugins), remove a few lines that are in config but not in plugins, and save the file to help_topics and thereby have a plugin.

This gives a not-too-technical user the ability to author help. It also makes it pretty easy to switch between config and plugins, if you have this contrib module installed.

And, we might eventually get the UI into core, along with the ability for admins to create topics specific to a site. To me, it's a strong need and belongs in Core... it's not going in now because the UI isn't ready.

Thoughts?

Comment #201

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 August 2018 at 21:25

Also regarding removing the chunk function from the HtmlChunker class, if that is the only way to get the patch into Core, then I am OK with taking it out and having it live in contrib for now. We should also rename the class in that case, because it won't do any chunking.

But I think it's a useful utility, that the two functions belong together in one class, and that it should be in core/lib/Component really.

Comment #202

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 23 August 2018 at 21:27

Right, so I think we can support both the twig based and the admin can edit with chunked storage approach.

in_array($id, $definition['list_on']))

8. I don't understand the question here?

if you're using in_array with strings, you should always use in_array($id, $definition['list_on'], TRUE))

https://3v4l.org/TpIip

Comment #203

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 August 2018 at 21:27

Also regarding the UI ... We would like to have more topics in Core. The system could be quite useful. But if we do not have a way for non-technical people (I mean not even module developers, but people who are good writers of documentation) to author topics, then we are not going to get that done.

Comment #204

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 23 August 2018 at 21:44

The problem is that if we move to "plugins use Twig" then we have no UI for module developers or Documentation contributors to use to author help topics. It's probably OK for module developers -- they're presumably fairly technical. But both Core and larger contrib modules have contributors who write documentation and who aren't so technical. Having a UI for them to use, even if it is in Contrib land, is a good thing.

Comment #205

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 24 August 2018 at 05:58

Another problem with using Twig templates: how would we run the text through token replace? That's a really really useful feature... otherwise there is no good way to make links to other topics or links to routes in the site.

Comment #206

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 27 August 2018 at 21:04

So... Not sure about Twig templates, due to the token replace question (#205), plus the question of having a UI (#203/204). Any thoughts on these @larowlan?

Meanwhile, we need to make a new patch to address the review points in #197 (see my responses in #199 on a few points). @Amber Himes Matz, do you want to do that?

Comment #207

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 27 August 2018 at 22:50

Twig has the url (https://www.drupal.org/docs/8/theming/twig/functions-in-twig-templates#url) and link functions, so I don't think we need the tokens to support linking topics/pages.

In terms of user-editing I'm advocating that we can do both. One plugin class that is twig based, another (possibly in contrib) that uses chunks and is derived based on user supplied values (so I assume a config entity).

I think the key thing we want to deliver is the notion of topics and the plumbing (Routing, controllers, plugin system) to support them. It may even be that those bits could eventually live in the help module. Once that plumbing is in place, the way they are built and stored is extensible.

Comment #208

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 27 August 2018 at 22:59

Point taken on the tokens not being needed in Twig.

Regarding the UI, I will try again to explain what I meant:

We want to be able to have contributors who are not technical write help topics for Core and Contrib (modules, themes, and distributions). For these contributors, having a UI for editing removes a barrier that gets in the way of them being able to contribute in this way.

If we keep the current system with the chunks, then such contributors can download the contrib Configurable Help module, use the UI to create a config entity, export the config entity, take out a few lines, and voila! they have a plugin file, because the YAML in the config entity and the YAML in the plugin is exactly the same.

If we change the plugin system to use Twig templates, then there is no UI for editing topics for distribution with Core and Contrib, and anyone who wants to contribute a topic to a project would need to be able to edit Twig files.

Comment #209

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 27 August 2018 at 23:08

Ah, ok I understand. Let me ponder that a bit.

Comment #210

markhalliwell

English

CreditAttribution: markhalliwell at Tag1 Consulting commented 28 August 2018 at 15:22

As I stated in another issue (somewhere), Twig is the correct approach for any sort of HTML based content like this, not YAML; which is intended mostly for configuration. We really don't need to reinvent the wheel here.

Like YAML, Twig can also be stored in the DB safely like Views currently does for its field rewrites. Any UI that is created (which really is a separate issue altogether and has since been descoped from this issue) can simply utilize a textarea on the form for the help topic that contains the Twig template for the markup/content; which we can also determine if it's been manually overridden if a DB entry exists and differs from the code based template (if any).

Re: tokens, any type of variable (and render arrays) can be passed to Twig templates and then printed out using the {{ variable }} notation. Furthermore, one could then have access to the full range of filters and functions also available in Twig templates. A standard set of predefined variables can be established if needed and anyone can just preprocess the template to add more in if needed.

IMO, using Twig would be a far richer and consistent approach with how HTML is delt with in the Drupal eco-system. Using YAML in this way feels like it's going against the grain.

Comment #211

effulgentsia CreditAttribution: effulgentsia at Acquia commented 28 August 2018 at 16:41

Sorry, I'm late to this conversation, so maybe am missing some history, but seems to me like we're discussing YAML or Twig, but what about YAML and Twig? Much of what's currently in the YAML seems like it belongs in YAML (like label, list_on, related, etc.). But the HTML belongs in Twig. The idea of a plugin requiring both some YAML and some Twig already has precedent in Drupal core's layout plugins and in https://www.drupal.org/project/ui_patterns (see blog post).

use the UI to create a config entity, export the config entity, take out a few lines, and voila! they have a plugin file

If a process that involves manually taking out a few lines from a file is acceptable, then is it meaningfully more cumbersome to use a UI that exports/imports two files per help topic?

Comment #212

markhalliwell

English

CreditAttribution: markhalliwell at Tag1 Consulting commented 28 August 2018 at 16:49

True, the configuration bits should likely remain in YAML and utilize the existing plugin system for discovery. My only concern has ever been about HTML in YAML which would really just create an anti-pattern in the eco-system.

Comment #213

effulgentsia CreditAttribution: effulgentsia at Acquia commented 28 August 2018 at 17:38

Looks like #197 already proposed what I wrote in #211.

However, another thought I have is that in my opinion, translatable text does make sense in YAML, even if it has some minor markup. E.g., we already have <em> tags within the description values of node.type.*.yml config entities.

What belongs in Twig is more significant HTML choices. It's a bit subjective on where you draw the line between minor markup (like <em>) and significant HTML, but what about something like the following for #197's example of config_basic.help_topic.yml:

Within YAML:

body:
  -
    name: introduction
    text: 'The settings for your site are configured on various administrative pages, as follows:'
  -
    name: settings
    type: definition_list
    items:
      - 
        term: 'Site name, slogan, and email address'
        definition: 'On the <a href="[route:url:system.site_information_settings]"><em>Basic site settings</em></a> page, which you can reach in the main <em>Manage</em> administrative menu, by navigating to <em>Configuration</em> &gt; <em>System</em> &gt; <em>Basic site settings</em>.'
      -
        term: 'Time zone and country'
        definition: 'On the <a href="[route:url:system.regional_settings]"><em>Regional settings</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>Regional settings</em>.'
      -
        term: 'Date and time formats'
        definition: '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>.'

Within Twig:

<p>
  {{ introduction }}
  <dl>
    {% for item in settings %}
      <dt>{{ item.term }}</dt>
      <dd>{{ item.definition }}</dd>
    {% endfor %}
  </dl>
</p>

Comment #214

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 28 August 2018 at 23:32

Status:

Reviewed & tested by the community

» Needs work

Sigh, regarding #213. If you look back at the original patches for this module, they were essentially what you're proposing in #113 and they were rejected. No one wanted us to invent that kind of thing. Long story.

Anyway. Looks like we should set this back to Needs Work and redo this again using Twig for the bodies of the help topics. Or maybe just give up on getting it into Core... It seems like someone doesn't like the architecture, no matter what it is, and I'm about done trying to make a patch that will add this to Core. I have no confidence that if we do totally rearchitect this patch to put the body of the help into Twig, that it will make it past review. Someone will most likely have another objection.

Comment #215

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 29 August 2018 at 01:31

I plan to take a look at the resources mentioned in #211 and see if I can comprehend how to update our approach here.

Comment #216

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 29 August 2018 at 04:49

Core committers have discussed the 'html in yaml' several times since comment 175.

We are concerned with inventing a new approach that has limited use outside this system and duplicates concepts we already have.

We're not convinced by the 'you can't have a UI for twig' argument.

We recognize the great efforts that the team working here have gone to, there have been multiple major changes of direction and the team have been 100% accommodating of those requests.

We also recognize that there is a real demand for a topic-based help system that isn't available with the existing help module.

It is because of this demand and the way in which the team here have been so accommodating that we're dedicated to finding a working solution so we can get this improvement in.

I'd like to repeat my offer to work on the twig approach, in a fashion that will enable a UI (initially in contrib) that will facilitate contribution by non technical contributors.

Comment #217

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 29 August 2018 at 05:54

@larowlan I welcome and thank you for your assistance! I am open to any discussion or questions as best as I can answer in the Drupal Slack #contribute or #documentation channel (@ambermatz). I am also open to being given some technical direction and examples and attempting a patch myself. But your help would be much appreciated, as I am admittedly a bit weary from the latest refactor.

Comment #218

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 29 August 2018 at 08:15

@markcarver @effulgensia I really wondered that twig question raised again here when the plan for it was discussed in #2592487: [meta] Help system overhaul

TL'DR twig-based help perfectly fits in current patch architecture - extension of plugin discovery, but that is not 80% case to core UI

Comment #219

effulgentsia CreditAttribution: effulgentsia at Acquia commented 29 August 2018 at 15:07

Sigh, regarding #213. If you look back at the original patches for this module, they were essentially what you're proposing in #113 and they were rejected. No one wanted us to invent that kind of thing.

Can you reference some comment links about that? What I found in a quick scan of the issue is #52, but I don't see how the desire for UI based on a single wysiwyg field instead of one per translatable chunk is satisfied by #191. Did the usability team ever approve a UI based on one wysiwyg field per translatable chunk? If so, then what about #213 is incompatible with that?

Comment #220

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 August 2018 at 15:22

I guess #213 is somewhat different from the early proposals for this module. But maybe not much?

#213 looks like you'd need to define a bunch of plugins (I think they'd be plugins??) that would define "types" you could put into the topics, such as "bullet list" and "paragraph" and "header" and "definition list". That seems kind of similar to the "paragraphs lite" plugins I had proposed early on. That plugin system was deemed to be complicated (maybe it was IRC conversations and didn't make it into comments in this issue, I don't remember) and also suffered from "inventing new stuff for no good reason" criticism, and also it was to support a UI that was rejected (where you edited each translatable chunk separately, unlike other editing UIs). So, we dumped that code.

So I don't think inventing "paragraphs lite" plugins to put into YAML for topics is a good way to move this issue forward. Someone will criticize it for inventing things and making it too complicated, and the patch will get rejected again. That is my prediction anyway, but maybe I am cynical after all the rearchitectures we have already tried to do, which has led us to this point of still not having a patch.

I hope that @larowlan and @Amber Himes Matz can get together and make one more iteration that will finally get past all the review hurdles and make it so we have a way to define help topics for Core and Contrib. We have needed this for decades. It seems to me like the proposal in #197 might be viable. It would require a module to put a hook_theme() entry in for each topic, not just create a YAML file and a Twig template -- which seems *very very weird* to me -- but it would probably at least work. It would make the patch smaller, because we wouldn't have the Token system or the Chunker class, so ... less code to review... hopefully. I just don't personally have the energy to try to refactor this completely one more time. Sorry.

Comment #221

effulgentsia CreditAttribution: effulgentsia at Acquia commented 29 August 2018 at 15:32

it was to support a UI that was rejected (where you edited each translatable chunk separately

Is there a working UI somewhere that outputs the structure of #191 where you don't edit each translatable chunk separately?

Comment #222

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 August 2018 at 16:13

Yes. The currently sandbox module does that. https://www.drupal.org/sandbox/jhodgdon/2369943

It should be in a workable state. It creates config entities that have the exact schema/structure of the plugins in #191, via the HtmlChunker class that is in this patch.

If the patch in #191 were accepted into Core, we would take parts that made it into this patch out of the Sandbox project and work on getting it to a release as a full Contrib project. That project would then have the following features:
- Give you a UI for editing topics, aimed at site builders who want to write help for their particular site setup
- The topics would save as config entities
- The config entities have a deriver that makes them into topic plugins, so they display along with the topics provided by modules/themes (from this patch's functionality)
- You could export the entities, remove 2 lines from the export, and put them in the right directory to have plugin topics for a module or theme, thereby giving core/contrib contributors a UI for authoring topics without having to write YAML

But currently, the Sandbox project has essentially this patch (minus recent revisions that have taken place on this issue and not made it into the Sandbox) plus the other stuff.

Comment #223

effulgentsia CreditAttribution: effulgentsia at Acquia commented 29 August 2018 at 17:04

File	Size
help_ui.png	35.6 KB
help_yaml.png	62.26 KB

Re #222, wow, that's pretty cool. Here's some screenshots of that, in case it helps others who join this issue without already knowing all of its history:

The sandbox UI:

Shows editing a help topic within a single wysiwyg field

The generated YAML:

Shows the generated YAML as containing multiple separate translatable text chunks

Comment #224

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 August 2018 at 17:16

Well, I think it's cool. :) But there is some code behind it (the HtmlChunker chunk/join methods), and the current reviews seem to be saying that we shouldn't be inventing this chunker etc. or putting the HTML into YAML.

Comment #225

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 29 August 2018 at 21:03

Assigned:

Unassigned

» larowlan

working on this a bit

Comment #226

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 29 August 2018 at 22:22

@larowlan asked for the context above, #2820166: Flexible plugin based help system is the original proposal I wrote a while ago.

In general I'm a bit confused about this statement:

We're not convinced by the 'you can't have a UI for twig' argument.

I'm quite convinced about this point in general. We do already have a way in core to store twig templates in a sandboxed environment (views), so why could we not do the same here as well?

Comment #227

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 29 August 2018 at 23:53

@dawehner Re:

@dawehner: In general I'm a bit confused about this statement:

@larowlan: We're not convinced by the 'you can't have a UI for twig' argument.

I'm quite convinced about this point in general. We do already have a way in core to store twig templates in a sandboxed environment (views), so why could we not do the same here as well?

I took @larowlan's statement to mean the same as what you're saying: that using Twig templates to display the HTML of a Help Topic plugin doesn't mean we can't also have a UI to edit the text of a Help Topic (in contrib for starters).

What I'm gathering is that the direction we want to explore now is using Twig for the body text of a help topic plugin, which I believe is what @larowlan is working on now.

Edit: formatting

Comment #228

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 30 August 2018 at 00:54

Yes, I'm saying using twig doesn't preclude people from editing, thanks.

Comment #229

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 30 August 2018 at 05:22

Issue summary:

-Needs framework manager review

File	Size
interdiff-ht.txt	11.46 KB
help-topics-2920309-229.patch	80.46 KB

Screen Shot 2018-08-30 at 3.20.34 pm.png	94.72 KB

here's a sample, i converted the config_basic help topic to twig

there is one issues

trans doesn't support {{ url() }}, but I think it could for simple urls, so I had to use a set, and then a render_var to turn it into a string.

whilst that works, its not ideal for making this accessible

i didn't convert anything else yet, just wanted to share

the hook_theme is auto, so its really just a twig file that follows help-topic-{plugin-id} naming and the yml file

screenshot showing the page

Assuming we could extend the twig trans parser to support url, it would get a lot simpler.

Thoughts?

Comment #230

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 30 August 2018 at 05:23

Of note, the controller now just calls $plugin->getBody() so that means the chunking logic could live in an alternate plugin class if so desired.

Comment #231

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 30 August 2018 at 13:39

This looks provisionally good!

A few notes:
a) You'll want to change the doc block for the HelpTopicPluginInterface::getBody() function to say something about it returning a render array instead of a string, I think?

b) Take body_format out of the standard plugin definition here:

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -38,7 +38,7 @@ class HelpTopicPluginManager extends DefaultPluginManager implements HelpTopicPl
     'body' => [],
     // The machine name of the text format for the body in HTML form.
     'body_format' => [],

This is in HelpTopicTwig -- should be @return $static not return $this.
https://www.drupal.org/node/1354#types
<blockquote>
When you are returning the main class object ($this), use @return $this.
When creating a new instance of the same class, use @return static.
</blockquote>

d) in HelpTopicTwig:
<code>
+  /**
+   * Gets theme hook from plugin ID.
+   *
+   * @param string $pluginId
+   *   Plugin id.
+   *
+   * @return string
+   *   Theme hook.
+   */

Should be

+  /**
+   * Gets the theme hook from the plugin ID.
+   *
+   * @param string $pluginId
+   *   Plugin ID.
+   *
+   * @return string
+   *   Theme hook.
+   */

Also don't we use underscores for local variables, so the input should be $plugin_id?

e) in Twig template:

+  <dd>{% trans %}
+      On the <a href="{{ information_url }}"><em>Basic site settings</em></a> page, which you can reach in the main <em>Manage</em> administrative menu, by navigating to <em>Configuration</em> &gt; <em>System</em> &gt; <em>Basic site settings</em>.
+    {% endtrans %}</dd>

Does this put extra newlines into the translation string at beginning and end? It looks like it would. And if so, I wouldn't do that. It would be really annoying on localize.drupal.org.

f) If you look at the core hook_help() functions, you'll see we normally pass URLs as placeholders to t(), so I think this is probably OK? I am assuming that what the POTX gets will look literally like what is in the Twig file, so things like information_url will be put in at the last minute as placeholders into the translation, which is what we want to happen. And hopefully the evaluation of the URL calls will happen at render time, so the URLs will get the correct page language. But we should verify that this works, and at least look at what gets into the local translation database for these strings.

It would be good to have an example for the URL for another topic...

Anyway this looks like a great start!

Comment #232

effulgentsia CreditAttribution: effulgentsia at Acquia commented 30 August 2018 at 16:28

@larowlan: What changes would you need to make to https://www.drupal.org/sandbox/jhodgdon/2369943 to support #229? Should we open an issue there to explore that?

Comment #233

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 30 August 2018 at 16:35

All we'd need to do in the Sandbox is
a) rip out the parts that are in Core (plugin base class, plugin manager, etc.)
b) write a few lines of code so that getBody() on the plugin makes a render array (some logic is currently in the controller in the Sandbox, where it gets the body as a string, runs token replace, and makes a render array -- that will need to move to the Plugin that supports the config entities instead).

I'm not concerned at all about doing that. Should be relatively easy. The basic structure of plugins/derivers is not changing in this patch, and the base plugin class isn't changing much (mostly the getBody() method has a new behavior/signature, as noted in #231 (a).

But let's wait until/if this gets into Core...

Comment #234

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 30 August 2018 at 22:26

Assigned:

larowlan

» Unassigned

I am assuming that what the POTX gets will look literally like what is in the Twig file, so things like information_url will be put in at the last minute as placeholders into the translation

yes TwigTransNode ends up with t({String}, {tokens}) however the url token comes out as '@url' instead of ':url', so that's another reason why we should add support for the url() function to the trans token parsing, we'll be able to distinguish url variables from normal ones. I'll open an issue for that, its a followup.

So left is to

address review from @jhodgdon at #231
convert the other plugins to twig
remove the unused parts (joiner/splitter)
adjust test coverage as appropriate

Will see if I can get to that today, will unassign for now in case someone else has time, and reassign when I pick it up again.

One final question - do we want the templates to be in /templates in the module, with other twig templates or do we want to split them into a help-templates or help-topics folder so that there is some separation?

Comment #235

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 30 August 2018 at 23:28

Opened #2996305: Add support for the url() function to twig {%trans%}

Comment #236

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 31 August 2018 at 14:53

It seems like for module developers, the easiest/nicest thing would be to put the YAML files in help_topics and and the corresponding Twig templates into help_topics/templates.

However, wouldn't that make the hook_theme() here pretty complicated? Seems like it would have to tell the theme system where to find said templates?

Also... hm... Will this hook_theme() even work with templates that are provided by a different module? Because the help_topics or help module will be doing the hook_theme(), so wouldn't that be where the theme system would look for the base Twig template? Seems problematic.

Anyway, hopefully that can be resolved.

And... One other item to address in the patch: the hook_help() that tells module developers how to define new help topic plugins will need to be revised to talk about the Twig templates they need to write.

Comment #237

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 3 September 2018 at 07:16

However, wouldn't that make the hook_theme() here pretty complicated? Seems like it would have to tell the theme system where to find said templates?

hook_theme accepts a path, and we have the module in scope as 'provider' for each plugin.

It feels achievable.

Also... hm... Will this hook_theme() even work with templates that are provided by a different module? Because the help_topics or help module will be doing the hook_theme(), so wouldn't that be where the theme system would look for the base Twig template?

Yep, that's a bug in the current hook_theme in help_topics.module, but should be resolvable as above.

Comment #238

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 8 September 2018 at 23:47

Assigned:

Unassigned

» larowlan

working on this again

Comment #239

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 10 September 2018 at 02:20

Assigned:	larowlan	» Unassigned
Status:	Needs work	» Needs review

File	Size
2920309-help-topics-239.interdiff.txt	58.04 KB
2920309-help-topics-239.patch	70.97 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,356 pass

Ok, this should be everything we were waiting on

Comment #240

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 11 September 2018 at 07:19

Great work @larowlan
Here is just a relatively quick review, I really like the direction though, no surprise.

+++ b/core/modules/help_topics/help_topics.module
@@ -0,0 +1,63 @@
+    $return[$hook] = [
+      'variables' => [],
+      'path' => drupal_get_path('module', $definition['provider']) . '/help_topics/templates',
+      'template' => strtr($hook, '_', '-'),
+    ];

❓Is there a reason we would not put the actual theme registry entry into the helper method?

+++ b/core/modules/help_topics/help_topics.permissions.yml
@@ -0,0 +1,2 @@
+view help topics:
+  title: 'View help topics'
diff --git a/core/modules/help_topics/help_topics.routing.yml b/core/modules/help_topics/help_topics.routing.yml

diff --git a/core/modules/help_topics/help_topics.routing.yml b/core/modules/help_topics/help_topics.routing.yml
new file mode 100644

new file mode 100644
index 0000000000..5038440d20

index 0000000000..5038440d20
--- /dev/null

--- /dev/null
+++ b/core/modules/help_topics/help_topics.routing.yml

+++ b/core/modules/help_topics/help_topics.routing.yml
+++ b/core/modules/help_topics/help_topics.routing.yml
@@ -0,0 +1,6 @@

@@ -0,0 +1,6 @@
+help_topics.help_topic:
+  path: '/admin/help/topic/{id}'
+  defaults:
+    _controller: '\Drupal\help_topics\Controller\HelpTopicPluginController::viewHelpTopic'
+  requirements:
+    _permission: 'view help topics'

The existing help pages use "'access administration pages'" as permission. I guess we don't do that to be useful for non admin users as well?

+++ b/core/modules/help_topics/help_topics.tokens.inc
@@ -0,0 +1,69 @@
+    'name' => t("Help Topics"),
+    'description' => t("Tokens for help topics."),

🙃Nitpick: Let's use single quotes.

+++ b/core/modules/help_topics/help_topics.tokens.inc
--- /dev/null
+++ b/core/modules/help_topics/help_topics/config_basic.help_topic.yml

I'm a bit confused about the logic/magic to find the right twig template. Would there be a point in making it explicit rather than implicit?

+++ b/core/modules/help_topics/help_topics/security_account_settings.help_topic.yml
index 0000000000..1a5b686c97
--- /dev/null

--- /dev/null
+++ b/core/modules/help_topics/help_topics/templates/help-topic-config-basic.html.twig

+++ b/core/modules/help_topics/help_topics/templates/help-topic-config-basic.html.twig
+++ b/core/modules/help_topics/help_topics/templates/help-topic-config-basic.html.twig
@@ -0,0 +1,12 @@

@@ -0,0 +1,12 @@
+{% set regional_url = render_var(url('system.regional_settings')) %}
+{% set information_url = render_var(url('system.site_information_settings')) %}
+{% set datetime_url = render_var(url('entity.date_format.collection')) %}
+<p>{% trans %}The settings for your site are configured on various administrative pages, as follows:{% endtrans %}</p>
+<dl>
+  <dt>{% trans %}Site name, slogan, and email address{% endtrans %}</dt>
+  <dd>{% trans %}On the <a href="{{ information_url }}"><em>Basic site settings</em></a> page, which you can reach in the main <em>Manage</em> administrative menu, by navigating to <em>Configuration</em> &gt; <em>System</em> &gt; <em>Basic site settings</em>.{% endtrans %}</dd>
+  <dt>{% trans %}Time zone and country{% endtrans %}</dt>
+  <dd>{% trans %}On the <a href="{{ regional_url }}"><em>Regional settings</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>Regional settings</em>.{% endtrans %}</dd>
+  <dt>{% trans %}Date and time formats{% endtrans %}</dt>
+  <dd>{% trans %}On the <a href="{{ datetime_url }}"><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>.{% endtrans %}</dd>
+</dl>

This looks so nice!

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,145 @@
+        /** @var \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginInterface $topic */
+        $topic = $this->helpTopicPluginManager->createInstance($other_id);

I wonder whether we should use assert() instead here

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginInterface.php
@@ -0,0 +1,92 @@
+/**
+ * Defines an interface for help topic plugin classes.
+ */
+interface HelpTopicPluginInterface extends PluginInspectionInterface, DerivativeInspectionInterface, CacheableDependencyInterface {

It would be nice to clarify what a help topic means. I could also imagine this could be a nice place to document that you can also use twig to write those help topics for yourself

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginInterface.php
@@ -0,0 +1,92 @@
+   * @return string
+   *   The HTML-formatted body of the topic in a TranslatableMarkup object.

Is there a reason why this doesn't typehint on TranslatableMarkupObject? To be honest I would rather have expected \Drupal\Component\Render\MarkupInterface

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginInterface.php
@@ -0,0 +1,92 @@
+   * @return bool
+   *   TRUE if this is a topic that should be displayed on the Help topics
+   *   list; FALSE if not.
+   */
+  public function isTopLevel();

👍Nice documentation!

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginInterface.php
@@ -0,0 +1,92 @@
+  /**
+   * Returns the cache tags appropriate for listings of plugins of this type.
+   *
+   * @return string[]
+   *   Array of cache tags.
+   */
+  public function getCacheTagsForList();

I'm curious whether it would make sense to use \Drupal\Core\Cache\CacheableDependencyInterface instead here.

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,228 @@
+  /**
+   * Checks to see if cached plugin definitions exist.
+   * If they do, then it returns the cached definitions.
+   * Otherwise, findDefinitions() is called.
+   *
+   * @see \Drupal\Core\Plugin\DefaultPluginManager::findDefinitions()
+   *
+   * @return array|\mixed[]|null
+   */
+  public function getDefinitions() {
+    return parent::getDefinitions();
+  }
+

Is there a reason we need this?

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,228 @@
+      if (in_array($id, $definition['list_on'])) {

🙃We could use a strict comparison here.

Comment #241

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 17 September 2018 at 01:21

Issue tags:

File	Size
2920309-help-topics-241-interdiff.txt	4.74 KB
2920309-help-topics-241.patch	70.84 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,402 pass, 1 fail

Thanks for the review @dawehner

We need both the hook (for the array key) and the structure. So we'd need either to pass the array by reference, which feels ick, or return an array of return values, which is worse. If you feel strongly about it, I'd go with pass by reference. Please advise.
Question for @jhodgdon and @Amber Himes Matz
Fixed
Do you mean put an entry in the plugin for the template name? That would imply that all topics have a template name, which is undesirable as other plugin implementations may not be template based. I guess we could add a generic settings map. Thoughts?
Thanks
Can you elaborate?
Added some docs
Fixed
All credit to previous contributors there
The plugin interface already extends from that, this is for listing pages, where the title of the plugin is shown on related pages. Kind of like the list cache tags for entities, e.g node_list and so on. At least that's how I interpret that work.
Fixed
Fixed

New patch attached, also cleaned up the no longer used token service in the controller.

Comment #243

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 19 September 2018 at 00:13

Status:

Needs work

» Needs review

File	Size
2920309-help-topics-243-interdiff.txt	699 bytes
2920309-help-topics-243.patch	70.83 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,404 pass

Test fix

Comment #244

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 19 September 2018 at 20:10

Thanks the review @dawehner and patches @larowlan!

Regarding question 2 in comment #240:

The existing help pages use "'access administration pages'" as permission. I guess we don't do that to be useful for non admin users as well?

Good question. I suppose we are assuming that these help topic pages will be viewed in the admin theme. In which case, we should add the permission "access administration pages" like the other help pages.

I don't have time at the moment to do a patch for that, but I can sometime this week. Along with testing the latest patch from @larowlan.

Comment #245

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 19 September 2018 at 20:46

Always improving!

Some thoughts/comments/reviews on the latest patch:

1. In the .module file hook_help():

+      $output .= '<dt>' . t('Providing help topics') . '</dt>';
+      $output .= '<dd>' . t("Modules and themes can provide help topics as YAML-file-based plugins in a project sub-directory called <em>help_topics</em>. Any file in a module or theme's <em>help_topics</em> directory with the suffix <em>*.help_topic.yml</em> will be discovered by the Help Topic module. Plugin-based help topics provided by modules and themes will automatically be updated when a module or theme is updated. Use the plugins in <em>core/modules/help_topics/help_topics</em> as a guide when writing and formatting a help topic plugin for your theme or module.") . '</dd>';

We should mention Twig and/or expand this a bit.

2. I agree with Amber, we can get rid of the specific permission here and just use the generic one, like the Help module does.

3. I thought we were getting rid of tokens in this patch? If so, we should get rid of:
- help_topics.tokens.inc
- HelpTopicTokensTest.php
- The Token service parameters in HelpTopicController constructor. Oh, it's gone, but the doc block for the @param is still there, and the Use statement:

+use Drupal\Core\Utility\Token;
...
+   * @param \Drupal\Core\Utility\Token $token
+   *   The token service.

4) I think we should also mention the Twig templates here:

+/**
+ * Represents a help topic plugin whose definition comes from a YAML file.
+ *
+ * The YAML files are stored in subdirectory help_topics, and must be named
+ * id.help_topic.yaml, where id is the plugin ID.
+ */
+class HelpTopicTwig extends HelpTopicPluginBase implements ContainerFactoryPluginInterface {

+/**
+ * Defines an interface for help topic plugin classes.
+ *
+ * Help topics can be created by adding a help_topics folder to your module.
+ * Each help topic has a {topic_id}.help_topic.yml file and a corresponding twig
+ * template inside help_topics/templates. The twig file should be named
+ * help-topic-{topic-id}.html.twig, where {topic-id} is the topic ID used in the
+ * YAML file, with the underscores swapped for hyphens. For example:
+ * help_topics/basic_pages.help_topic.yml would have a corresponding
+ * help_topics/templates/help-topic-basic-pages.html.twig.
+ */
+interface HelpTopicPluginInterface extends PluginInspectionInterface, DerivativeInspectionInterface, CacheableDependencyInterface {

I see we mention Twig here. Maybe the HelpTopicTwig class can link here... or better yet, move this documentation to the HelpTopicTwig class (since it's specific to this class, and not to the interface, which could be used for the config-based plugin as well)? Maybe link from the interface to the class?

6) Also nitpick: Twig should be capitalized in the docs in item 5.

7) Here's another place where we're not completely documenting what module developers need to do:

+ * Modules and themes can provide help topics in YAML files called
+ * name_of_topic.help_topic.yml inside the module or theme sub-directory
+ * help_topics.
+ */
+class HelpTopicPluginManager extends DefaultPluginManager implements HelpTopicPluginManagerInterface {

Again... I think we should put the docs in 1 place only, and link between them.

8) Someone should probably look at all the Twig-based topics and verify they all look good in the UI, and that the links work, etc. -- they've been rewritten in Twig since I made them as YAML files. :)

Comment #246

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 10 October 2018 at 18:45

Status:

Needs review

» Needs work

This issue seems to be stalled. I'm going to go ahead and set it to Needs Work on the basis of my last comment. @larowlan -- are you planning to take this to completion, or do we need to find someone else to do that?

Comment #247

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick at Acquia commented 10 October 2018 at 18:48

Priority:

Normal

» Major

Going to bump this to major as well, since the lack of topic-based help has come up in literally every single usability study we've done over the past decade. The people who are likely to consult the help system for guidance are also generally NOT the same subset of people with Einstein-level understanding of which modules provide which functionality.

Comment #248

dawehner

German

CreditAttribution: dawehner at Chapter Three commented 11 October 2018 at 23:41

I'm wondering, why is there all this token code in there? It feels like this is not longer needed after the rewrite of @larowlan?

One thing I'm wondering, should we add some documentation around the idea that the old help module will be replaced by this one, which is kind of my understanding at this point.

Comment #249

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 12 October 2018 at 13:23

Yeah, the token code must go, see my last review of this patch, #245 item 3.

And I do not think we are replacing hook_help() at this point. This is an addition. But I think we do want it to be part of the one and only Help module eventually. The question is whether to do it now or later. If it's "experimental", we should defer, probably? That is really up to the Core Committers.

Comment #250

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 30 October 2018 at 21:49

Status:

Needs work

» Needs review

File	Size
interdiff-2920309-250.txt	13.18 KB
2920309-250.patch	451.01 KB
8.7.x: PHP 7.2 & MySQL 5.5 CI aborted

Addressed #245

Comment #251

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 30 October 2018 at 21:51

File	Size
2920309-250.patch	65.05 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,546 pass

Do you even merge origin larowlan?

Comment #253

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 30 October 2018 at 23:19

Status:

Needs review

» Needs work

Looking much better, thanks! I took a very careful look at this patch, with fairly fresh eyes since it had been a while since the previous patch. I have a few suggestions:

a) In help_topics.module hook_help implementation:

... YAML-file-based plugins in a project sub-directory called <em>help_topics</em> the contents of the topic are contained in a matching twig file.  ...

1. This is a typo -- needs to be two sentences.

2. Also Twig should be capitalized.

3. I think we should say where the Twig files need to be placed, since it's in help_topics/templates, not the main templates directory.

b) Also in help_topics.module:

+function help_topics_theme($existing, $type, $theme, $path) {
+  $twigPlugins = array_filter(\Drupal::service('plugin.manager.help_topic')->getDefinitions(), function (array $definition) {
+    return $definition['class'] === HelpTopicTwig::class;
+  });
...

1. If it were me writing this patch, I think I would have put this function on the help plugin manager as a public method, something like listTwigPlugins() or something like that. Or even make a method that does what this function does as a whole, called listTwigTemplates().

2. Do we really want to just match classes with HelpTopicTwig, or also allow subclasses here? I guess maybe matching is all we can do without instantiating, and if some module makes a subclass, they can be responsible for doing their own hook_theme(). So... OK.

c) In src/Plugin/HelpSection/HelpTopicSection.php :

+  /**
+   * {@inheritdoc}
+   */
+  public function getCacheContexts() {
+    // The links are checked for user access, so we need the user permissions
+    // context.
+    return ['user.permissions'];
+  }

In my earlier patches where topics were configuration, we were checking links for admin/help for user access, but I think in this implementation, we are not checking access, so I think this should be removed? [If so, there's a test that is testing it that will need to be modified -- tests/src/Functional/HelpTopicTest.php ].

d) In src/Plugin/HelpTopic/HelpTopicPluginManager.php :

+  public function getTopLevelTopics() {
+    $topics = [];
+
+    foreach ($this->getDefinitions() as $definition) {
+      if ($definition['top_level']) {
+        /* @var \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginInterface $topic */
+        $topic = $this->createInstance($definition['id']);
+        $label = (string) $topic->getLabel();
+        $topics[$label] = $topic;
+      }
+    }

1. It occurs to me that it's possible there is more than one topic with the same title. So probably doing $topics[label] = $topic is not too great. Maybe for purposes of array keys, append the topic ID to the label, just to make sure we don't clobber topics with the same name?

2. This same logic applies to HelpTopicPluginManager::findMatches -- although this is documented to return a list of IDs keyed by title, so... not much to do about it.

e) In src/Plugin/HelpTopic/HelpTopicTwig.php :

+class HelpTopicTwig extends HelpTopicPluginBase implements ContainerFactoryPluginInterface {
+
+  /**
+   * Constructs a HelpTopicDefaultPlugin.

Should be "Constructs a HelpTopicTwig.".

f) Same file:

+  /**
+   * Gets the theme hook from then plugin ID.

Typo: "from then" should be "from the".

+++ b/core/modules/help_topics/templates/help-topic.html.twig
@@ -0,0 +1,16 @@
+{#
+/**
+ * @file
+ * Default theme implementation to display a help topic.
+ *
+ * Available variables:
+ *  - body: The body of the topic.
+ *  - related: List of related topic links.

Nitpick: the - for list items under "Available variables" should start under the A of Available. There is an extra space.

h) The permissions seem a little inconsistent:

+++ b/core/modules/help_topics/help_topics.permissions.yml
+view help topics:
+  title: 'View help topics'

...

+++ b/core/modules/help_topics/help_topics.routing.yml
+help_topics.help_topic:
+  path: '/admin/help/topic/{id}'
+  defaults:
+    _controller: '\Drupal\help_topics\Controller\HelpTopicPluginController::viewHelpTopic'
+  requirements:
+    _permission: 'access administration pages'

...

+++ b/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php
+ * @HelpSection(
+ *   id = "help_topics",
+ *   title = @Translation("Topics"),
+ *   description = @Translation("Topics can be provided by modules or themes. Top-level help topics on your site:"),
+ *   permission = "view help topics"
+ * )
+ */
+class HelpTopicSection extends HelpSectionPluginBase implements ContainerFactoryPluginInterface {

I don't understand why we have a "view help topics" permission defined, which is only used for the section on admin/help, but each help topic can be viewed by anyone with 'access administration pages" permission? I think we should just get rid of that view help topics permission?

Comment #254

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 30 October 2018 at 23:21

Also the test bot found one coding standards issue:

src/Plugin/HelpTopic/HelpTopicPluginBase.php
line 9	Unused use statement

Comment #255

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 2 November 2018 at 01:43

Assigned:

Unassigned

» Amber Himes Matz

I am working on a new patch that incorporates @jhodgdon's feedback in #253 and #254.

Comment #256

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 3 November 2018 at 06:38

File	Size
2920309-help-topics-256.patch	64.8 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,590 pass

Here's a new patch with the patch from #250 applied and then incorporating the changes that @jhodgdon suggested/requested in #253 and #254. (Sorry no interdiff as I got punched by Composer and had to re-do my changes...anyway...sigh.)

A) core/modules/help_topics/help_topics.module

1. Copy edits and added mention of help_topics/templates as the location for the Twig files for help topic plugins in hook_help.

2. The biggest change I made was based on @jhodgdon's comment in #253 "b. 1)" about hook_theme in help_topics.module. I poked around and found that core/layout_discovery's hook_theme in its layout_discovery.module returns a getThemeImplementations() method from its plugin manager. So I did something similar here as well and added a getThemeImplementations() to the HelpTopicPluginManager and returned that in the hook_theme implementation in help_topic.module.

B) core/modules/help_topics/help_topics.permissions.yml

1. Deleted. Because we decided we didn't need a separate 'view help topics' permission and could use 'access administration pages' instead.

C) core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php

1. Changed permission to 'access administration pages'.

2. Removed public function getCacheContexts() which returned ['user.permissions'], which I understood was no longer needed.

D) core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php

1. In getTopLevelTopics(), I appended the plugin id to the key here to ensure uniqueness, since it is possible that help topics could have the same title.

2. Added public function getThemeImplementations()

E) core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManagerInterface.php

1. Adds getThemeImplementations() to interface

F) core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicTwig.php

1. Corrected comment text to "Constructs a HelpTopicTwig."

2. Copy edit on getBody() comment.

G) core/modules/help_topics/templates/help-topic.html.twig

1. Comment formatting (removed leading space from list items of variables)

H) core/modules/help_topics/tests/src/Functional/HelpTopicTest.php

1. Removed mention of 'view help topics' from setUp()

2. Removed cacheContexts assertion since that function has been removed.

Comment #258

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 3 November 2018 at 18:58

It occurs to me that getThemeImplementations() isn’t the right name for what it is doing. I’ll work on a subsequent patch to change that.

Comment #259

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 4 November 2018 at 00:07

This looks great (the recent patch)!

Suggestion for the docs/function name for getThemeImplementations(), which is documented in the HelpTopicPluginManagerInterface like this:

+  /**
+   * Gets theme implementations for layouts.
+   *
+   * @return array
+   *   An associative array of the same format as returned by hook_theme().
+   *
+   * @see hook_theme()
+   */
+  public function getThemeImplementations();

a) It's not "for layouts" in the first line (this must have been copy/pasted from somewhere else?).

b) Looking at the docs for hook_theme() itself...
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

I think the first line could say "Gets information about theme implementations for help topics.", as that is consistent with the docs for the return value of hook_theme(). If that will fit on one line. If not, maybe shorten to "info"?

Then maybe the function could be called getThemeInfo()? But given the docs for hook_theme(), I think getThemeImplementations() is fine. Especially if the same function in Layouts is called that -- let's keep them consistent with each other.

c) Cool that the Layouts team had the same idea of putting it into the plugin manager. :)

Anyway, the rest of the changes look good! Thanks!!

Comment #260

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 4 November 2018 at 00:09

By the way, someone (maybe me tomorrow) still I think needs to make time to read (a) the hook_help() output and (b) all the provided help topics, to make sure they are formatted OK and the links work.

Comment #261

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 4 November 2018 at 00:11

Thank you for the review! Yes, copy/pasta fail. I will fix that up with a new patch. Thanks for the suggestions about the docblock and function name.

Comment #262

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 4 November 2018 at 15:39

Issue summary:

I just applied the patch in #256 in simplytest.me, and reviewed what I could see in the UI.

Some things to address:

1. On admin/help, the list of help topics is appearing below the list of hook_help() output. I thought we wanted help topics to be above?

2. There is a link from the hook_help() to https://www.drupal.org/modules/help_topics -- this is standard for all Core modules -- we need to write this page on drupal.org.

That's it -- all the topics look good to me, and are formatted correctly. I'm going to take a look through the issue summary now and update anything that's outdated, and add item 2 here to the summary if it isn't there already.

Oh yeah... item 1 is already a separate issue that is in the issue summary here: #2994748: Make a way for Help Page Sections to be ordered, and item 2 is already there. So... there's nothing new to do from my review of the topics in the UI. They look good! And issue summary has been updated.

Comment #263

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 6 November 2018 at 02:43

File	Size
interdiff-2920309-263.txt	709 bytes
2920309-help-topics-263.patch	64.82 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,594 pass

Latest patch (and interdiff) includes a correction for the docblock in the HelpTopicPluginManagerInterface for getThemeImplementations as well as all of the work done by @larowlan in his patch in #251 and the suggestions by @jhodgdon in her recent reviews.

I think we're ready for some final reviews and testing with an eye towards can this be RTBC'd?

Comment #265

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 7 November 2018 at 11:43

24 files were hidden/shown/deleted

File	Size
2920309-124.patch	184.8 KB
8.6.x: PHP 7 & MySQL 5.5 24,134 pass
interdiff-125-to-141.txt	123.01 KB
interdiff-comment-158-162.txt	501 bytes
help_topics-api_only-162.patch	79.92 KB
8.6.x: PHP 7 & MySQL 5.5 24,159 pass
interdiff-comment-162-166.txt	3.06 KB
help_topics-api_only-166.patch	80.31 KB
8.6.x: PHP 7 & MySQL 5.5 24,308 pass 8.7.x: PHP 7 & MySQL 5.5 24,280 pass
help_topics_hook_help_screenshot.png	314.74 KB
interdiff-comment-166-191.txt	1.94 KB
help_topics-api_only-191.patch	80.91 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,344 pass
help_ui.png	35.6 KB
help_yaml.png	62.26 KB
interdiff-ht.txt	11.46 KB
help-topics-2920309-229.patch	80.46 KB

Screen Shot 2018-08-30 at 3.20.34 pm.png	94.72 KB
2920309-help-topics-239.interdiff.txt	58.04 KB
2920309-help-topics-239.patch	70.97 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,356 pass
2920309-help-topics-241-interdiff.txt	4.74 KB
2920309-help-topics-241.patch	70.84 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,402 pass, 1 fail
2920309-help-topics-243-interdiff.txt	699 bytes
2920309-help-topics-243.patch	70.83 KB
8.7.x: PHP 7.1 & MySQL 5.7 24,404 pass
interdiff-2920309-250.txt	13.18 KB
2920309-250.patch	451.01 KB
8.7.x: PHP 7.2 & MySQL 5.5 CI aborted
2920309-250.patch	65.05 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,546 pass
2920309-help-topics-256.patch	64.8 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,590 pass

Mostly minor nits

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,132 @@
+        if (!$this->helpTopicPluginManager->hasDefinition($other_id)) {
+          continue;

as of PHP 7.3 it should be break... https://www.drupal.org/node/3011831

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,132 @@
+        $topic = $this->helpTopicPluginManager->createInstance($other_id);
+        if ($topic) {

this condition is not needed, cos you check existence above otherwise `PluginException` will be thrown

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,132 @@
+      $build['#related'] = [
+        '#theme' => 'links',

I think it needs to be kinda `links__related`

+++ b/core/modules/help_topics/src/HelpBreadcrumbBuilder.php
@@ -0,0 +1,49 @@
+    $breadcrumb->addLink(Link::createFromRoute($this->t('Help'), 'help.main'));
+    $breadcrumb->addCacheContexts(['route.name']);

as current topic is not here and the list is static, no reason to vary by route

+++ b/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php
@@ -0,0 +1,113 @@
+  protected $topicList;
...
+  protected $cacheTagList;

does it need dependencySerializationTrait?

Comment #266

dqd

(formerly "diqidoq")

CreditAttribution: dqd as a volunteer and at MAROQQO studios commented 7 November 2018 at 12:06

File	Size
illustration_help_topics_87x.png	91.75 KB

as of PHP 7.3 it should be break... https://www.drupal.org/node/3011831

oh(!) yes, 8.6.x spits a mass of warnings regarding this when testing on php 7.3 ...

Apart from that: Impressive work on this! SimpleTest.me run cleanly without flaws. +1 for RTBC from me after added recommendations from #265.

(I added the screenshot result of simpletest.me as an illustration for others to get what this issue is about.)

Comment #267

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented 7 November 2018 at 13:22

re #265.1

as of PHP 7.3 it should be break...

Why exactly? The instance mentioned doesn't involve a switch statement. This continue is targeting a foreach.

Comment #268

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext commented 10 January 2019 at 06:24

Really nice work @Amber Himes Matz, @jhodgdon and @larowlan this is looking close. Here is another review. Feel free to move this to follow-ups

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,132 @@
+      if ($other_id !== $help_topic->getPluginId()) {

s/$help_topic->getPluginId()/$id

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,132 @@
+        $topic = $this->helpTopicPluginManager->createInstance($other_id);

Let's move this outside the loop and use createInstances instead.

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,132 @@
+    foreach ($liston as $topic) {

s/$liston/$list_on

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,132 @@
+      uasort($links, function ($a, $b) {
+        if ($a['title'] == $b['title']) {
+          return 0;
+        }
+        return ($a['title'] < $b['title']) ? -1 : 1;
+      });

Should we make a private function?

+++ b/core/modules/help_topics/src/HelpBreadcrumbBuilder.php
@@ -0,0 +1,49 @@
+    return $breadcrumb;

We don't have tests for this.

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,243 @@
+    return $return;

Should we statically cache this?

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicTwig.php
@@ -0,0 +1,86 @@
+    return 'help_topic_' . $plugin_id;

Plugin ID can have : in them should we sanitize this?

+++ b/core/modules/help_topics/tests/modules/help_topics_test/help_topics/templates/help-topic-help-test.html.twig
@@ -0,0 +1,2 @@
+{% set help_topic_url = render_var(url('help_topics.help_topic', {id: 'help_topic_writing'})) %}

I know this is an open bug but should we add preprocess for this?

Comment #272

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 8 November 2018 at 07:26

File	Size
interdiff-2920309-272.txt	2.72 KB
2920309-help-topics-272.patch	64.72 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,596 pass

Thank you @andypost for the review in comment #265. I don't know about your #5. Maybe @larowlan can answer? Attached patch includes edits for your #1-4 items.

Thank you @jibran for your review in comment #268. Attached patch includes edits for your #1 and 3.

Re: #2

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,132 @@
+ $topic = $this->helpTopicPluginManager->createInstance($other_id);

Let's move this outside the loop and use createInstances instead.

Why? Please explain?

Re: #4

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,132 @@
+ uasort($links, function ($a, $b) {
+ if ($a['title'] == $b['title']) {
+ return 0;
+ }
+ return ($a['title'] < $b['title']) ? -1 : 1;
+ });

Should we make a private function?

Must we? So tired. Any other folks agree here? If there are some +1s to this, then ok.

Re: #5

+++ b/core/modules/help_topics/src/HelpBreadcrumbBuilder.php
@@ -0,0 +1,49 @@
+ return $breadcrumb;

We don't have tests for this.

Ok. This is not my strength. If @jhodgdon or @larowlan or someone else can do this, I would be grateful.

Re: #6

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,243 @@
+ return $return;
Should we statically cache this?

I don't know? Can someone give me some code and I'll add it to a subsequent patch, please and thank you.

Re: #7

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicTwig.php
@@ -0,0 +1,86 @@
+ return 'help_topic_' . $plugin_id;

Plugin ID can have : in them should we sanitize this?

This seems like an important thing to do. Is there a built-in sanitization function we can use here? I couldn't readily tell which one would be appropriate.

Re: #8

+++ b/core/modules/help_topics/tests/modules/help_topics_test/help_topics/templates/help-topic-help-test.html.twig
@@ -0,0 +1,2 @@
+{% set help_topic_url = render_var(url('help_topics.help_topic', {id: 'help_topic_writing'})) %}

I know this is an open bug but should we add preprocess for this?

I don't know. Does @larowlan or @jhodgdon or anyone else have an opinion about this or +1?

It's late, so I'd appreciate some fresh eyes on this patch and if you have answers to any of the questions raised in these latest reviews, I sure would appreciate as specific guidance as you are willing to provide! Thank you! Patch and interdiff attached.

Comment #273

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 8 November 2018 at 16:27

Regarding testing the breadcrumbs (@jibran #5) -- I think we could add a few lines to
core/modules/help_topics/tests/src/Functional/HelpTopicTest.php
that would test the breadcrumbs. We would just need to:
- Place the breadcrumb block (I think it's a block?) in the setUp() function
- Verify that links exist on a help topic page to Admin and Help pages, with the correct titles that are created by the breadcrumb code.

Regarding @jibran #6 -- this is asking if we should static cache I think the return value of the getThemeImplementations() method. I think that's kind of unnecessary, since it should only be called by the theme system, and the theme system has quite a lot of caching already.

Regarding @jibran #7 -- Can plugin IDs really contain : in them? If they're discovered via YAML file names, as these are? I think we should document that for help topic plugins, the IDs/file names should only contain letters, numbers, and _, and maybe the plugin manager should enforce that by filtering out anything that has something else in it? Sigh. More coding to do. But that is probably the right answer.

Regarding @jibran #8 -- let's not force this patch to do everything that other Core issues need to do, or hold it up on fixing other Core issues.

Comment #274

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 8 November 2018 at 21:21

I will attempt to add the test for breadcrumbs. Thanks @jhodgdon for the guidance there. (But if someone beats me to it, I won't complain!) :D

Comment #275

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 8 November 2018 at 22:55

File	Size
interdiff-2920309-275.txt	1.12 KB
2920309-help-topics-275.patch	64.89 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,602 pass, 1 fail

Added testing for breadcrumbs. Patch and interdiff attached.

Locally, I got the following error/notice when I ran the tests:

ERRORS!
Tests: 1, Assertions: 7, Errors: 1.

Remaining deprecation notices (1)

1x: Passing a Session object to the ExpectationException constructor is deprecated as of Mink 1.7. Pass the driver instead.
1x in HelpTopicTest::testHelp from Drupal\Tests\help_topics\Functional

But we're not throwing an ExpectationException in this file, so I think it must be someone else's problem?

Comment #277

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 9 November 2018 at 01:51

Assigned:	Unassigned	» Amber Himes Matz
Status:	Needs work	» Needs review

Working on the breadcrumb test. (First attempt didn't work.) Have an example to go from at core/modules/system/tests/src/Functional/Menu/BreadcrumbTest.php

Comment #278

https://cgit.drupalcode.org/entity_hierarchy/tree/entity_hierarchy.links...
https://cgit.drupalcode.org/entity_hierarchy/tree/src/Plugin/Derivative/...

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 9 November 2018 at 02:17

Assigned:	Amber Himes Matz	» Unassigned
Status:	Needs review	» Needs work

File	Size
interdiff-2920309-comments-272-278.txt	1.6 KB
2920309-help-topics-278.patch	65.33 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,604 pass

This patch includes a (passing on local, yay!) breadcrumb test to our HelpTopicsTest class (see interdiff).

What remains (pending further reviews or responses) is via @jhodgdon in #273:

Regarding @jibran #7 -- Can plugin IDs really contain : in them? If they're discovered via YAML file names, as these are? I think we should document that for help topic plugins, the IDs/file names should only contain letters, numbers, and _, and maybe the plugin manager should enforce that by filtering out anything that has something else in it? Sigh. More coding to do. But that is probably the right answer.

Does someone have time to take that on? @larowlan or @jhodgdon?

Comment #279

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 9 November 2018 at 03:25

Discussed with jibran, he's right - yaml based plugin discovery can do derivative discovery which would yield : in the plugin IDs, he even pointed me to some code I wrote that does that!

Comment #280

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 9 November 2018 at 18:04

Oh right, derivatives... That would be a problem for the configurable help derivative (the original sandbox module that will probably be made into a contrib module if we ever get this into Core), because it will be a derivative. It also would not want the Twig templates.

So I think if things are derivatives, we don't want to have them assume they have a Twig template. We should only have Twig templates for the HelpTopicTwig class itself.

So, in this code

+  public function getThemeImplementations() {
+    $twigPlugins = array_filter($this->getDefinitions(), function (array $definition) {
+      return $definition['class'] === HelpTopicTwig::class;
+    });
+    $return = [
+      'help_topic' => [
+        'variables' => [
+          'body' => [],
+          'related' => [],
+        ],
+      ],
+    ];
+    foreach ($twigPlugins as $pluginId => $definition) {
+      $hook = HelpTopicTwig::getThemeHook($pluginId);
+      $return[$hook] = [
+        'variables' => [],
+        'path' => drupal_get_path('module', $definition['provider']) . '/help_topics/templates',
+        'template' => strtr($hook, '_', '-'),
+      ];
+    }
+    return $return;
+  }
+
+}

We are actually already doing that. We're only assuming there is a Twig template if the class of the plugin is exactly equal to HelpTopicTwig::class

Therefore, there is not a possibility that there is a : in the name, because it's not a derivative. The only problem would be if some other module tried to make a derivative that used the same exact class HelpTopicTwig. Which seems unlikely, but to ensure that isn't somehow done we should document in the doc block of HelpTopicTwig that it cannot be used as-is for a derivative... let's see, the doc block already says this:

+/**
+ * Represents a help topic plugin whose definition comes from a YAML file.
+ *
+ * The YAML files are stored in subdirectory help_topics, and must be named
+ * id.help_topic.yaml, where id is the plugin ID. Each YAML file has a
+ * corresponding Twig template inside help_topics/templates. The Twig file
+ * should be named help-topic-{topic-id}.html.twig, where {topic-id} is the
+ * topic ID used in the YAML file, with the underscores swapped for hyphens.
+ * For example: help_topics/basic_pages.help_topic.yml would have a
+ * corresponding help_topics/templates/help-topic-basic-pages.html.twig.
+ */
+class HelpTopicTwig extends HelpTopicPluginBase implements ContainerFactoryPluginInterface {

Let's add something like this:

This class cannot be used as-is for a derivative plugin, because the plugin ID would include a :, which makes it unusable as a theme hook and/or Twig template name. If you want to make a derivative plugin in your own module, you would need to:
- Extend this class and override the getThemeHook() method, to make a valid theme hook name out of the plugin ID (remove the : at a minimum).
- Implement hook_theme() in your module, to tell the theme system about your derivative's theme hooks. See help_topics_theme() and \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginManager::getThemeImplementations() for an example of how to do this.

Thoughts?

Comment #281

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 9 November 2018 at 23:36

Status:

Needs work

» Needs review

File	Size
interdiff-2920309-281.txt	1.33 KB
2920309-help-topics-281.patch	66.02 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,604 pass

Just because I want to keep this moving forward, let's assume @jhodgdon is correct in #280 and all that is needed is additional info in the HelpTopicTwig docblock. This latest patch adds the text that @jhodgdon suggested, with some minor copy edits of my own.

Patch and interdiff attached.

Comment #282

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 10 November 2018 at 17:33

Status:

Needs review

» Needs work

Interdiff looks OK to me. I'm not sure about the punctuation in

+ * This class cannot be used as-is for a derivative plugin; because, the
+ * plugin ID would include a colon (:), which makes it unusable as a theme hook
+ * and/or Twig template name.

I think I would do "... plugin, because the ...". The ; and then , after because seems weird to me. Other than that, I think it's good, but let's see what @jibran says.

Comment #283

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 15 November 2018 at 18:58

Yes, I may have been a little overzealous about the semicolon (having a Semicolon Appreciation Society sticker on one's laptop will do this).

That aside for the moment, we still have the more important question open about whether an update to the docblock satisfies the sanitization issue raised by @jibran in #268.

@jibran or @larowlan, can you weigh in please re: #280?

Comment #284

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 15 November 2018 at 20:45

My 2c: I think it would be simpler to move from strtr to str_replace, and move both _ and : to -. Prevents people from getting into issues.

Comment #285

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 15 November 2018 at 20:59

So,

1. Updating getThemeImplementations() in src/Plugin/HelpTopic/HelpTopicPluginManager.php to:

public function getThemeImplementations() {
    $twigPlugins = array_filter($this->getDefinitions(), function (array $definition) {
      return $definition['class'] === HelpTopicTwig::class;
    });
    $return = [
      'help_topic' => [
        'variables' => [
          'body' => [],
          'related' => [],
        ],
      ],
    ];
    foreach ($twigPlugins as $pluginId => $definition) {
      $hook = HelpTopicTwig::getThemeHook($pluginId);
      $return[$hook] = [
        'variables' => [],
        'path' => drupal_get_path('module', $definition['provider']) . '/help_topics/templates',
        'template' => str_replace(['_', ':'], '-', $hook),
      ];
    }
    return $return;
  }

Specifically, changing

'template' => strtr($hook, '_', '-'),

'template' => str_replace(['_', ':'], '-', $hook),

And

2. not updating the docblock in src/Plugin/HelpTopic/HelpTopicTwig.php

@larowlan and/or @jibran -- does that look like it would do? I'll create a new patch if so.

Comment #286

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 15 November 2018 at 21:31

I'm happy with that, we'd need to specifically mention it in the docs that talk about example file names. I'll ping jibran today to get him to confirm

Comment #287

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext commented 10 January 2019 at 06:23

First of all, thank you for all of your hard work. I know getting a module in core is tiring and this is especially true here with so many changes in technical direction :). I reviewed the patch because I wanted to help not to block the progress :). Yes, I agree we don't have to deliver an ideal solution here but we can still identify the issues which can be addressed in followups.

#268.2: It was minor code performance improvment I suggested but as per @larowlan he is fine with current implementation.
#268.4: It was suggestion. I agree let's not do it.
#268.6: It was anoter minor code performance improvment I suggested but I agree with @jhodgdon let's leave it.
#268.8: I think leaving it is fine for now but we do need a followup issue or maybe wait for #2996305: Add support for the url() function to twig {%trans%}.

I agree with #284 but here is my idea, if you look at views_theme it is doing something like

      // If there is no theme function for the given theme definition, it must
      // be a template file. By default this file is located in the /templates
      // directory of the module's folder. If a module wants to define its own
      // location it has to set register_theme of the plugin to FALSE and
      // implement hook_theme() by itself.
      if (!function_exists('theme_' . $def['theme'])) {
        $hooks[$def['theme']]['path'] .= '/templates';
        $hooks[$def['theme']]['template'] = Html::cleanCssIdentifier($def['theme']);
      }

\Drupal\Component\Utility\Html::cleanCssIdentifier('hello:world') returns helloworld and I'd say instead of mannualy using str_replace or strtr the use of Html::cleanCssIdentifier() is cleaner, gets the job done and has already been used in core.

Comment #289

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 16 December 2018 at 00:35

Assigned:	Amber Himes Matz	» Unassigned
Status:	Needs work	» Needs review

File	Size
interdiff-2920309-278-289.txt	955 bytes
2920309-help-topics-289.patch	65.38 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,838 pass

This patch augments the patch in #278 and sanitizes the plugin ID (hook) for use in a template file using the \Drupal\Component\Utility\Html::cleanCssIdentifer() method that Views uses as well to filter hook names for use in template files. (This means that colons : will be filtered out.)

Comment #290

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 7 January 2019 at 21:04

Status:

Needs review

» Needs work

I reckon we're close here, but I can't RTBC, perhaps @jibran will need to be the one to pull the trigger.
A few minor changes I think we still need:

+++ b/core/modules/help_topics/help_topics/templates/help-topic-ui-contextual.html.twig
index 0000000000..69536add95
--- /dev/null

--- /dev/null
+++ b/core/modules/help_topics/help_topics/templates/help-topic-ui-shortcuts.html.twig

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,130 @@
+      throw new AccessDeniedHttpException();

Should this be access denied or page not found?

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,130 @@
+        return ($a['title'] < $b['title']) ? -1 : 1;

Come on Dec 2019 where we can use php7 features like the <=> operator

```
+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,244 @@
+      $hook = HelpTopicTwig::getThemeHook($pluginId);
+      $return[$hook] = [
```
I think we have to sanitize the key too - because theme hook keys need to be valid function names -
So perhaps just move the call to cleanCssIdentifier into the getThemeHook method?

Comment #291

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 8 January 2019 at 20:31

Assigned:

Unassigned

» Amber Himes Matz

Working on a new patch to address 1 and 3 in comment #290. Thanks for the review!

Comment #292

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 9 January 2019 at 00:41

File	Size
interdiff-2920309-289-292.txt	3.26 KB
2920309-help-topics-292.patch	65.32 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,886 pass

1. Removed use Drupal\help_topics\Plugin\HelpTopic\HelpTopicTwig; statement from help_topics.module since IDE reported it wasn't being used.

2. As suggested in (1) of #290, replaced AccessDeniedHttpException with NotFoundHttpException in the controller.

3. Moved the sanitization of the theme hook to getThemeHook() in HelpTopicTwig.php as suggested in (3) of #290

Interdiff of patch in #289 attached as well as new patch.

Ready for a review and hopefully RTBC. Thanks!

Comment #293

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 9 January 2019 at 00:42

Assigned:	Amber Himes Matz	» Unassigned
Status:	Needs work	» Needs review

Setting status to needs review and un-assigning myself.

Comment #294

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext commented 10 January 2019 at 06:23

Status:

Needs review

» Reviewed & tested by the community

Thanks, @Amber Himes Matz, for addressing the feedback.

We had a conversation in slack the other day, @larowlan, @jhodgdon, @Amber Himes Matz and I agreed that patch seems ready for RTBC so let's add this helpful module to core.

Comment #295

9 January 2019 at 03:54

tim.plunkett credited Gábor Hojtsy.

Comment #296

9 January 2019 at 03:54

tim.plunkett credited MariaDenysyuk.

Comment #297

9 January 2019 at 03:54

tim.plunkett credited Mixologic.

Comment #298

9 January 2019 at 03:54

tim.plunkett credited SenthilMohith.

Comment #299

9 January 2019 at 03:54

tim.plunkett credited amateescu.

Comment #300

9 January 2019 at 03:54

tim.plunkett credited catch.

Comment #301

9 January 2019 at 03:54

tim.plunkett credited ckrina.

Comment #302

9 January 2019 at 03:54

tim.plunkett credited gnuget.

Comment #303

9 January 2019 at 03:54

tim.plunkett credited miro_dietiker.

Comment #304

9 January 2019 at 03:54

tim.plunkett credited sandboxpl.

Comment #305

9 January 2019 at 03:54

tim.plunkett credited tstoeckler.

Comment #306

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett at Acquia commented 9 January 2019 at 03:54

Adding credit per the issue summary.

Comment #307

9 January 2019 at 03:55

tim.plunkett credited vijaycs85.

Comment #308

9 January 2019 at 03:55

tim.plunkett credited yo30.

Comment #310

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 9 January 2019 at 10:45

Status:

Reviewed & tested by the community

» Needs review

+++ b/core/modules/help_topics/help_topics.module
@@ -0,0 +1,45 @@
+      $output .= '<dd>' . t('The title and body text of help topics provided by contributed modules and themes are translatable using the <a href=":locale_help">Interface Translation module</a>. Topics provided by custom modules and themes are also translatable if they have been viewed at least once in a non-English language, which triggers putting their translatable text into the translation database.', [':locale_help' => (\Drupal::moduleHandler()->moduleExists('locale')) ? \Drupal::url('help.page', ['name' => 'locale']) : '#']) . '</dd>';

\Drupal::url() is deprecated. Should be creating a Url object here instead

+++ b/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php
@@ -0,0 +1,113 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getCacheTags() {
+    if (!isset($this->topicList)) {
+      $this->calculateTopics();
+    }
+
+    return $this->cacheTagList;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function listTopics() {
+    if (!isset($this->topicList)) {
+      $this->calculateTopics();
+    }
+
+    return $this->topicList;
+  }
+
+  /**
+   * Calculates the topic list and cache tags.
+   */
+  protected function calculateTopics() {
+    /** @var \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginInterface[] $plugins */
+    $plugins = $this->pluginManager->getTopLevelTopics();
+
+    $this->topicList = [];
+    $cache_tags = [];
+    foreach ($plugins as $plugin) {
+      $this->topicList[$plugin->getPluginId()] = $plugin->toLink();
+      foreach ($plugin->getCacheTagsForList() as $tag) {
+        $cache_tags[] = $tag;
+      }
+    }
+
+    $this->cacheTagList = array_unique($cache_tags);
+  }

I think this is all a bit neater and less stateful if written like this:

  /**
   * {@inheritdoc}
   */
  public function getCacheTags() {
    if (!isset($this->cacheTagList)) {
      $this->cacheTagList = [];
      foreach ($this->listTopics() as $topic) {
        $this->cacheTagList = array_merge($this->cacheTagList, $topic->getCacheTagsForList());
      };
      $this->cacheTagList = array_unique($this->cacheTagList);
    }

    return $this->cacheTagList;
  }

  /**
   * {@inheritdoc}
   */
  public function listTopics() {
    if (!isset($this->topicList)) {
      $plugins = $this->pluginManager->getTopLevelTopics();

      $this->topicList = [];
      foreach ($plugins as $plugin) {
        $this->topicList[$plugin->getPluginId()] = $plugin->toLink();
      }
    }

    return $this->topicList;
  }

```
+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginBase.php
@@ -0,0 +1,151 @@
+  /**
+   * Makes a cache tag from a help topic plugin ID.
+   *
+   * @param string $id
+   *   The plugin ID to make a cache tag from.
+   *
+   * @return string|null
+   *   The main cache tag for the topic, or NULL if there is not one.
+   */
+  protected function makeCacheTag($id) {
+    $definition = $this->getPluginManager()->getDefinition($id);
+    if (!$definition) {
+      return NULL;
+    }
+
+    return $definition['cache_tag'];
+  }
+
```
I'm not sure this should be a separate method. Also I'm not sure that returning NULL when an invalid ID is passed is correct. As far as I recall getDefintion() will throw an exception if the $id does not exist. See \Drupal\Component\Plugin\Discovery\DiscoveryTrait::getDefinition()

Also the use of plugin manager here points to an idea that the plugin manager should be setting the cache tags on each definition correctly when it is getting the definitions - we don't need to reach back to it now it could either add the cache tag to the definitions cache tags or it could set up a related_cache_tags property automatically for us.
```
+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,243 @@
+  /**
+   * Constructs a new HelpTopicManager object.
+   *
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The module handler.
+   * @param \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler
+   *   The theme handler.
+   */
+  public function __construct(ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler) {
+    $this->moduleHandler = $module_handler;
+    $this->themeHandler = $theme_handler;
+  }
```
It is interesting that we're not doing any cached discovery. This means that there will be some disk access to look for definitions. Yes it does use file cache but it does file_exists() checks first.

Another thing I wonder about is that whilst we extend DefaultPluginManager we override many methods and don't call the parents. I think this might make for fragile code in the long run. This is illustrated by the opposite point - because we call the parent in alterDefinitions but afaics $this->alterHook is not called. So below I thought I was wrong about alterability but now I appear to be right. That's super confusing.

Also I notice we have no alterability. (Hmmm I was wrong about this - see later comment) Whilst this is unlike other plugin managers I think that this is a good way to start for an experimental module and will help to keep the help topics declarative. And to make things less complex.

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,243 @@
+  /**
+   * Checks to see if plugin is provided by an uninstalled module or theme.
+   */
+  public function alterDefinitions(&$definitions) {

Ah you do have alterability - oh okay I guess it is consistent with other plugin managers. The PHPDoc for this method is wrong. I suggest an @inheritdoc and the comment here is already (sort of) in the code.

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManager.php
@@ -0,0 +1,243 @@
+    $topics = [];
+
+    foreach ($this->getDefinitions() as $definition) {
+      if ($definition['top_level']) {
+        /* @var \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginInterface $topic */
+        $topic = $this->createInstance($definition['id']);
+        $label = (string) $topic->getLabel() . (string) $topic->getPluginId();
+        $topics[$label] = $topic;
+      }
+    }
+
+    ksort($topics);
+    return $topics;

+++ b/core/modules/help_topics/src/Plugin/HelpTopic/HelpTopicPluginManagerInterface.php
@@ -0,0 +1,55 @@
+   * @return \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginInterface[]
+   *   Array of all of the help topics that are marked as top-level, sorted
+   *   by topic title.

It is always helpful to document what the array key is. In this case it is the help topic title + the plugin ID. This makes me wonder though because the title will be translated so sort order will depend on translation. That might be right but should it documented? Also in this case it might be best to remove the keys since they only exist for sorting purposes - ie. do any array_values() on $topics before returning it.

We should mark all the new API as @internal because this is an experimental module - I think we should also consider how we might merge this module with the help module because having both help and help_topics is super confusing
I'm leaving at needs review because we do want more reviews. Also I think the plugin manager inheritance / cacheability / alterability stuff needs review and careful thought.
I think this is very very close to RTBC - as a framework manager I've reviewed all the possible public API and except for the concerns raised above things are looking good. Also I think the move to twig for help topics is great and a really like that we not adding another configurable thing that doesn't really fit configuration into core.

Comment #311

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 9 January 2019 at 21:02

Issue summary:

Thanks for the review @alexpott!

I am editing the issue summary to add a note about making this part of the existing Help module eventually. There was already quite a bit in there about how to take this from Experimental to Full. Just clarifying that part a bit and adding a few more notes.

Also I just want to remind whoever makes the next patch to respond to the review in #310: I still hope to make another plugin class (in contrib) that would add the ability for admins to write their own help topics and have them displayed with these help topics. These would be stored as config entities (don't worry, it's not going into Core), and will have their own cache tags requirements. So... whatever is done for this patch, please don't make it impossible for the other plugin class to take care of cache tags itself. That is, don't have the plugin manager decide about cache tags -- other plugin classes might not work the same way as these. Thanks!

Comment #312

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 10 January 2019 at 00:27

File	Size
292-312-interdiff.txt	16.32 KB
2920309-312.patch	65.42 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass

1. Fixed
2. I was wrong but I've changed the method name to setProperties as this is what this is doing. I've been pondering if \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginManagerInterface::getTopLevelTopics() should really be there or whether this should all take place inside \Drupal\help_topics\Plugin\HelpSection\HelpTopicSection and not be part of a public API. I've left it as it is for now.
3. Fixed and also injected the plugin manager properly into the plugin.
4. I've not addressed this yet.
5. This is really part of 4 - and so not addressed. I think we could have a follow-up to determine alterability for the plugin manager and whether it should extend the default plugin manager.
6. I've removed the keys - they are not useful.
7. Fixed

I've also removed some dead code - ie. HelpTopicPluginManagerInterface::findMatches() and its implementation. As this is not used anywhere it is completely untested and therefore even if needed for later work we should add it there and not now.

I've also fixed some coding standards.

Also

Should \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginBase::getCacheTags() use array_unique() - I think so.
Should HelpTopicPluginBase::getCacheMaxAge() and HelpTopicPluginBase::getCacheContexts() merge in the same way as HelpTopicPluginBase::getCacheTags().
I still think that as \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginInterface::getRelated() is part of the plugin interface the manager could add cache tags and the rest for all help topic plugins regarless of the provider or plugin specifics.
Should \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginManager::getAllListOn() be sorted like \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginManager::getTopLevelTopics()? Maybe not because \Drupal\help_topics\Controller\HelpTopicPluginController::viewHelpTopic() sorts these later. For me this might more evidence that \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginManagerInterface::getTopLevelTopics() doesn't really need to be there.
I think we need a follow-up to add extensive test coverage. I would expect more unit test coverage of each part.
I think that having both related and list_on is redundant - we merge them anyway in \Drupal\help_topics\Controller\HelpTopicPluginController::viewHelpTopic() - we could just use one key - related - and - if I’m a help topic and I say I’m related to another one it doesn’t matter if the other one says it is related to me - I appear on its list. And vice versa. You get the same ability but it’s less confusing.

Comment #313

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 10 January 2019 at 00:54

File	Size
312-313-interdiff.txt	12.55 KB
2920309-313.patch	63.49 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass

Here's a patch that removes the list_on and replaces it with merging all the related info when building definitions. This also showed that \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginManager::alterDefinitions() was not being called. It now is.

Comment #314

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 10 January 2019 at 04:41

Just a quick note -- merging Related and List On is a good idea. Thanks!

Comment #315

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 10 January 2019 at 10:54

File	Size
313-315-interdiff.txt	12.76 KB
2920309-315.patch	66.29 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass

Patch attached addresses:

Alterability - the help topic plugin manager is now alterable via a hook like all the other plugin managers
Cacheability - uses the same system as all the other plugin managers
Adds a test for help topics provided by themes - this was missing and broken
Removes HelpTopicPluginManagerInterface::getTopLevelTopics() and moves the functionality to \Drupal\help_topics\Plugin\HelpSection\HelpTopicSection - if we feel that this ability is useful for other section plugins we could move some of the code to help section plugin base class.

Feedback still to think about:

Should HelpTopicPluginBase::getCacheMaxAge() and HelpTopicPluginBase::getCacheContexts() merge in the same way as HelpTopicPluginBase::getCacheTags().
I still think that as \Drupal\help_topics\Plugin\HelpTopic\HelpTopicPluginInterface::getRelated() is part of the plugin interface, the manager could add cache tags and the rest for all help topic plugins regardless of the provider or plugin specifics.
I think we need a follow-up to add extensive test coverage. I would expect more unit test coverage of each part. For example there's lots cache tags logic but as far as I can see this is not used or tested (which is a big concern for commit - because we'd be committing untested and unused code).
I think we could have a follow-up to determine alterability for the plugin manager and whether it should extend the default plugin manager. (although this is less of an issue now we're doing caching and altering which is more inline with the default plugin manager)

Also I'm not currently working to address this. So if someone else wants to take over feel free!

Comment #316

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 10 January 2019 at 13:21

File	Size
315-316-interdiff.txt	5.25 KB
2920309-316.patch	65.77 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass

+++ b/core/modules/help_topics/help_topics.services.yml
--- /dev/null
+++ b/core/modules/help_topics/help_topics/config_basic.help_topic.yml

We should remove the .help_topic from all these filenames. It is not necessary and we're in a directory called help_topics so these yml files cannot be anything else. This is the same as migration templates - which also uses yml discovery. As the attached patch shows there's no functional change because the .help_topic suffix was not required at all. The suffix passed to YamlDiscovery is not about the filenames but about something added to the filecache keys generated for each file. Which is perhaps how we eneded up with these suffixes on the files.

Still not working on the additional stuff in #315.

Comment #317

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 10 January 2019 at 13:37

File	Size
316-317-interdiff.txt	2.31 KB
2920309-317.patch	64.51 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass

Looking at that made me realise there was a tonne of unneeded code in the plugin manager - since we extend the default plugin manager we can use its implementations.

This resolves #315.4 - no follow-up is necessary we're now much closer to the default plugin manager by way of less code.

Comment #318

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 10 January 2019 at 13:58

File	Size
317-318-interdiff.txt	10.35 KB
2920309-318.patch	63.83 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass

Ah I just realised something else confusing. All the plugin manager and twig plugin class is in \Drupal\help_topics\Plugin\HelpTopic but that area is 100% used by annotated class discovery in core. So we shouldn't use it here. All of this stuff belongs in \Drupal\help_topics as it is the main part of the module. This allows contrib to provide help topics via annotation classes if they like without confusing things here.

Comment #319

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 10 January 2019 at 15:15

Issue summary:

Notes added to issue summary about more followups needed.

Also have been discussing this with @alexpott in Slack. We decided we need to make the plugin IDs be provider_name.whatever_string. For now just document this and change our existing topics to do it; eventually we should enforce it (in discovery class or a discovery decorator?) -- that "eventually" is added to issue summary as a follow-up task after this is committed, because it's a common problem with other plugins to have clashes.

Comment #320

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 10 January 2019 at 15:23

File	Size
318-320-interdiff.txt	2 KB
2920309-320.patch	63.68 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass

Fix the translatability to use YamlDiscovery in the way it is intended and removing some unnecessary yaml.

Comment #321

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 11 January 2019 at 01:40

File	Size
320-321-interdiff.txt	41.19 KB
2920309-321.patch	63.48 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,871 pass

Having to define a help topic in both a yaml file and then create a separate twig template was bothering me. The reason we went for annotations was to keep the discovery along with the code. I think the same rule appears here. If I'm adding a help topic I'd like to only add one file for the help topic.

The attached patch rewrite help topic discovery to only use .html.twig files. Metadata is added to these files via meta html tags. Their plugin ID is determine via the file name. This get's us into the situation where to add a new help topic to a module or theme you create one twig file in its help_topics directory and it'll work. Also because the template and plugin ID are the same we no longer have to prefix all the templates with help-topic-. The theme implementations added still have this to prevent clashes.

Comment #322

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 11 January 2019 at 01:49

Neat!

+++ b/core/modules/help_topics/src/HelpTopicDiscovery.php
@@ -0,0 +1,147 @@
+            case 'top_level':
+              $data[$key] = strtolower($value) === 'true' ? TRUE : FALSE;

could we just omit this tag if it wasn't top level or is the content attribute required?

+++ b/core/modules/help_topics/tests/modules/help_topics_test/help_topics/help-test-additional.html.twig
@@ -0,0 +1,3 @@
+<meta name="help_topic:label" content="Additional topic">
+<meta name="help_topic:related" content="help-test">

oh, you've done that.

so do we need to check for true/false ?

Comment #323

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 11 January 2019 at 12:21

File	Size
321-323-interdiff.txt	21.39 KB
2920309-323.patch	65.5 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,871 pass

The next thing that was bothering me was adding all the help topic templates as themes - this feels off. They're not meant to themed this way or have any off the functionality that core gives theme templates. Patch attached removes all of the exposing help topics in themes and replaces that with a custom Twig loader so we can render a help topic by doing:
\Drupal::service('twig')->load('@help_topics/config-basic.twig.html')->render().

This opens up the interesting possibility of provided some default context for these help topics via the ->render() call in \Drupal\help_topics\HelpTopicTwig::getBody(). Not sure what we'd add yet but perhaps someone else has ideas.

I've also dropped the value from the top level meta as per #322.

Also now the plugin manager exposes no new API ftw.

Comment #324

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 11 January 2019 at 21:02

+++ b/core/modules/help_topics/src/HelpTopicTwig.php
@@ -2,7 +2,8 @@
  * Represents a help topic plugin whose definition comes from a YAML file.

we need to update this doc now - probably should do a full pass on docs for new approach

+++ b/core/modules/help_topics/src/HelpTopicTwigLoader.php
@@ -0,0 +1,60 @@
+class HelpTopicTwigLoader extends \Twig_Loader_Filesystem {

nice! heaps cleaner, and smaller api surface

Comment #325

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext commented 11 January 2019 at 21:32

+++ b/core/modules/help_topics/src/HelpTopicTwigLoader.php
@@ -0,0 +1,60 @@
+      $this->addPath($extension->getPath() . '/help_topics');
...
+      $this->addPath($extension->getPath() . '/help_topics');

We should use the constant self::MAIN_NAMESPACE here.

Comment #326

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 11 January 2019 at 22:26

File	Size
323-325-interdiff.txt	13.13 KB
2920309-325.patch	66.96 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,872 pass

@larowlan thanks for the review. Good point about the docs. I've addressed that in this patch. I've also streamlined a few things I noticed on the way round.

@jibran I've refactored that code. But I disagree - path and namespace are not the same thing in Twig land.

We still have to sort the cache stuff. I think embedding the cache tag in the plugin definition isn't quite right. Each plugin type should be able to specific it's own cacheability where uses the plugin should merge the cache metadata appropriately. The merging should not happen internally to the plugin.

Comment #327

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 11 January 2019 at 23:04

File	Size
326-327-interdiff.txt	11.5 KB
2920309-327.patch	65.03 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,872 pass

Here's a patch that fixes up cacheability. Instead of \Drupal\help_topics\Plugin\HelpSection\HelpTopicSection reaching into the plugins which have to answer it now treats the plugins as the cacheable dependency objects they are and uses the CacheableMetadata object to merge it all together. So now it supports merging their max age and cache contexts to ftw.

Turns out that this is what \Drupal\help_topics\Controller\HelpTopicPluginController::viewHelpTopic() was already doing wrt to adding related help topic plugin cache metadata to the render array.

This means that alternate help plugins can satisfy \Drupal\Core\Cache\CacheableDependencyInterface as they wish and the cacheable metadata will be merged in appropriately.

Comment #328

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 January 2019 at 23:20

Thanks for the new patches! This is a review of the patch in #323, as the other patches came while I was in progress of reviewing...

I'm putting most of my review in the form of an interdiff that fixes up some docs, including what @larowlan pointed out in #324 item 1. Just as easy as pointing out the problems, and more efficient. Besides, I haven't made a patch on this issue in a while. :) [Would have made a new patch but @alexpott is patching at the same time. Hope this is OK and doesn't conflict too much.]

Additional questions/comments on the latest patch that are not covered by my interdiff:

a) With the new format having the plugin meta-data in meta tags in the Twig file: Is the help topic title still translatable and getting to localize.drupal.org? It doesn't look like it:

+++ b/core/modules/help_topics/help_topics/config-basic.html.twig
@@ -0,0 +1,15 @@
+<meta name="help_topic:label" content="Changing basic site settings"/>

That needs to be fixed, unless I'm missing something.

b) If the plugin ID has underscores, are the Twig files using hyphens or underscores in their file names? For example, is the plugin ID for the basic-pages.html.twig basic-pages or basic_pages? Not sure how that is working. I didn't test this patch -- are the links still working between topics? Also in the interdiff I assumed the ID was basic-pages; docs I modified need to be changed if this isn't the case.

OK, looking at the Discovery class, I can answer the question: the IDs are identical to the file names (minus .html.twig). So... we need to make sure that the IDs we've put in the related and in in-text links have been updated, because before they were underscores. At first glance, it looks like the Related section at least is OK, but I haven't looked through everything.

c) Should also look at the patch that removed list-on (somewhere in the last day or two) and make sure all the topics in List On were moved to Related in each topic.

d) Are we still going to recommend namespacing the plugin IDs? I think we should. So the Twig file names in this patch should be updated now.

e) In the Discovery code where it deals with meta tags:

        foreach (get_meta_tags($file) as $key => $value) {
          $key = substr($key, 11);
          switch ($key) {
            case 'related':
              $data[$key] = explode(',', $value);
              break;

Shouldn't we also trim down the values for related so they don't contain spaces by mistake?

f) I'm a bit concerned about the meta tags in the Twig files. As far as I can see, they will be displayed when we display a help topic. However, meta tags (for valid HTML) can only appear in the head element of the page, not in the body. So I think we need to remove them when we display the topics, somehow?

Comment #329

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 January 2019 at 23:21

File	Size
interdiff.txt	10.41 KB

I forgot my interdiff.

Unfortunately, @alexpott made similar changes in his interdiff/patch in #326, so this will be a bit difficult to sort out. :(

Comment #330

Berdir

German

Switzerland

CreditAttribution: Berdir at MD Systems GmbH commented 11 January 2019 at 23:46

+++ b/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php
@@ -79,56 +78,77 @@
+      $this->cacheableMetadata = new CacheableMetadata();
+      foreach ($this->getPlugins() as $plugin) {
+        $this->cacheableMetadata->addCacheableDependency($plugin);

Only looked at the caching interdiff.

You removed the cache stuff from the base class, so that means they might or mit not implement that.

addCacheableDependency($not_a_cacheable_dependency) means completely uncacheable (max age = 0). If other plugins should by default be cacheable, then you'd need to add an instanceof check.

Comment #331

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 11 January 2019 at 23:51

Issue summary:

Two things from recent Slack discussions:

a) We need to sort out translation a bit (updating issue summary to do post-this-issue).

b) It turns out the way topics are currently being displayed, they are getting XSS filtered, so the meta tags are filtered out, so we don't need to worry about #328 (f)

Comment #332

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 11 January 2019 at 23:53

File	Size
327-330-interdiff.txt	1.16 KB
2920309-330.patch	65.18 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,872 pass
327-330-interdiff.txt	1.16 KB

The breadcrumb test was testing nothing. There's a gotcha in \Drupal\Tests\system\Functional\Menu\AssertBreadcrumbTrait::assertBreadcrumbParts() that I'll file another issue about but I've made the test simpler and more robust.

You removed the cache stuff from the base class, so that means they might or mit not implement that.

The base class implements \Drupal\Core\Cache\CacheableDependencyInterface - see \Drupal\help_topics\HelpTopicPluginInterface - they have to.

Comment #333

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 12 January 2019 at 00:16

File	Size
332-333-interdiff.txt	5.28 KB
2920309-333.patch	64.47 KB
8.7.x: PHP 7.2 & MySQL 5.5 Patch Failed to Apply

OK, I've sorted out the docs interdiff. Here are my proposed docs changes.

Comment #335

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 12 January 2019 at 00:23

Status:

Needs work

» Needs review

File	Size
2920309-333.patch	64.47 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,872 pass

1 file was hidden/shown/deleted

File	Size
2920309-333.patch	64.47 KB
8.7.x: PHP 7.2 & MySQL 5.5 Patch Failed to Apply

Patch file upload apparently failed. ?!?

Comment #336

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 12 January 2019 at 10:03

File	Size
335-336-interdiff.txt	29.27 KB
2920309-336.patch	70.89 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,873 pass

Patch attached namespaces all the help topics and enforces this. Only the Help Topic module can provide help topics for other extensions.

It also adds test coverage for the fact this introduces the idea that the help topic module can now provide topics for disabled modules by providing one help topic for the shortcut module. I'm not 100% that this is desired and perhaps that help topic should be provided by core. But the ability warrants discussion.

It also adds test coverage for translatability.

It also sets the correct cache tag for HelpTopicTwig plugins - these vary on core.extension - yes the plugin manager will always be cleared on extension change but there is no harm in getting this right.

It also adds test coverage to ensure that the meta tags added for plugin information are not appearing in the rendered help topic html.

We need to add unit test coverage of HelpTopicDiscovery - there's important logic in there about plugin validity that is not tested.

Comment #337

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext commented 12 January 2019 at 20:39

This is some great work. I ignored the trivial stuff cuz it is WIP. Here are some observations.

+++ b/core/modules/help_topics/src/Controller/HelpTopicPluginController.php
@@ -0,0 +1,124 @@
+          break;

Shouldn't this be continue? If not then can we please explain it in a comment.

+++ b/core/modules/help_topics/src/HelpTopicDiscovery.php
@@ -0,0 +1,168 @@
+        // @todo Remove help_topics special case once Help Topics is stable and
+        //   core modules can provide their own help topics.

Let's add the issue ID.

+++ b/core/modules/help_topics/src/Plugin/HelpSection/HelpTopicSection.php
@@ -0,0 +1,154 @@
+      // Sort the top level topics by label and if they match by plugin ID.

Is this correct? How can plugin ID be same?

Comment #338

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 12 January 2019 at 21:28

RE #337 item 3 -- we are displaying the topics alphabetically by title (label), and then if there is a match between two topic titles, breaking the tie by ID. That seems to be what the comment says?

Also just a thought about where to place topics... One of the drawbacks always identified in the hook_help() system was you couldn't see the help without installing the module. So I think we will want to sometimes write topics that document things like "If you want this type of functionality, turn on this module". We'll need to think about where topics belong when we write them. It's supposed to be task-oriented help, not module-oriented help (another drawback of the hook_help() system). Maybe for Core we want most of the topics to be in the core namespace, in that case? Anyway, that's just a thought and not something I think we need to address now... I guess I'll add this thought to the issue about writing topics, which exists now in the Sandbox project and will be moved into Core when this gets in.

Comment #339

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 13 January 2019 at 00:44

File	Size
336-339-interdiff.txt	11.75 KB
2920309-339.patch	76.88 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,873 pass, 1 fail

Re #337

ah yeah well that's now taken care of by the plugin manager so that code is superfluous. So it's gone
Fixed - opened #3025577: Move help topics to core or the correct modules
Made it clearer and also made the sort better so that 'Two' and 'Three' would sorted the way a human would expect. That said, top level help topics with the same label is interesting with respect to UX.

@jhodgdon re #338 yeah that's what gave me pause. Certainly feels resolvable when we have more help and in #3025577: Move help topics to core or the correct modules

Sometimes I feel that we should not limit help topic scanning to installed extensions but also include uninstalled extensions that are around. But this case only really happens for core because it's the only thing the ships with lots of uninstalled modules. So we can fix this by putting all the topics in core/help_topics (which I've added support for). The problem we have though is that we have to be super careful we don't use routes in url generation for modules that are optional. Atm all the routes we are using in the twig templates are for required modules / core so we don't have a problem but this easily could be.

Added test of alter hook.

Added unit test coverage of HelpTopicDiscovery

Opened issue about AssertBreadcrumbTrait not failing when it should - see #3025580: \Drupal\Tests\system\Functional\Menu\AssertBreadcrumbTrait::assertBreadcrumbParts is vulnerable to false positives

Comment #341

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 13 January 2019 at 08:06

Issue summary:	View changes
Status:	Needs work	» Needs review

File	Size
339-341-interdiff.txt	11 KB
2920309-341.patch	78.12 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,879 pass

Fixed the test namespace that results in the runner error.
Improved the exception messages
Trimmed all the related plugin IDs as per #328
Added an exception when the top_level meta tag has a content value as this currently is ignored so could be confusing if you did meta name="help_topic:top_level" content="0" and top level is still true.

I've done some thinking about the move to the core Help module. I think we should aim to do this because having two optional modules about exposing help will be very confusing. So in order to make this easier I've made all the concrete classes exposed by the module final so they definitely are not extension points. If we move to help then we can alias the base class and interfaces to the help_topics namespace to maintain BC but we have much less of a surface area to worry about. We can keep the hook_help_topics_info_alter() with the same name and the plugin.manager.help_topic with the same name. This gives the smallest API surface area possible.

Comment #342

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 14 January 2019 at 19:49

Thought about #339 and being "super careful we don't use routes in url generation for modules that are optional. Atm all the routes we are using in the twig templates are for required modules / core so we don't have a problem but this easily could be.".

It seems to me that we could make sure of that with a test: use Minimal install profile, and visit all of the help topic pages, making sure they all display without any exceptions or Twig errors or whatever would result?

Comment #343

+Needs release manager review

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 15 January 2019 at 03:39

File	Size
interdiff-341-343.txt	500 bytes
2920309-343.patch	78.03 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,883 pass

Just a small docblock fix: in help_topics.module's hook_theme implementation. There was a comment referencing a removed method (@see \Drupal\help_topics\HelpTopicPluginManager::getThemeImplementations()), so this patch removes that bit.

Tested it out in the admin interface and help topics and internal links are displaying as expected. Thank you @alexpott!

Comment #344

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 17 January 2019 at 23:10

Issue tags:

We discussed this on the committers call and decided that given the scope of the patch has been changed (reduced) significantly, this should be given another review by release managers.

Comment #345

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 18 January 2019 at 09:02

Thanks @larowlan. One point of feedback I had was regarding:

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

However, in https://www.drupal.org/core/experimental#requirements, we have this as a requirement before commit. It's helpful for us to evaluate whether a module is on a good track to become stable, and so on. So it would be a good next step for contributors to go over that handbook page section and review/document the module's status against the things in the list. I'd include in that moving some of the great info in the IS to a planning issue that identifies which issues are core gates blockers; which if any might impact the data model, security, or BC; which are should-haves, and so on. :)

I think the current implementation with Twig templates (and a shifted focus to providing a solid API) is a great improvement over what I reviewed way back in #50. Nice work!

Comment #346

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 18 January 2019 at 10:06

Discussed this with xjm. Agreed the patch looks a lot nicer now (although I didn't do a line by line review or anything).

The issue summary could really use an update - it's still mentioning YAML. Additionally it's not clear to me from #2592487: [meta] Help system overhaul what the immediate next steps are following this issue. Would also be good to know what the plan is to eventually replace hook_help() (again wasn't able to find easily from the meta issue).

Leaving the tag on.

Comment #347

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 18 January 2019 at 10:14

Also I don't think we should add 'final' on all the classes in what will be an experimental module anyway - we really need a core policy issue to define what does and does not get final.

Comment #348

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 18 January 2019 at 10:19

Status:

Needs review

» Needs work

NW for #347 and also for creating an up-to-date experimental module roadmap. @catch and I have more feedback we'd like to discuss on the roadmap that will block adding this to core, but don't want to overload the initial implementation issue with those discussions. We'll keep the tag on this issue until that issue has signoff too. Sound good?

Great work; I know this has been a marathon issue!

Comment #349

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 18 January 2019 at 10:59

Final is there to allow as to move all of this to the help module once it is going to be beta / stable - we need to discuss how that happens. If we add final now we prevent a whole class of problems for people doing early experimentation. For me it makes no sense to not make this easy for everyone when there is a language feature that allows us to do that. With final the API we're adding here is extremely well defined - if we choose to not merge this with help module we can remove the final in a follow-up. But being cautious early gives us all the options.

Comment #350

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 18 January 2019 at 12:45

If we're going to do that with final it should be documented on the experimental modules page and etc.

Comment #351

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 18 January 2019 at 14:08

(Meaning, we should not add it unless/until it comes to be documented on the Experimental Modules page through a core policy decision.)

Comment #352

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 19 January 2019 at 01:04

As @alexpott already mentioned in #349, final isn't being used because of its experimental status alone, it's being used because we plan to move the module into Help. So it seems like a one-off use-case and not necessarily something that needs to be documented as policy for experimental modules. But maybe I'm misunderstanding...

As for the issue summary updates and roadmap comments (and etc), I will look into that and make the updates hopefully over the weekend.

Thanks very much @catch and @xjm for the comments and reviews.

Comment #353

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 19 January 2019 at 10:37

Thanks @Amber Himes Matz.

What we're saying is that right now, under current core policy, final should only be used for things that will never, ever be overridden, because that's what current core practice does. In general, I advise against tying this issue up in a policy discussion that hasn't been concluded (and is contentious).

When we move code, we use our deprecation process to do so. We already have a process and best practice that does not involve using final. The code can be deprecated and wrap the new code in help.module -- that is what we've done in the past for other experimental modules and other APIs that needed to move. Using final would "protect" extending code by disallowing extending code, but there would still be a BC break for callers.

The one tricky piece of the process for moving code between modules is machine names and such -- e.g., Layout Discovery declared core-namespaced service names from the start so that the code could be moved safely. (final would not help that either.)

The other alternative is to move the code before beta, i.e., go straight from alpha to stable (possibly during the 8.8.0 development cycle).

We have more we'd like to discuss on the process for potentially merging the module with help.module, but we'd like to do so on an up-to-date roadmap issue so that we don't bog this already two-page implementation issue down with those bigger-picture discussions. :)

Hope this helps!

Comment #354

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 19 January 2019 at 11:24

Issue summary:

Added #3027054: Help Topics module roadmap: the path to beta and stable to issue summary.

Comment #355

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 19 January 2019 at 11:27

Status:

Needs work

» Needs review

File	Size
343-354-interdiff.txt	4.54 KB
2920309-354.patch	77.99 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,885 pass

Here's patch without final. I've opened #3027053: [Policy, no patch] Allow experimental modules to use final to limit public API during development phase to discuss final in experimental modules.

Comment #356

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 19 January 2019 at 19:53

Issue summary:

Yay for #3025580: \Drupal\Tests\system\Functional\Menu\AssertBreadcrumbTrait::assertBreadcrumbParts is vulnerable to false positives!

Cleaned up the issue summary to remove mentions of YAML and bring it up to date.

There was a question above about whether we would get rid of hook_help() but I think we should leave that to #3027054: Help Topics module roadmap: the path to beta and stable, and I'll make sure that is mentioned on that issue. Let's continue that discussion there.

Comment #357

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 21 January 2019 at 14:38

OK... So since we asked for release manager review, the things I saw brought up were:
- Update the issue summary [done on #356]
- Make an issue for the roadmap to beta/stable [done on #354]
- Don't use "final" for classes [done on #355]

Anything else from the release managers, or can we remove the "Needs release manager review" tag?

How about another review of the latest patch so we can get back to RTBC again hopefully?

Thanks!!

Comment #358

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 January 2019 at 13:31

Bump... What do we still need to do, to get this to RTBC and committed?

Comment #359

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext commented 31 January 2019 at 22:58

52 files were hidden/shown/deleted

File	Size
interdiff-2920309-263.txt	709 bytes
2920309-help-topics-263.patch	64.82 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,594 pass
illustration_help_topics_87x.png	91.75 KB
interdiff-2920309-272.txt	2.72 KB
2920309-help-topics-272.patch	64.72 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,596 pass
interdiff-2920309-275.txt	1.12 KB
2920309-help-topics-275.patch	64.89 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,602 pass, 1 fail
interdiff-2920309-comments-272-278.txt	1.6 KB
2920309-help-topics-278.patch	65.33 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,604 pass
interdiff-2920309-281.txt	1.33 KB
2920309-help-topics-281.patch	66.02 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,604 pass
interdiff-2920309-278-289.txt	955 bytes
2920309-help-topics-289.patch	65.38 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,838 pass
interdiff-2920309-289-292.txt	3.26 KB
2920309-help-topics-292.patch	65.32 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,886 pass
292-312-interdiff.txt	16.32 KB
2920309-312.patch	65.42 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass
312-313-interdiff.txt	12.55 KB
2920309-313.patch	63.49 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass
313-315-interdiff.txt	12.76 KB
2920309-315.patch	66.29 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass
315-316-interdiff.txt	5.25 KB
2920309-316.patch	65.77 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass
316-317-interdiff.txt	2.31 KB
2920309-317.patch	64.51 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass
317-318-interdiff.txt	10.35 KB
2920309-318.patch	63.83 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass
318-320-interdiff.txt	2 KB
2920309-320.patch	63.68 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,887 pass
320-321-interdiff.txt	41.19 KB
2920309-321.patch	63.48 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,871 pass
321-323-interdiff.txt	21.39 KB
2920309-323.patch	65.5 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,871 pass
323-325-interdiff.txt	13.13 KB
2920309-325.patch	66.96 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,872 pass
326-327-interdiff.txt	11.5 KB
2920309-327.patch	65.03 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,872 pass
interdiff.txt	10.41 KB
327-330-interdiff.txt	1.16 KB
2920309-330.patch	65.18 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,872 pass
327-330-interdiff.txt	1.16 KB
332-333-interdiff.txt	5.28 KB
2920309-333.patch	64.47 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,872 pass
335-336-interdiff.txt	29.27 KB
2920309-336.patch	70.89 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,873 pass
336-339-interdiff.txt	11.75 KB
2920309-339.patch	76.88 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,873 pass, 1 fail
339-341-interdiff.txt	11 KB
2920309-341.patch	78.12 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,879 pass
interdiff-341-343.txt	500 bytes
2920309-343.patch	78.03 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,883 pass
343-354-interdiff.txt	4.54 KB

This is indeed ready. Did a final code review. Just come up with few nits.

+++ b/core/modules/help_topics/help_topics.module
@@ -0,0 +1,64 @@
+      $output .= '<dd>' . t('The top-level help topics are listed on the main <a href=":help_page">Help page</a>. Links to other topics, including non-top-level help topics, can be found under the "Related" heading when viewing a topic page.', [':help_page' => Url::fromRoute('help.main')->toString()]) . '</dd>';
...
+        ':help_page' => Url::fromRoute('help.main')->toString(),

Let's create a local variable for this Url.

+++ b/core/modules/help_topics/help_topics.module
@@ -0,0 +1,64 @@
+function help_topics_themes_installed($theme_list) {
+  \Drupal::service('plugin.manager.help_topic')->clearCachedDefinitions();
+}
...
+function help_topics_themes_uninstalled($theme_list) {
+  \Drupal::service('plugin.manager.help_topic')->clearCachedDefinitions();
+}

Can we please add one line of comment why these are necessary?
As we are using both $themeHandler and $moduleHandler in plugin manager for discovery then why we are not implementing hook_modules_installed and hook_modules_uninstalled.

+++ b/core/modules/help_topics/help_topics.services.yml
@@ -0,0 +1,16 @@
+      - { name: breadcrumb_builder, priority: 900 }
...
+    - { name: twig.loader, priority: 100 }

Can we please document the reason to choose these priorities?

Comment #360

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 February 2019 at 15:54

File	Size
2920309-360.patch	77.79 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,937 pass, 2 fail
interdiff.txt	3.64 KB

1 file was hidden/shown/deleted

File	Size
2920309-354.patch	77.99 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,885 pass

Thanks very much for the review!

I do not know the answers to items 2 and 3 in the review in #359. But here is a quick patch that fixes item 1 (also making a local variable for the Locale help page), and an interdiff.

Hopefully @alexpott can help with the other two items?

Comment #361

1 February 2019 at 16:58

Status:

Needs review

» Needs work

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

Comment #362

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 February 2019 at 21:45

Status:

Needs work

» Needs review

File	Size
2920309-362.patch	77.86 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,938 pass
interdiff.txt	1.1 KB

2 files were hidden/shown/deleted

File	Size
2920309-360.patch	77.79 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,937 pass, 2 fail
interdiff.txt	3.64 KB

Oops. The local variable needs to be inside the switch statement. Otherwise the Url calls are failing in that test. Which probably is unrealistic, because this hook doesn't get called if the Help module is not turned on, but whatever. This should prevent the (I think artificial) test failure.

Comment #363

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 2 February 2019 at 01:20

File	Size
362-363-interdiff.txt	1.74 KB
2920309-363.patch	77.95 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,949 pass, 1 fail

I'm not sure that local variables are really worth it in hook_help but /shrug that's a personal taste thing it's not as if there is a right answer.

Re #359.2 the plugin cache is automatically cleared for modules and it is not for themes. But actually there is a better way to do this - add a cache tag.

Re #359.3 I've documented and changed the twig one. The breadcrumb one is a shrug because I'm not sure there is much rhyme and reason behind many of the breadcrumb builders priorities apart from to come before the generic PathBasedBreadcrumbBuilder. The ::applies method matters much more. The breadcrumb builder priorities are not documented anywhere else so I don't think we should here.

Comment #364

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran as a volunteer and at PreviousNext commented 2 February 2019 at 02:23

Status:

Needs review

» Reviewed & tested by the community

3 files were hidden/shown/deleted

File	Size
2920309-362.patch	77.86 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,938 pass
interdiff.txt	1.1 KB
362-363-interdiff.txt	1.74 KB

Thank you very much @jhodgdon and @alexpott for addressing the feedback.

Comment #365

4 February 2019 at 04:20

Status:

Reviewed & tested by the community

» Needs work

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

Comment #366

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott at Acro Commerce, Thunder commented 4 February 2019 at 13:31

Status:

Needs work

» Reviewed & tested by the community

File

Size

363-366-interdiff.txt

665 bytes

2920309-366.patch

78.01 KB

8.8.x:
PHP 7.1 & MySQL 5.7 26,464 pass, 1 fail

I don;t think this fix affects the rtbc from #364.

Comment #367

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 7 February 2019 at 22:53

We discussed this again at the committers meeting.

I put forward that I felt it was ready and asked Product Managers to check the current scope and the topics provided to ensure it addresses their UX and product goals.

@catch agreed to look at the road map and path to stable again and discuss with @xjm from a release management point of view.

Comment #368

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick at Acquia commented 7 February 2019 at 23:04

Issue tags:

+Needs product manager review

Don't have time to do this right now, but for later reference...

Comment #369

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 8 February 2019 at 12:12

Note that there's feedback from both me and xjm on #3027054: Help Topics module roadmap: the path to beta and stable - so please see there for release manager stuff.

Comment #370

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 10 February 2019 at 04:07

@catch & @xjm -- I updated the Roadmap issue summary, created child issues, etc... I think all the comments from you two have been incorporated. Anything else we need to address from a Release Manager perspective that is blocking commit?

@webchick or other Product Managers -- anything else we need to do for your perspective before the initial commit, or that we need to add to the roadmap issue?

Just trying to keep this moving forward... Thanks!

Comment #371

15 February 2019 at 04:58

Status:

Reviewed & tested by the community

» Needs work

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

Comment #372

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy at Acquia commented 15 February 2019 at 14:05

Status:

Needs work

» Reviewed & tested by the community

Drupal CI fail.

Comment #373

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 28 February 2019 at 21:51

I raised this again with release and product managers

Comment #374

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 28 February 2019 at 22:54

File	Size
help-topics-admin-help.png	100.89 KB
help-topics-example-topic.png	51 KB

Adding a couple of screenshots to help with review. @webchick

Comment #375

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick at Acquia commented 28 February 2019 at 23:36

Reviewed this in Slack with @ambermatz.

As a long-time member of the UX team, I really REALLY REALLY want this in. Going back to the very first Usability study at University of Minnesota, we saw users crash into Drupal and go looking for help and then just be utterly mystified and frustrated. I can't over-emphasize how UNhelpful the current help system is.

So one of the things that gives me pause is that the topic-based help, which ideally is the solution for this, since it mirrors almost every other help system out there (other than keyword search-based help, but there's #2664830: Add search capability to help topics for that), is buried below the UNhelpful help. :( This vastly reduces its effectiveness. Amber pointed out #2994748: Make a way for Help Page Sections to be ordered as one solution to this. And one could argue it maybe makes sense to do that after, since we only have a small smattering of help topics atm, but even LITERALLY ANY TOPIC-BASED HELP AT ALL that uses ACTUAL WORDS FOR THINGS instead of module names is still SO much better that this feels in the "block-ish" territory.

Another thing in the "block-ish" territory... the help under those topics ranges from "yeah, ok" at https://www.drupal.org/files/issues/2019-02-28/help-topics-example-topic... to basically placeholder text ("The related topics listed here describe how to set up various aspects of site navigation and URLs.") to actually pretty darn decent (the big about "Configuring error responses, including 403/404 pages"). This quality whiplash is not a great starting point for a module that aims to improve UX, esp. for confused and scared users. It'd be better IMO to take a single topic and flesh it out a couple levels deep (but a topic around a content authors/site builder topic, not one for help writers... not really sure "Writing good help" belongs here vs. a handbook page, since the audience is *extremely* narrow compared to all of the possible people who can access this page).

However! I almost didn't even find that decent/well-written help page at all, because it's linked in sub-links from a "stub" page, and I've been conditioned to ignore those links because of the aforementioned unhelpful help. ;) There, they're merely references/"FYI", not primary navigation. And I don't really have an answer for this one, but yikes.

So I'm struggling with this a lot, because on the one hand did I mention how much I REALLY REALLY REALLY want to see this get in and for module-based help to die? And also I am HUGELY sensitive to how much this team has gotten yanked around through at least two development cycles on this feature. :( "And yet" it feels like we need a minimum UX bar for something that's ostensibly about improving UX. Margh.

I think I'm going to sleep on it and come back tomorrow to consider the removal of that tag and/or what would be needed to get there, but in the meantime I welcome any other thoughts.

Comment #376

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 28 February 2019 at 23:54

Ok, putting in some of the conclusions from a Slack discussion with @webchick and @larowlan on next steps/goals:

1. @larowlan: so how about this... as soon as 8.8 is opened, we put it in...with a list of things that have to be fixed before 8.8 alpha

2. @webchick: we should find a subset of those things that are “must have, before commit” and another subset of those things that are “must have, before beta” and yet another subset that’s “must have, before stable”

Comment #377

-Needs release manager review

she/her

English

Portland, OR USA

CreditAttribution: Amber Himes Matz at Drupalize.Me commented 1 March 2019 at 00:02

File	Size
transcript_help-topics-in-core-thread_2019-02-28.txt	29.19 KB

And here is a transcript of the Slack discussion from today in which @webchick reviewed the issue.

Comment #378

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 March 2019 at 15:51

It seems like there are 3 stumbling blocks identified by @webchick in this issue comment and the slack thread:

Blocker 1: The Topics should be above the Module Overviews.

This is currently a separate issue
#2994748: Make a way for Help Page Sections to be ordered
but I think it is actually a small fix and could be fixed before we commit.

Blocker 2: The current topics, which I wrote a long time ago as a demonstration of the concept, are kind of lame.

This is also a separate issue
#2687107: Reorganize topics into sensible outline, and/or write more topics
But maybe for this patch, we could just pare down to one or two topics that *are* currently full of useful content, and omit the rest?

Blocker 3: No search.

This is also a separate issue
#2664830: Add search capability to help topics

So..... how do we move forward?

I would like to point out a few things:

Point 1. We are not currently claiming this module is "beta" quality. So, as it is, it would not go into a release at all, even as an Experimental module.
Point 2. All of the above issues (plus some more) have already been identified on the "Roadmap to beta/stable" issue
#3027054: Help Topics module roadmap: the path to beta and stable
Point 3. Due to objections/comments from the Release Managers (I think it was xjm) on the Roadmap issue, I think that the current plan is that this module would not have a Beta phase at all, but would only go into a release once it is really fully stable: help page ordered properly, hook_help deprecated, with a search system, at least a good list of useful help topics [perhaps we could release as Beta with a partially-realized but still-useful list of topics?].
Point 4. As per the current Experimental Modules policy, https://www.drupal.org/core/experimental#alpha -- this is proposed as an ***alpha*** module. It is not going into a release.

So.

Could we do this, as I think proposed by @larowlan in Slack:

Proposal part A. Not commit this now, because I think we're in 8.7 alpha timeline (or will be shortly).
Proposal part B. When the 8.8 branch opens, commit this patch as it currently is.
Proposal part C. Then work as hard as we can to fix the "Blockers" and the rest of the Roadmap to get this to at least what we decide is "beta" quality before the 8.8 release

As an alternative to Proposal B, we could do:

Proposal part B-alt2: Commit this patch with only a few topics that actually have some useful content. Delete the rest for now.

Comment #379

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 4 March 2019 at 00:15

I'm plus one on this approach

Proposal part A. Not commit this now, because I think we're in 8.7 alpha timeline (or will be shortly).
Proposal part B. When the 8.8 branch opens, commit this patch as it currently is.
Proposal part C. Then work as hard as we can to fix the "Blockers" and the rest of the Roadmap to get this to at least what we decide is "beta" quality before the 8.8 release

@jhodgdon has distilled @webchick's comments from #375 in #378 so I think we just need to make sure they're all covered in #3027054: Help Topics module roadmap: the path to beta and stable and that we've split #3027054: Help Topics module roadmap: the path to beta and stable into 'must/should/could have' and that there is an issue for each action item.

Once that is done, I think that is enough to commit this to 8.8 (once it opens) but I'll confer with other committers.

Comment #380

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 4 March 2019 at 00:59

I have verified that all 3 of the issues identified as "blockers" in #378 are on the Roadmap issue #3027054: Help Topics module roadmap: the path to beta and stable, and also (shown in #378) they all do have issues. I've also created issues for the other items in the roadmap that didn't already have issues. It's pretty much just an issue list now with a few notes. Since I think everything on there is a "must do", except for 1 thing that is labeled as "we need to think about it", ... well, see if you think it's clearer now and all issues?

Comment #381

4 March 2019 at 22:05

Status:

Reviewed & tested by the community

» Needs work

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

Comment #382

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 4 March 2019 at 23:30

The test fail was in Drupal\Tests\media\FunctionalJavascript\MediaStandardProfileTest::testMediaSources so unrelated to this patch. Hitting retest.

Comment #383

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 4 March 2019 at 23:31

Status:

Needs work

» Reviewed & tested by the community

Comment #384

4 March 2019 at 23:31

Version:

8.7.x-dev

» 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #385

vadim.hirbu CreditAttribution: vadim.hirbu at FFW commented 29 March 2019 at 08:39

Status:

Reviewed & tested by the community

» Needs review

File	Size
2920309-385.patch	78.5 KB
8.8.x: PHP 7.1 & MySQL 5.7 26,263 pass
interdiff-366-385.txt	340 bytes

Added help topic for big_pipe module related to issue #3044059: Convert big_pipe, dynamic_page_cache, page_cache module hook_help() to topic(s).

Comment #386

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 March 2019 at 12:34

Status:

Needs review

» Reviewed & tested by the community

2 files were hidden/shown/deleted

File	Size
2920309-385.patch	78.5 KB
8.8.x: PHP 7.1 & MySQL 5.7 26,263 pass
interdiff-366-385.txt	340 bytes

Sorry, I guess my comment on the other issue was unclear! We have been working on this patch for years on this issue. We just need to get in the previous patch, and add the topic on the other issue.

So, hiding this patch and going back to the RTBC patch that is on comment ~~#356~~ #366.

Comment #387

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 March 2019 at 21:48

File	Size
2920309-366.patch	78.01 KB
8.8.x: PHP 7.1 & MySQL 5.7 26,286 pass PHP 7.1 & PostgreSQL 9.1 26,456 pass, 3 fail PHP 7.1 & SQLite 3.15 26,464 pass, 1 fail PHP 7.1 & MySQL 5.7 26,464 pass, 1 fail

Re-uploading the correct patch, in case someone is scrolling up from the end.

Comment #388

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 March 2019 at 21:51

6 files were hidden/shown/deleted

File	Size
2920309-363.patch	77.95 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,949 pass, 1 fail
363-366-interdiff.txt	665 bytes
help-topics-admin-help.png	100.89 KB
help-topics-example-topic.png	51 KB
transcript_help-topics-in-core-thread_2019-02-28.txt	29.19 KB
2920309-366.patch	78.01 KB
8.8.x: PHP 7.1 & MySQL 5.7 26,286 pass PHP 7.1 & PostgreSQL 9.1 26,456 pass, 3 fail PHP 7.1 & SQLite 3.15 26,464 pass, 1 fail PHP 7.1 & MySQL 5.7 26,464 pass, 1 fail

Aaaand, just leaving visible the one original correct patch. :)

Comment #389

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 27 June 2019 at 14:29

File	Size
interdiff.txt	3.31 KB
2920309-8.8-389.patch	78.05 KB
8.8.x: PHP 7.1 & MySQL 5.7 26,470 pass

1 file was hidden/shown/deleted

File

Size

2920309-366.patch

78.01 KB

8.8.x:
PHP 7.1 & MySQL 5.7 26,464 pass, 1 fail

Clean-up deprecated phpunit call (interdiff for #387) and unpublished patch from #366

Comment #390

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 28 June 2019 at 08:45

Issue tags:

I've discussed this with xjm a couple of times. We're both concerned about exactly what the plan is going to end up looking like for replacing hook_help(), but it seems like that's going to be a lot easier to flesh out with this patch actually in core (as an alpha stability experimental module). So removing the 'needs release manager review' tag.

Comment #391

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy at Acquia commented 28 June 2019 at 12:12

Adjusting credits.

Comment #392

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy at Acquia commented 28 June 2019 at 12:15

Status:	Reviewed & tested by the community	» Fixed
Issue tags:	-Needs product manager review

I agree with @jhodgdon in #378, this is proposed as an alpha module and will not be in a release until *Must have* parts of @webchick's review are addressed. It will be able to move forward much faster as an alpha module in the dev branch compared to a patch with 400 comments. Framework managers signed off on the architecture, and fixing the display and help topics is much less disruptive in an existing module. Most things @webchick identified are already on the roadmap to beta.

Let's move on! 🎉

Comment #393

28 June 2019 at 12:24

Gábor Hojtsy credited Fabianx.

Comment #394

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy at Acquia commented 28 June 2019 at 12:24

Comment #395

28 June 2019 at 12:24

Gábor Hojtsy committed 992f677 on 8.8.x

Issue #2920309 by jhodgdon, Amber Himes Matz, alexpott, larowlan,...

Comment #396

markhalliwell

English

CreditAttribution: markhalliwell at Tag1 Consulting commented 28 June 2019 at 14:54

Cool :D

Interesting idea to use <meta> tags for metadata in Twig templates. This may be useful with #2547363: Use front matter to version templates.

FWIW, I just tested wrapping the meta tags in a Twig comment and it appears PHP can still parse them.

http://ideone.com/hfOcQO

I know the current approach ignores this because XSS filters them out, but it probably wouldn't hurt to ensure they're never printed (even if XSS filtering isn't used).

Comment #397

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 28 June 2019 at 16:52

Interesting idea! I've created a child issue to explore it.
#3064854: Allow Twig templates to use front matter for metadata support
also will add this to the Beta/Stable roadmap issue.

Comment #398

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 1 July 2019 at 00:15

Should we have a change record about this that could

explain the new module/api
point people to issues they could help with
point people to the roadmap required for this to move to beta

thoughts - ?

Comment #399

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 July 2019 at 11:38

Is that normally done with new experimental modules? If so, let's do it. If not... there are so many change records, I would be skeptical that adding one would bring much attention to the new module. But we definitely need to get the word out, at least to the Documentation people that there are issues they can help with (writing/adapting help text for topics). A change record may not be the best vehicle for doing that.

Comment #400

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 1 July 2019 at 12:00

It makes sense to announce it in core gdo but no other experimental modules used change records
The latest example #3047804: Add scaffolding for config_environment experimental module

PS: the only exception was https://www.drupal.org/node/2863992 but because media pre-existing in contrib

Comment #401

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 July 2019 at 12:25

I will write up a post and cross-post to groups.drupal.org/core and documentation groups. Should be done shortly.

Comment #402

15 July 2019 at 12:29

Status:

Fixed

» Closed (fixed)

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

Comment #403

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan at PreviousNext commented 9 October 2019 at 04:42

Issue tags:

+8.8.0 highlights

Tagging for release highlights

Comment #404

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 9 October 2019 at 04:54

Issue summary: