Merge Help Topics classes into Help with BC layer

As help topics can be set as stable it needs to solve this issue

how to mark it stable if it should be merged into help, probably it's all goes to 10.2

As I got we still need to mark the module stable the same time as remove "experimental" from codebase

Also the same commit should update all @internal interfaces with comment that it's experimental until move in 📌 Mark Help topics as a stable module Closed: won't fix

Per discussion in 📌 Mark Help topics as a stable module Closed: won't fix I think this is the next step for help topics now. We can move the API and make some documentation updates in one patch (or more if it's easier, but hopefully one), and then the actual help topics themselves can be moved to their respective modules separately. Then we'll need a further follow-up to mark help_topics module obsolete once everything is moved.

Initial move

- classes with BC shims in help_topics as on update container will reference old classes
- routing and service, needs careful review as I started to add type-hints
- template and schema

Needs work

- hook_help rewording
- move tests without BC
- add update hook to disable help_topics and upgrade test

Looks it's easy to review as patch when files are moved

Moved tests and config, needs now upgrade path

1. +++ b/core/modules/help/config/optional/block.block.claro_help_search.yml
   @@ -0,0 +1,29 @@
   +id: claro_help_search
   

   it may have conflict when both help & help_topics enabled and search installed to add the block

2. +++ b/core/modules/help/config/optional/search.page.help_search.yml
   @@ -0,0 +1,11 @@
   +dependencies:
   +  module:
   +    - help
   

   this block should be kept by upgrade path but different dependency

3. +++ b/core/modules/help/src/Plugin/HelpSection/HelpTopicSection.php
   @@ -127,6 +127,7 @@ protected function getPlugins() {
   +      $this->topLevelPlugins = [];
   

   found bug when no topics available sorting bellow fails as NULL coming instead of array

4. +++ b/core/modules/help/tests/src/Functional/HelpTopicsSyntaxTest.php
   @@ -1,12 +1,12 @@
   -use Drupal\help_topics_twig_tester\HelpTestTwigNodeVisitor;
   ...
   +use Drupal\help_twig_tester\HelpTestTwigNodeVisitor;
   

   fixed

Probably we need to remove optional config from help topics to solve #20.1

Updated IS

Looks like everything works and now needs review before adding test for upgrade path - should we disable help_topics or not?
Hope there's no custom modules depending on experimental module

The last failed test ViewsFixRevisionIdUpdateTest using to install extra module before running updates and search plugin (which already configured on 9.4 dump) can't find table

That's because help topics require re-index each time new topic(files) installed see \Drupal\help\HelpSectionManager::clearCachedDefinitions()

To fix last test it needs to modify the test or add kill-switch to prevent indexing to access not-created-yet database table

Kind of that allowed to pass

fix cs

Regarding the upgrade path tests and related tasks...I'm reading the docblock for core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php, which seems like a good to-do list thought I've never done this before. @andypost is this what needs to be done? I will work on that if so.

 * To write an update test:
 * - Write the hook_update_N() implementations that you are testing.
 * - Create one or more database dump files, which will set the database to the
 *   "before updates" state. Normally, these will add some configuration data to
 *   the database, set up some tables/fields, etc.
 * - Create a class that extends this class.
 * - Ensure that the test is in the legacy group. Tests can have multiple
 *   groups.
 * - In your setUp() method, point the $this->databaseDumpFiles variable to the
 *   database dump files, and then call parent::setUp() to run the base setUp()
 *   method in this class.
 * - In your test method, call $this->runUpdates() to run the necessary updates,
 *   and then use test assertions to verify that the result is what you expect.
 * - In order to test both with a "bare" database dump as well as with a
 *   database dump filled with content, extend your update path test class with
 *   a new test class that overrides the bare database dump. Refer to
 *   UpdatePathTestBaseFilledTest for an example.

@Amber no, looking at LB patch - all @internal blocks has changes, a-la

@internal
+ *   Tagged services are internal.

I need help merging hook_help() from topics into help_help() there's few tricky wordings I can't paraphrase for help module

+++ b/core/modules/help/help.api.php
@@ -79,6 +109,22 @@ function hook_help_section_info_alter(array &$info) {
+ * @internal
+ *   Help Topics is currently experimental and should only be leveraged by
+ *   experimental modules and development releases of contributed modules.
+ *   See https://www.drupal.org/core/experimental for more information.

I mean this doc-blocks, I fixed the most of them by removing this but it needs something like patch did for LB #3041053: Mark Layout Builder as a stable module →

Tested on 10.1 with a standard profile.
Had help_topics preinstalled
Applied the patch
The help section is working fine
No errors in the logs.
Using the site normally for creating content no issues. So moving things didn't cause any regression.

Moving to NW for the upgrade path + tests.

Oh also ran update hooks after the patch and those ran fine. Forgot to mention.

Thank you! so only few missing @internal and upgrade tests)

Probably it needs follow-up and todo to clean-up ViewsFixRevisionIdUpdateTest

as plugin cache for topics should be cleaned on code deploy we reseting search index on every action

squashed last commits to simplify review by commits

Will need rebase after 🐛 Resolve test failures on 11.x branch Fixed

Added upgrade tests
- one when help topics already installed, using fixture for search page to make sure dependencies updated
- second to make sure block and search page is installed from optional config

Probably it needs to change name help_update_10100 to help_update_10200 for 10.2

We should import new config as hard-coded data because it may change over time but this update hook should always install expected page and block

Hiding patch file since we're using merge requests for this issue.

Removing credit from myself. Rebased to trigger a rebuild to see if failure is random. Retest button was unavailable.

Sorry rebased again, earlier I got CORS errors in browser and Gitlab was down

btw I;m not sure the fix https://git.drupalcode.org/project/drupal/-/merge_requests/3900/diffs?co... is good but I found no other way to allow this test pass

Also wording for new comments and the moving of properties to constructor promoted arguments (as the code is new for help module) needs review

I squashed last 2 commits to upgrade path one, now it will be easier to review and rebase

Retested same as #29

Had help_topics preinstalled
Applied the patch
The help section is working fine
No errors in the logs.
Using the site normally for creating content no issues. So moving things didn't cause any regression.
Database updates run without issu

Thanks for testing @smustgrave. I haven't had a chance to complete my review of the grammar of newly added comments, as @andypost requested. (I have found 1 error so far.) I'll update the issue to Needs work after my review is completed.

I've added comments and suggestions in GitLab, in the merge request.

Thank you Amber! Applied all suggestions as extra commit and it needs follow-up for @todo to be filed.

I guessed in comment what it could be for but not sure enough, will git blame this weekend and file issue if nobody will do it )

I've opened the follow-up: ✨ Display links to help topics provided by uninstalled modules Active .

Thank you, 👍 exactly words I wanted to find out

So only this todo fix is left

Added missing link for TODO

Now it looks ready

The MR was rebased to 11.x, so updating the version metadata in the issue to 11.x-dev.

Additional changes since review in #41 look good.

We decided in #3037230-26: Finalize the merge of Help Topics into Help → that it makes sense to incorporate the changes in that MR into this one, since they affect several of the same files. So I am doing that.

Rebased after 📌 Enable more service autowiring by adding interface aliases to core modules Fixed

Added changes from #3037230-26: Finalize the merge of Help Topics into Help →

Guess now it should be ready

Seems failure is legit to the issue.

Will keep an eye out for this back in the queue so hopefully can get into 10.2 early.

Yes newly added topic now is displayed in results

Limited topics to help_topic_test module, to prevent collisions.
Previously installing help module in tests is not supposed that all topics will be accessible

All green. Think this would be good for early 10.2

Rebased after 🐛 Update topic contact.setting_default to use route instead of "/contact" Fixed

Rebased after 📌 Add tests to enforce correct use of help_topic_link and help_route_link functions Fixed

It could have collision with 📌 Fix up minor copy problems in help topics Fixed

Did a review of this and it all looks good to me with one exception.

Per 📌 [policy, then docs] Change how we deprecate classes Fixed should we be adding a constructor with a trigger_error for the stub classes?

Huge effort folks

I'm sure we should skip adding deprecation to constructor of help_topics's classes because:

- module initially marked as experimental and after commit it becoming deprecated, there's no policy yet #3135100: [policy and meta] Provide a proper mechanism for deprecating modules and themes →
- all this classes are @internal intentionally as they been supposed to be moved to help
- new classes has this mark in doc-blocks only where it supposed (no BC promises), inspired by #3041053: Mark Layout Builder as a stable module →

Would be great to have a docs page for beta stage a-la we have for alphas https://www.drupal.org/about/core/policies/core-change-policies/experime... →

Rebased and added @internal to \Drupal\help_topics\HelpTwigExtension (somehow it was missing)

I've asked for an RM opinion on the depreciation errors

Release managers agreed this doesn't need to trigger depreciation errors

Committed to 11.x

Added a release note and a change record.

Thank you a lot, working on remaining issues

No CR I can find

andypost → closed merge request !3900

I held off on change record until the parent is resolved

I noticed that help-topics.html.twig was left behind in the stub module. I asked about it on Slack, and I am adding a note to the issue summary (Proposed resolution section) explaining it.

See comment inline in MR.

Filed 🐛 Fix HelpSearch queries to ensure db identifiers are properly escaped. Fixed  for follow-up.

xjm → credited quietone → .

Adding some missed issue credits for the initial IS and updates, reviewers, and release manager feedback. (Which ends up being everyone plus quietone.)

Also, I am not convinced of this approach. Internal APIs are supposed to receive BC and deprecations where possible, and Help Topics is a beta experimental module (not alpha) which means it gets that BC promise.

Quoting myself from Slack:

Counterpoint: Is HT alpha or beta? If it's alpha, definitely nothing needed. If it's beta, we should consider what happens if contrib has integrated with it and check for contrib usages. IIRC it is beta.

We broke BC for Layout Builder in beta and people were pissed AF, but LB was a major extension point with impacts on data model and upgrade path. Whereas contrib help text is vanishingly rare. So my guess is that we don't need to trigger deprecations, but I would still check to ensure we're not causing fatal errors during minor updates or breaking existing contrib integrations.

Not reverting yet, but setting NR so it doesn't get sucked into the void.

@xjm do you mean to file follow-up to add deprecation warnings? I think it needs new issue to discuss in which places as there's no contrib using topics because they supposed to be merged to help initially.

I also searched for "help_topics" in http://grep.xnddx.ru/ and only seemed to find matches in forks of core.

This is probably the best status while the release managers hash things out. @catch did mention using parts of HT's API on a client site (albeit later undoing that). I'd like to review what's in scope internal BC-break-wise myself. It would also help me do so if there were a change record with more details than "Help ate Help Topics"; we should have one anyway.

The usage I did on a client site was this:

/**
 * Implements hook_help_topics_info_alter().
 */
function mymodule_help_topics_info_alter(array &$info) {
  // Remove all help topics except our own.
  foreach ($info as $name => $definition) {
    if ($definition['provider'] !== 'mymodule') {
      unset($info[$name]);
    }
  }
}

This was to use help topics as a cross-site help repository on a site that doesn't otherwise use help module, although the MR isn't merged yet so it's not in production anywhere or anything.

However the hook name hasn't changed as a result of this MR, so this would be unaffected, I thought it might be and was ready to change it when we go to 10.2.x, but just checked and it was preserved across (which makes sense given they're still called help topics, I just hadn't realised).

@catch this workaround could be removed because next step is merged too 📌 Move help topics to core or the correct modules Fixed

Exactly this commit https://git.drupalcode.org/project/drupal/-/merge_requests/4257/diffs?co...

@xjm I filed draft CR (maybe it needs to mention namespace changes as well) OTOH release note is more bold for developers

FYI this issue and 📌 Add help_search plugin to the base admin theme test Fixed are only blockers to mark the plan and parent issues fixed 🌱 Help Topics module roadmap: the path to beta and stable Fixed

I've added this to the change record:

Help topics continue to be defined in a my_module/help_topics directory, and the existing alter hook, hook_help_topics_info_alter() remains the same.

The help_twig.extension service remains the same.

The plugin.manager.help_section_topicsservice has changed to plugin.manager.help_topic.

The only class here that isn't a controller/tagged service is the plugin manager, but there would be no reason to use that in contrib or custom code, so I don't think bc for any of the classes/services would help (no pun intended...) anyone here. The two actual integration points - providing help topics and the alter hook, are unchanged by the move.

The plugin.manager.help_section_topics service has changed to plugin.manager.help_topic.

This is not true

The plugin.manager.help_section_topics was a decorator and removed

The plugin.manager.help_topic using the same name, just defined in help module now - this is topics discovery

Ah thanks I was wondering about that, edited the CR again.

Thank you, only RM review left

I set RTBC the roadmap issue, technically it depends on that one

Also I updated → https://www.drupal.org/community-initiatives/documentation-and-help-init... →

Given this was committed months ago, I've published the change record. I don't feel like a BC break here is going to affect anyone as much as breaking Layout Builder would have done.

As stated in #83 there is not a lot we can do if someone was explicitly depending on the now-uninstalled module, but as it was experimental anyway the policy for these is "use at your own risk" and so I don't think we should be introducing an awkward BC layer that we have to remove again in the future.

I've also updated https://www.drupal.org/about/core/policies/core-change-policies/experime... → to note that Help Topics is now part of Help.

Automatically closed - issue fixed for 2 weeks with no activity.

Updated docs page https://www.drupal.org/node/3074421/revisions/view/12370148/13304979 →

and cleaned in https://www.drupal.org/node/3074421/revisions/view/13304979/13304981 →

Adding tag per Gábor's request.

I am not really sure why config factory instead of the config entity API was used here to create new entities, but this is not the right way. The config_plus → modules catches such issues and throws errors, which is how I've found out about this. Maybe I am missing something or should I open a new issue to fix this?

Actually I've just discovered that an existing search page config in the system is even missing the uuid and this is still breaking for me when saving as a config entity. This is what the config_plus module was created initially for to prevent from configs in the system without uuids. It turns out this config has the issue. Further it is not even possible to set the update due to core checking for the uuid changing ignoring the fact that it might not be there at all. I am just including a quick fix in here and we can discuss how to proceed.

@hchonov please file new issue