core/help_topics directory does not work

Could be. I didn't look into it yet, just created the issue when I found it and moved on.

Here's a patch that we can consider to be a test-only patch, because it makes it so if you enable Help Topics and go to admin/help, you get this error. It just moves all the topics in core/modules/help_topics/help_topics whose names begin with "core" to the core/help_topics directory.

I'm working on a patch for the code.

OK, that was easy. Here's the 3-line code patch.

OK. I think we need some of what was in the patch on #3072312: Review/fix/delete existing help topics on here. Namely, we should move the topics in core/modules/help_topics/help_topics that pertain to other modules into those modules, remove the discovery code that allows help topics to be in that directory temporarily, and remove the test that verifies the non-help-topics topics are not displayed if their module is absent.

However, I'm not sure it is OK to do this right now. We might need to wait until we are ready to go Stable to move those topics around. I'm just not sure if help topics can spill out across Core before the Help Topics module is stable.

I've asked in Slack about this... We may need to defer actually moving the modules around until we're closer to stable.

As a note on this issue... We cannot actually move the topics into the core directory at this time -- I asked in Slack and found out from @xjm that all of the code *and topics* related to the Help Topics module need to stay under core/modules/help_topics until we are Stable.

However, I think we should still fix this bug so that when we do move the topics, they will work fine. So my suggestion is that we make a patch that moves 1 topic and verifies it works (obviously fixing the code at the same time). Then make a new patch that just fixes the bug without moving the topic, and get that committed.

Then when we get to #3025577: Move help topics to core or the correct modules (which will actually move the topics), we will be ready.

Summary update...

Good idea in #10.

Good though in #11, but I think it is OK. A few lines below that, there is a discovery fail if the provider doesn't match the file name prefix, so I think it is probably good enough. I guess we should look at the code for the directory scanner and see what the circumstances are for returning nothing as the provider, but since we're only scanning module, profile, theme, and core directories, I think it is probably correct.

I will work on a test using a VFS today. And incidentally this issue is limited to making help topics inside core/help_topics work, not removing the ability to have all the help topics inside core/modules/help_topics.

Oh gracious. I put some code into the HelpTopicDiscoveryTest, to test that the Core directory worked, and it passed.

So the problem is actually in HelpTopicPluginManager::getDiscovery(). It is supposed to pass an array of directories **keyed by provider** to the HelpTopicDiscovery constructor. But instead it is doing:

      $all_directories = array_merge(
        ['core'],
        $module_directories,
        $this->themeHandler->getThemeDirectories()

No array key on ['core'] there! That is the problem. Also that directory needs to be fully mapped out, not relative to the app root. So that is fairly easy to fix.

By the way, I don't think we can inject something into the existing core directory structure using a VFS -- it's a separate file system -- so I think the only way to verify that the fix in the plugin manager works is to (temporarily) add at least one help topic in the core/help_topics directory. Given that, here are some patches:
- Patch to commit adds to the unit test, fixes another test, and fixes the plugin manager. This involves injecting the app root into the plugin manager.
- Test-only patch adds a test file to the core/help_topics directory to illustrate the problem, but we cannot commit this now because we don't want topics there (plus this is a test topic).
- Fix-plus-patch adds these two together, to demonstrate that the patch we should commit will work when we get to the point of adding topics to core when the module is stable. We also cannot commit this now, but it illustrates that the fix works. At least, I spot checked a few tests and it seems to be OK, but let's see if the bot agrees.

No interdiff because this is not related to the previous patches.

Yeah, also depending on the timing, if we have already marked Help Topics as Beta, we may need to add a deprecation notice about needing another argument for the plugin manager constructor (API change).

Looks like TestHelpSearch still fails with this patch, and there is also a coding standards violation. I will make a new patch in a few hours.

It turned out that the TwigLoader class also needed to know about the core directory. Now if you have this test core topic added to your system, you can view the page. However, the Help Search test doesn't pass locally -- the uninstall test is failing. Not sure why, because if I manually go and uninstall the module, there is not a problem on the help page... though this is not the same as the test, I think -- there may be some cache clearing happening. Not sure.

Anyway, let's see what the bot says. Here's an interdiff, and two of the 3 patches updated -- to fix the Twig problem and also the coding standards problem (the test-only patch did not change).

OK, the bot says the patch is ready to go. Any reviews please? I think it would be good to get this in before Help Topics beta, because of small API change.

OK, here's a new patch that does that (just edited the previous patch file, and I only did it in the To Commit patch).

Only this line changed in HelpTopicPluginManger (added cast to string):

    $this->root = (string) $root;

@larowlan is too fast for me -- commit on #3087496: Change prefixes of Help Topics machine names to help and make sure APIs are marked @internal conflicted in the services.yml file. Here's a new patch that should apply.