Fixed installer not removing the custom search module installation line. #1971
Conversation
WalkthroughInstaller changes split Drupal provisioning so Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User/CI
participant I as Installer (Services::process)
participant F as File util
participant S as provision-10-example.sh
U->>I: Run installer (choose services)
I->>I: Check SOLR selection
alt SOLR selected
I->>S: Keep/ensure combined or SOLR installs (ys_base + ys_search)
else SOLR not selected
I->>F: replaceContentInFile(provision-10-example.sh, change install to ys_base only)
note right of S #bfe3d4: ys_search is inside SOLR-scoped region\nand will be skipped
I->>S: Provision script runs (ys_base installed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 phpcs (3.7.2).vortex/installer/src/Prompts/Handlers/Services.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information .vortex/installer/tests/Functional/Handlers/ServicesInstallTest.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vortex/installer/src/Prompts/Handlers/Services.php (1)
134-147: Only the first matching “*_search” directory is removed; remove all matches.Current code finds a single path and leaves others behind. This can leave custom search modules in profiles or legacy locations.
Apply this diff to delete all matches:
$locations = [ $t . sprintf('/%s/modules/custom/*_search', $w), $t . sprintf('/%s/sites/all/modules/custom/*_search', $w), $t . sprintf('/%s/profiles/*/modules/*_search', $w), $t . sprintf('/%s/profiles/*/modules/custom/*_search', $w), $t . sprintf('/%s/profiles/custom/*/modules/*_search', $w), $t . sprintf('/%s/profiles/custom/*/modules/custom/*_search', $w), ]; - $path = File::findMatchingPath($locations); - if ($path) { - File::rmdir($path); - } + foreach ($locations as $pattern) { + foreach (glob($pattern) ?: [] as $path) { + File::rmdir($path); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
.vortex/installer/tests/Fixtures/install/_baseline/scripts/custom/provision-10-example.shis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/code_provider_other/.github/workflows/-update-dependencies.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/scripts/custom/provision-10-example.shis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_solr/scripts/custom/provision-10-example.shis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/scripts/custom/provision-10-example.shis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (3)
.vortex/installer/src/Prompts/Handlers/Services.php(1 hunks).vortex/installer/tests/Functional/Handlers/ServicesInstallTest.php(1 hunks)scripts/custom/provision-10-example.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
scripts/custom/**
📄 CodeRabbit inference engine (CLAUDE.md)
Project-specific custom scripts should be placed in 'scripts/custom/'
Files:
scripts/custom/provision-10-example.sh
🧬 Code graph analysis (3)
.vortex/installer/tests/Functional/Handlers/ServicesInstallTest.php (1)
.vortex/installer/tests/Functional/FunctionalTestCase.php (1)
assertSutNotContains(139-154)
scripts/custom/provision-10-example.sh (1)
scripts/vortex/provision.sh (1)
drush(74-74)
.vortex/installer/src/Prompts/Handlers/Services.php (1)
.vortex/installer/src/Utils/File.php (1)
File(10-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-docs
🔇 Additional comments (1)
scripts/custom/provision-10-example.sh (1)
66-70: Ys_search gated correctly behind SOLR token.Splitting installs and scoping ys_search within SERVICE_SOLR fixes the non-SOLR path. Looks good.
| File::removeLine($t . DIRECTORY_SEPARATOR . '.ahoy.yml', 'VORTEX_HOST_SOLR_PORT=$(docker compose port solr 8983 2>/dev/null | cut -d : -f 2) && \\'); | ||
| // @todo Remove after 25.10.0 release. | ||
| File::removeLine($t . DIRECTORY_SEPARATOR . '.ahoy.yml', 'VORTEX_HOST_SOLR_PORT=$(docker compose port solr 8983 2>/dev/null | cut -d : -f 2) \\'); | ||
| File::replaceContentInFile($t . DIRECTORY_SEPARATOR . 'scripts/custom/provision-10-example.sh', 'drush pm:install ys_base ys_search', 'drush pm:install ys_base'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the replacement resilient (use regex to match whitespace/flags).
Exact-string search for the old combined install is brittle. Use a regex so variations like extra spaces or flags don’t slip through.
Apply this diff:
- File::replaceContentInFile($t . DIRECTORY_SEPARATOR . 'scripts/custom/provision-10-example.sh', 'drush pm:install ys_base ys_search', 'drush pm:install ys_base');
+ File::replaceContentInFile(
+ $t . DIRECTORY_SEPARATOR . 'scripts/custom/provision-10-example.sh',
+ '/\bdrush\s+pm:install\s+ys_base\s+ys_search\b/',
+ 'drush pm:install ys_base'
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| File::replaceContentInFile($t . DIRECTORY_SEPARATOR . 'scripts/custom/provision-10-example.sh', 'drush pm:install ys_base ys_search', 'drush pm:install ys_base'); | |
| File::replaceContentInFile( | |
| $t . DIRECTORY_SEPARATOR . 'scripts/custom/provision-10-example.sh', | |
| '/\bdrush\s+pm:install\s+ys_base\s+ys_search\b/', | |
| 'drush pm:install ys_base' | |
| ); |
🤖 Prompt for AI Agents
In .vortex/installer/src/Prompts/Handlers/Services.php around line 132, the
File::replaceContentInFile call is using an exact-string search for "drush
pm:install ys_base ys_search" which is brittle; change it to perform a
regex-based replacement that matches "drush pm:install" followed by any
whitespace and optional additional package names or flags (e.g.,
\s+[\w\-\:]+(?:\s+[\w\-\:]+)* and optional flags like --[^\s]+) and replace the
whole matched command with "drush pm:install ys_base" so variations in spacing
or extra flags/packages are handled robustly.
| Env::put(PromptManager::makeEnvName(AiCodeInstructions::id()), AiCodeInstructions::CLAUDE); | ||
| }), | ||
| static::cw(fn(FunctionalTestCase $test) => $test->assertSutNotContains('solr')), | ||
| static::cw(fn(FunctionalTestCase $test) => $test->assertSutNotContains(['solr', '_search'])), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Broadened assertion is good; also assert the exact install line is absent.
Checking for both 'solr' and '_search' is helpful. Add a precise regex to ensure the ys_search install command is gone, reducing false positives.
Apply this diff:
- static::cw(fn(FunctionalTestCase $test) => $test->assertSutNotContains(['solr', '_search'])),
+ static::cw(function (FunctionalTestCase $test): void {
+ $test->assertSutNotContains(['solr', '_search']);
+ // Ensure the installer does not try to install ys_search anywhere.
+ $test->assertSutNotContains('/drush\s+pm:install\s+ys_search/');
+ }),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static::cw(fn(FunctionalTestCase $test) => $test->assertSutNotContains(['solr', '_search'])), | |
| static::cw(function (FunctionalTestCase $test): void { | |
| $test->assertSutNotContains(['solr', '_search']); | |
| // Ensure the installer does not try to install ys_search anywhere. | |
| $test->assertSutNotContains('/drush\s+pm:install\s+ys_search/'); | |
| }), |
🤖 Prompt for AI Agents
.vortex/installer/tests/Functional/Handlers/ServicesInstallTest.php around line
41: the current assertion only checks for 'solr' and '_search' substrings which
can yield false positives; add a precise regex-based negative assertion to
ensure the ys_search install command itself is absent. Update the test to
include an additional check using assertSutNotMatches (or equivalent) with a
regex like '/(?:vortex:install|install)\s+ys_search/i' (or
'/ys_search.*vortex:install/i') to explicitly fail if the exact "install
ys_search" command appears.
| #;< SERVICE_SOLR | ||
| drush pm:install ys_search | ||
| #;> SERVICE_SOLR |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: add a task log line for SOLR-scoped install.
For consistent operator feedback (mirroring other sections), add a task message before installing ys_search.
Apply this diff:
#;< SERVICE_SOLR
- drush pm:install ys_search
+ task "Installing search site modules."
+ drush pm:install ys_search
#;> SERVICE_SOLR📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #;< SERVICE_SOLR | |
| drush pm:install ys_search | |
| #;> SERVICE_SOLR | |
| #;< SERVICE_SOLR | |
| task "Installing search site modules." | |
| drush pm:install ys_search | |
| #;> SERVICE_SOLR |
🤖 Prompt for AI Agents
In scripts/custom/provision-10-example.sh around lines 68 to 70, the SOLR-scoped
install (drush pm:install ys_search) lacks a preceding task log for operator
feedback; add a task message line consistent with other sections (e.g. a
task/echo statement like "Installing SOLR-scoped modules: ys_search")
immediately before the drush pm:install ys_search line so operators see the
action being taken.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1971 +/- ##
========================================
Coverage 77.68% 77.69%
========================================
Files 90 90
Lines 5549 5550 +1
Branches 44 44
========================================
+ Hits 4311 4312 +1
Misses 1238 1238 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/fix-installer-not-removing-search
branch
from
4a4b4fa to
e40f83c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
scripts/custom/provision-10-example.sh (1)
68-70: Add a task log line before installing ys_search.For consistency with other sections and clearer operator feedback, log a task message within the SOLR block.
#;< SERVICE_SOLR + task "Installing search site modules." drush pm:install ys_search #;> SERVICE_SOLRIf you adopt this, update the Bats expectations in .vortex/tests/bats/unit/provision.bats accordingly (see my comment there).
.vortex/installer/src/Prompts/Handlers/Services.php (1)
132-132: Make replacement resilient; also remove standalone ys_search installs when SOLR is off.Exact-string replacement misses spacing/flags and won’t catch lone ys_search lines in older scripts. Use regex for both combined and standalone forms.
- File::replaceContentInFile($t . DIRECTORY_SEPARATOR . 'scripts/custom/provision-10-example.sh', 'drush pm:install ys_base ys_search', 'drush pm:install ys_base'); + // Replace a combined install line regardless of spacing/flags/order. + File::replaceContentInFile( + $t . DIRECTORY_SEPARATOR . 'scripts/custom/provision-10-example.sh', + '/^\s*drush\s+pm:install\b[^\n]*\bys_base\b[^\n]*\bys_search\b[^\n]*$/m', + 'drush pm:install ys_base' + ); + // Drop any standalone ys_search install lines (defensive for older templates). + File::replaceContentInFile( + $t . DIRECTORY_SEPARATOR . 'scripts/custom/provision-10-example.sh', + '/^\s*drush\s+pm:install\b[^\n]*\bys_search\b[^\n]*\n?/m', + '' + );.vortex/installer/tests/Functional/Handlers/ServicesInstallTest.php (1)
41-41: Also assert the exact ys_search install command is absent.Prevents false positives where generic “search” text is removed but the install command remains.
- static::cw(fn(FunctionalTestCase $test) => $test->assertSutNotContains(['solr', '_search'])), + static::cw(function (FunctionalTestCase $test): void { + $test->assertSutNotContains(['solr', '_search']); + // Ensure no direct ys_search installation occurs anywhere. + $test->assertSutNotContains('/^\s*drush\s+pm:install\s+ys_search\b/m'); + }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
.vortex/installer/tests/Fixtures/install/_baseline/scripts/custom/provision-10-example.shis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/code_provider_other/.github/workflows/-update-dependencies.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/names/scripts/custom/provision-10-example.shis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_solr/scripts/custom/provision-10-example.shis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/scripts/custom/provision-10-example.shis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (4)
.vortex/installer/src/Prompts/Handlers/Services.php(1 hunks).vortex/installer/tests/Functional/Handlers/ServicesInstallTest.php(1 hunks).vortex/tests/bats/unit/provision.bats(7 hunks)scripts/custom/provision-10-example.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
scripts/custom/**
📄 CodeRabbit inference engine (CLAUDE.md)
Project-specific custom scripts should be placed in 'scripts/custom/'
Files:
scripts/custom/provision-10-example.sh
🧠 Learnings (1)
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
PR: drevops/vortex#1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/bats/unit/provision.bats
🧬 Code graph analysis (3)
.vortex/installer/tests/Functional/Handlers/ServicesInstallTest.php (1)
.vortex/installer/tests/Functional/FunctionalTestCase.php (1)
assertSutNotContains(139-154)
.vortex/installer/src/Prompts/Handlers/Services.php (1)
.vortex/installer/src/Utils/File.php (1)
File(10-102)
scripts/custom/provision-10-example.sh (1)
scripts/vortex/provision.sh (1)
drush(74-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-docs
🔇 Additional comments (1)
scripts/custom/provision-10-example.sh (1)
66-66: Split install is correct.Installing ys_base unconditionally, and moving ys_search under the SOLR block, matches the intent.
| "@drush -y pm:install ys_base" | ||
| "@drush -y pm:install ys_search" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
If you add the SOLR-scoped task log, mirror it in test expectations.
Keeps the fixtures aligned with the proposed operator message.
@@
- " > Installing custom site modules."
- "@drush -y pm:install ys_base"
- "@drush -y pm:install ys_search"
+ " > Installing custom site modules."
+ "@drush -y pm:install ys_base"
+ " > Installing search site modules."
+ "@drush -y pm:install ys_search"
@@
- " > Installing custom site modules."
- "@drush -y pm:install ys_base"
- "@drush -y pm:install ys_search"
+ " > Installing custom site modules."
+ "@drush -y pm:install ys_base"
+ " > Installing search site modules."
+ "@drush -y pm:install ys_search"
@@
- " > Installing custom site modules."
- "@drush -y pm:install ys_base"
- "@drush -y pm:install ys_search"
+ " > Installing custom site modules."
+ "@drush -y pm:install ys_base"
+ " > Installing search site modules."
+ "@drush -y pm:install ys_search"
@@
- " > Installing custom site modules."
- "@drush -y pm:install ys_base"
- "@drush -y pm:install ys_search"
+ " > Installing custom site modules."
+ "@drush -y pm:install ys_base"
+ " > Installing search site modules."
+ "@drush -y pm:install ys_search"
@@
- " > Installing custom site modules."
- "@drush -y pm:install ys_base"
- "@drush -y pm:install ys_search"
+ " > Installing custom site modules."
+ "@drush -y pm:install ys_base"
+ " > Installing search site modules."
+ "@drush -y pm:install ys_search"
@@
- " > Installing custom site modules."
- "@drush -y pm:install ys_base"
- "@drush -y pm:install ys_search"
+ " > Installing custom site modules."
+ "@drush -y pm:install ys_base"
+ " > Installing search site modules."
+ "@drush -y pm:install ys_search"
@@
- " > Installing custom site modules."
- "@drush -y pm:install ys_base"
- "@drush -y pm:install ys_search"
+ " > Installing custom site modules."
+ "@drush -y pm:install ys_base"
+ " > Installing search site modules."
+ "@drush -y pm:install ys_search"Also applies to: 281-282, 422-423, 568-569, 709-710, 844-845, 986-987
🤖 Prompt for AI Agents
In .vortex/tests/bats/unit/provision.bats around lines 149-150 (and also update
the similar expectations at 281-282, 422-423, 568-569, 709-710, 844-845,
986-987), the test fixtures/expected output do not include the new SOLR-scoped
task log entry introduced by the operator; update each corresponding expected
output line(s) to mirror the new SOLR-scoped log message exactly where the
operator now emits it (insert the same SOLR task log string into the test
expectation sequence for each listed line range) so the test fixtures remain
aligned with the operator output.
Summary by CodeRabbit