Problem/Motivation

The Help module has a Search plugin.

Steps to reproduce

Proposed resolution

Move HelpSearch to Search sub-module

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#18 3581109-nr-bot_qmwtydqp.txt98 bytesneeds-review-queue-bot
#16 3581109-nr-bot_0qwx6b3y.txt91 bytesneeds-review-queue-bot
#10 3581109-nr-bot_7axr8dkn.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3581109

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

quietone created an issue. See original summary.

quietone opened merge request !15184

smustgrave’s picture

smustgrave commented
Assigned: Unassigned » smustgrave

Going to give it a shot this weekend

smustgrave’s picture

smustgrave commented
Status: Active » Needs review
quietone’s picture

quietone commented
Status: Needs review » Needs work
smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Addressed the feedback.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I have concerns about one of the moved test functions. But I also left several other comments. Setting to Needs Work because of the test.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review

Thoughts?

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Thank you for considering my feedback.

This looks good. Feedback was addressed. I believe the only open comments are self-review notes. Thank you for adding those. They helped during the review.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3581109-nr-bot_7axr8dkn.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Updates were necessary due to #3569127: Add new 11.3.x database dump fixtures, without modules deprecated for removal in 12.x. They look good to me. Back to RTBC.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

NW for merge conflict.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Rebase with conflicts in UpdatePathTestBaseFilledTest.php and UpdatePathTestBaseTest.php. And then the baseline needed to be regenerated.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Makes sense because the update fixtures were updated in the last week or so. I checked the most recent changes. They seem appropriate to me. Tests are green. Back to RTBC.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3581109-nr-bot_0qwx6b3y.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community

The rebase was to remove update tests removed in #3580877: Remove updates added prior to 11.3.0 from 12.x. Since that was straightforward I am restoring the RTBC

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3581109-nr-bot_qmwtydqp.txt98 bytes

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

godotislate’s picture

godotislate
he/him
commented

A few comments on the MR. It also has a merge conflict.

quietone’s picture

quietone commented
Status: Needs work » Needs review

@godotislate, thanks!

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

The applied suggestions looks good to me. The change to the fixture makes sense too since the module would not be installed in any core fixture.

godotislate closed merge request !15184

  • godotislate committed 317a6474 on main 
    task: #3581109 Move HelpSearch to Search sub-module

By: quietone
By:...
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Fixed

Committed 317a647 and pushed to main. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • godotislate committed 3d0c1dae on main 
    Revert "task: #3581109 Move HelpSearch to Search sub-module"

This...
godotislate’s picture

godotislate
he/him
commented
Status: Fixed » Needs work

Reverted, because this was causing very slow runs in CI: https://git.drupalcode.org/project/drupal/-/pipelines/802911.
Credited @amateescu and @nicxvan for finding/investigating.

  • godotislate committed bc0673ec on main 
    task: #3581109 Move HelpSearch to Search sub-module

By: quietone
By:...
godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Fixed

Never mind! It was #3496257: Race conditions in CacheCollector/State (again) causing the slowness in HelpTopicsSyntaxTest.

Committed bc0673e and pushed to main

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Needs work

Re-opening for a different problem:

core/modules/search/modules/search_help/tests/src/Kernal/HelpSearchPluginTest.php

We need a quick follow-up to rename the directory. The namespace in the class itself is OK.

quietone’s picture

quietone commented
Status: Needs work » Fixed

There is another error in this commit and I opened a new issue to fix both. #3586821: Fix typos in search_help module

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

quietone’s picture

quietone commented
Related issues: +#3586821: Fix typos in search_help module