Problem/Motivation

With system_update_11401() we can't remove search module until all sites have run this update, which would mean requiring sites with search module (or all sites to be sure) to update to 11.4.0 before 12.x

However if we fix #3608897: Drupal 11.4 update fails on The "help_search" plugin does not exist and #3608897: Drupal 11.4 update fails on The "help_search" plugin does not exist then I think we could move the update logic back to search module itself, and then sites just need to install the search contrib module before they update which is the same as any other module.

Tagging this as an 11.5.0 release target but we may want to consider an 11.4.x backport too, and especially to the search contrib module.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3608912

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

catch created an issue. See original summary.

catch opened merge request !16241

catch’s picture

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

OK tests fail hard just moving the update.

Next, cherry-picking the commit from #3608710: Core 11.3.13 to 11.4 fails on help_search_items doesn't exist.

If tests continue to fail on the search_help plugin, will cherry pick the commit from that issue too (or possibly also if they don't fail...).

catch’s picture

catch
he/him
Primary language English
commented

OK well that's something.

Moving the help update back to search module exposes #3607971: HTTP ERROR 500 Update node_search plugin does not exist in test failures. So we at least have kind-of test coverage for that case if not everything else that's been reported.

catch’s picture

catch
he/him
Primary language English
commented
Title: Move system_update_11401 back to search » Move search updates back to search module and fix missing plugin errors
Status: Needs work » Needs review

MR is green.

Because the two new modules are alphabetically after node and help their plugins should automatically be discovered instead of the deprecated ones by plugin discovery. We might want to add some extra deprecation messaging anyway, but I think this is ready enough for some testing.

godotislate’s picture

godotislate
he/him
commented

Commented about triggering deprecations in the old plugin class constructors.

Separately, I wonder if the old classes can be essentially empty, since I think the reason the plugins are being loaded is because the router rebuild loads the search page entities which uses those plugins, so as long as we can get past that, we don't actually need much functionality in the class. Not a blocker here though, because maybe there are unaccounted for use cases where the plugins are being loaded for a different purpose during update.

Otherwise, this looks good.

catch’s picture

catch
he/him
Primary language English
commented

Adding the constructor deprecation triggers and also emptied out the classes a bit. It would be possible to empty them even more but that would mean extending different base classes etc.

nicxvan’s picture

nicxvan commented

Applying this change to a site that had issues worked.

I did have to also apply this: #3608134: Drupal 11.4 Compatibility following change to NodePreviewController

godotislate’s picture

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

I think this looks good now.

@nicxvan - is that the right issue number? It's a contrib issue.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +12.0.0 release notes

Tagging for 12.0.0 release notes. We need to double check that the contrib search module when it exists has these exact updates as added here and also that this fixes the upgrade path from 11.3 or 11.2 to both 11.4 and main if that's installed via composer first. And we may also need to tell people to update to 11.4.2 prior to updating to 12.x

nicxvan’s picture

nicxvan commented

@godotislate, yes, multiple things are affecting the upgrade path, this issue is one, and the issue I linked for this particular client.

#3609124: Add proper BC for ImageFormatter is also affecting some people which is core and RTBC as well.

godotislate’s picture

godotislate
he/him
commented

Oh, okay, I wasn't seeing the connection to this issue.

But yes, if core introduces a public or protected property in a class, and a contrib child class already had an existing property with that name, and there's a type mismatch, there's nothing really that can be done, especially since Controllers and Plugins aren't API.

nicxvan’s picture

nicxvan commented

Yep that should be resolved in same page preview.

Thanks for pointing out the mechanism, I didn't see why the bc layer didn't work until you pointed out the pre existing property.

I was just pointing out for others testing this mr.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

There's an actual test failure (maybe random?) but also a bunch of deprecations that are causing multiple functional test jobs to fail:

     ⚠ Search help install
    
    1 test triggered 1 deprecation:
    
    1) /builds/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:51
    Drupal\node\Plugin\Search\NodeSearch is deprecated in drupal:11.4.0 and is removed from drupal:12.0.0. Instead, use \Drupal\search_node\Plugin\Search\SearchNode. See https://www.drupal.org/node/3590298
    
    Triggered by:
    
    * Drupal\Tests\search\Functional\Update\SearchUpdateTest::testSearchHelpInstall
      /builds/core/modules/search/tests/src/Functional/Update/SearchUpdateTest.php:38
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Also added a question about cleanup on uninstall, hook_schema() cleans up after itself but this new mechanism does not as far as I can see.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Yes we need the uninstall, that's a good spot. That pattern is mostly used in core services that aren't installed/uninstalled so it hasn't come up much.

Added IgnoreDeprecations to all the update tests, which is all we can do for that.

Moving back to RTBC because these are all trivial changes.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

Is core/modules/help/tests/src/Functional/HelpSearchHelpUpgradeTest.php in the right place? I just added another IgnoreDeprecations to it, but should it be in search.module instead?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review 
core/modules/help/tests/src/Functional/HelpSearchHelpUpgradeTest.php

Should be in search module so that it lives in the contrib search module when it exists.

I also found help_post_update_search_help_dependencies() which also needed to be moved to search module for the same reason.

godotislate’s picture

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

Applied @catch's comment fix. There's an intermittent test failure (ConfigInstallProfileOverrideTest) which I don't really feel like re-running again.
(May have solution in #3608805: [random test failure] Drupal\Tests\config\Functional\ConfigInstallProfileOverrideTest::testInstallProfileConfigOverwrite).

I think this lgtm now.

catch’s picture

catch
he/him
Primary language English
commented

Put up an 11.x backport MR. The MR diff applied with patch -p1 (not git apply --index) except for the sequences table test but that's because it's main-only.

catch opened merge request !16279

catch’s picture

catch
he/him
Primary language English
commented

Pushed one extra commit to the help update to check if the table exists before we drop it, this might not be 100% necessary but given we have sites in slightly undefined states that already tried to update to 11.4 and ran into issues, seems slightly safer to be defensive in what will be a new update for those sites too.

godotislate’s picture

godotislate
he/him
commented

main MR still looks good after latest commit.
11.x MR looks good for RTBC as well.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -11.5.0 release target, -12.0.0 release notes +11.4.2 release notes

Looks good to me, although I haven't manually tested possible scenarios here. This is extremely tricky to get right, and I can't imagine there are that many sites that use help search; in a way I wish we deprecated search only in 11.x but left the plugins in place, then it could be solved in contrib alone in Drupal 12 and we would just delete the code from core. But still we started this process now, so let's finish it.

Backporting to 11.4.x as this is causing errors on upgrade to 11.4.0/11.4.1 and so we should solve them asap.

Committed 09561ef and pushed to main. Thanks!

Committed and pushed af7dd6efc02 to 11.x and 7c75b993db5 to 11.4.x. 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.

  • longwave committed 7c75b993 on 11.4.x 
    refactor: #3608912 Move search updates back to search module and fix...

longwave closed merge request !16241

  • longwave committed af7dd6ef on 11.x 
    refactor: #3608912 Move search updates back to search module and fix...

longwave closed merge request !16279

  • longwave committed 09561ef8 on main 
    refactor: #3608912 Move search updates back to search module and fix...
cafuego’s picture

cafuego commented

Not sure if it's related to this fix, but after bumping from 11.4.1 to 11.4.2 to fix my "missing node_search plugin" I now appear to no longer have access to search (even as user 1) #3611516: No content search access

quietone’s picture

quietone commented
Parent issue: » #3565780: [meta] Tasks to deprecate the Search module

Adding as a child of the meta to deprecate Search.

joegraduate’s picture

joegraduate
Primary language English
Location Arizona, USA
commented

Does the original 11.4.0 change record about the search plugins being moved need to be updated now that the search plugins have moved back?

catch’s picture

catch
he/him
Primary language English
commented

@joegraduate no because they haven't moved back, deprecated stubs were added back which should be overridden by the new plugins when the new modules are installed.

Status: Fixed » Closed (fixed)

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