Problem/Motivation

Over in #2664830-213: Add search capability to help topics we discovered that search blocks don't have a dependency on the search page they submit to. This can cause problems when uninstalling modules.

Steps to reproduce:

a) Create and place a search form block that connects to a particular type of search page provided by a module that you can uninstall later, such as Help search if #2664830: Add search capability to help topics is installed and Help Topics module is turned on (or a testing module's page if you are writing a test). Visit the page where the block is placed and verify you can submit the search form.

b) Uninstall the module that provides the search page.

c) Visit the page with the block on it. You will get a White Screen of Death due to a routing exception, saying the route to the search page cannot be found.

Proposed resolution

Fix the plugin class to add the correct dependencies. This should make the block get deleted as part of the module uninstall.

Remaining tasks

Make a patch, including a test.

User interface changes

Search Form blocks connected to particular Search pages will be deleted when the search page is deleted, such as when the providing module is uninstalled.

API changes

None.

Data model changes

Search Form blocks will have a dependency on the configured search page.

Release notes snippet

CommentFileSizeAuthor
#33 3086824-nr-bot.txt144 bytesneeds-review-queue-bot
#29 3086824-29.patch7.8 KBandypost
#29 interdiff.txt2.74 KBandypost
#28 3086824-28.patch7.39 KBandypost
#28 interdiff.txt2.11 KBandypost
#23 3086824-23-do-not-commit.patch9.93 KBandypost
#23 interdiff.txt562 bytesandypost
#20 3086824-20-do-not-commit.patch9.39 KBandypost
#20 interdiff.txt716 bytesandypost
#16 3086824-13.patch9.04 KBandypost
#13 3086824-12.patch8.69 KBandypost
#13 interdiff.txt558 bytesandypost
#10 3086824-10.patch8.14 KBandypost
#10 interdiff.txt2.44 KBandypost
#3 interdiff.txt1.35 KBandypost
#8 3086824-8.patch5.7 KBalexpott
#3 3086824-3.patch3.4 KBandypost
#2 3086824-2.patch2.96 KBalexpott
#8 3-8-interdiff.txt2.3 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new 3086824-2.patch2.96 KB

Here's a start on this. It's a shame that the search page repository can't load a page by an ID given that that is already injected.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt1.35 KB
new 3086824-3.patch3.4 KB

Fix "deprecation dance"

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue summary: View changes

Filling out the issue summary.

However... There is another aspect to this problem, which is that you can also *disable* a search page from admin/search/pages. This I think will also trigger the WSOD. However, it will not be fixed by just adding this dependency.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Related issues: +#2901590: Investigate route rebuilding problem wrt Search module

I think this is related

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Actually I am pretty sure this issue is a duplicate of the other one. And the title is wrong -- the title is about the proposed fix, not about the actual bug, which is that the Search Form Block doesn't verify that the search page it would want to submit to actually exists and is currently enabled.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

RE #4, that part is now a separate issue #3086846: Deprecate ability to disable search pages (no reason for it and causes problems and code complexity), so the issue summary here is OK.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 3-8-interdiff.txt2.3 KB
new 3086824-8.patch5.7 KB

Adding a test and an update path. Now we need an update path test.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Assigned: Unassigned » andypost
Related issues: +#3067943: Add capability for search blocks on non-default search
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt2.44 KB
new 3086824-10.patch8.14 KB

Here goes upgrade test, looks if no search page configured it must add dependency on search settings (default page)

andypost’s picture

andypost
he/him
Primary language Russian
commented

Umami and standard's profile blocks use to define search page so added their dependencies

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/profiles/standard/config/install/block.block.bartik_search.yml
@@ -1,6 +1,8 @@
+  config:
+    - search.page.node_search

It's odd that \Drupal\FunctionalTests\Installer\StandardInstallerTest::testStandardConfig() didn't catch this. It's also odd that page_id is set in Standard. FYI the search_post_update_block_page() will not update existing Standard installs.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt558 bytes
new 3086824-12.patch8.69 KB

Patch fixing and one more block

@alexpott that's supposed behaviour - block can set "empty" to point to use default page from settings, see \Drupal\search\Form\SearchBlockForm::buildForm() starting lines

That makes a question - if default configured ...isn't it the blocks with "" should depends on config search.settings, see \Drupal\search\SearchPageRepository::getDefaultSearchPage()

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think the patch is going to fail because this patch on HEAD generates a \Drupal\Core\Config\UnmetDependenciesException() when installing standard. Which is interesting because #2922417: Profile provided configuration entities can have unmeetable dependencies was supposed to have solved this :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Ah I see. So this isn't really caused by #2922417: Profile provided configuration entities can have unmeetable dependencies the unmet dependency fail also occurs on commit hash bd5908ad2e - which is the commit prior to 2922417 going in (phew). The problem is that search.page.node is optional configuration in the node module and is only installed when the optional config for install profile is installed - which is after the config/install config for Standard is installed. SO we have 2 options:

  • Move the block to standard/config/optional
  • Copy the search.page.node to standard/config/install
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 3086824-13.patch9.04 KB

Moved to optional

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

There's a third interesting option. In install_install_profile() we could install all available optional config both before and after the installation of the profile. This allows profile configuration to depend upon optional configuration provided by the modules it installs.

The last submitted patch, 10: 3086824-10.patch, failed testing. View results

The last submitted patch, 13: 3086824-12.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt716 bytes
new 3086824-20-do-not-commit.patch9.39 KB

Here's testing patch - calling install optional config before profile install and blocks moved back

Status: Needs review » Needs work

The last submitted patch, 20: 3086824-20-do-not-commit.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Opened #3087130: Profile configuration should be able to depend on optional configuration provided by modules = the fail in #20 is valid we need to update the help topic config to move the dependency out of enforced.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt562 bytes
new 3086824-23-do-not-commit.patch9.93 KB

Fixed "enforced" looks this one should be postponed on #3087130: Profile configuration should be able to depend on optional configuration provided by modules

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Assigned: andypost » Unassigned
Issue tags: -Needs tests
StatusFileSize
new interdiff.txt2.11 KB
new 3086824-28.patch7.39 KB

Re-roll for 9.3, tests added

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt2.74 KB
new 3086824-29.patch7.8 KB

Fix post update hook to prevent Olivero's blocks from update and polishing

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3086824-nr-bot.txt144 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone’s picture

quietone commented
Status: Needs work » Postponed

The Search Module was approved for removal in #3476883: [Policy, no patch] Move Search module to contrib .

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3565780: [meta] Tasks to deprecate the Search module and the removal work in #3565783: [meta] Tasks to remove the Search module.

Search will be moved to a contributed project before Drupal 12.0.0 is released.

quietone’s picture

quietone commented
Project: Drupal core » Search (from core)
Version: main » 1.0.0
Component: search.module » Code
Status: Postponed » Needs work

The Search Module has moved from core to contrib,