Merge Help Topics classes into Help with BC layer

We could do this now using class alias, so that all the classes lived in help topics but had help namespace equivalents

Actually I don't think adding the aliases now is a good idea. We don't want to expose anything that looks like a public API until we're ready to mark the module stable. When we do, the help_topics APIs can be left in place, and just wrap the same stuff in help for BC and a non-fatal ugprade path until folks turn off help_topics.

I think when we will merge topics into help we should disable help_topics module from update hook so this class aliases will not be accessible
So the only viable way is to keep help topics module enabled and make help to depend on it

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 #3355659: Mark Help topics as a stable module

Per discussion in #3355659: Mark Help topics as a stable module 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 the sorting of plugins 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

EDIT changes

 $ git diff 10.1.x --stat
...
 86 files changed, 500 insertions(+), 2141 deletions(-)

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 though 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 #3359077: Resolve test failures on 11.x branch

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

EDIT The failed is unrelated and known as random problem Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testLinkManualDecorator

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: #3360133: Display links to help topics provided by uninstalled modules.

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 #3325557: Enable more service autowiring by adding interface aliases to core modules

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 #3358575: Update topic contact.setting_default to use route instead of "/contact"

Rebased after #3219923: Add tests to enforce correct use of help_topic_link and help_route_link functions

It could have collision with #3121340: Fix up minor copy problems in help topics

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

Per #2935897: [policy, then docs] Change how we deprecate classes 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 module from begining
- 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

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 #3371751: Fix HelpSearch queries to ensure db identifiers are properly escaped. for follow-up.

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 should throw,
as there's no contrib using topics no usage can be found.
And topics been supposed to be merged to help module 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 #3025577: Move help topics to core or the correct modules

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 #3144933: Add help_search plugin to the base admin theme test are only blockers to mark the plan and parent issues fixed #3027054: Help Topics module roadmap: the path to beta and stable

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.

edited to add: also I just realised something else after looking at this - we are forcing uninstall of the module so it can be obsolete, and removed in 11.x. Even if we were to reinstate the classes in help_topics module extending from the versions in help with a deprecation or similar, sites absolutely should not have the module installed anyway so the classes won't be available. i.e. a module has to remove a dependency on help_topics for this minor release if they had added one, so that it can be uninstalled.

So it would be impossible to provide bc unless we did something like class_alias in help module itself. For me this would be a lot worse than no bc layer - we'd be introducing bits of the obsolete experimental module into the non-experimental module just to provide bc - it would have been better to introduce help topics as non-experimental in the first place then since the end result would be less complex.

So I think 'best effort' for the help_topics classes is 'nothing' in this case.

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.

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