Improved Acquia settings with env var overrides and three-tier temp path handling.#2445
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Acquia-specific configuration and environment variables: optional Acquia settings include, three-tier Drupal temporary file path precedence, a trusted-host variable rename, and a simplified Acquia DB download curl invocation; updates Acquia hosting docs and related unit tests. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/vortex/download-db-acquia.sh`:
- Line 265: Update the BATS unit test for download-db-acquia to assert the exact
curl argv used by the script: ensure
.vortex/tests/bats/unit/download-db-acquia.bats (the test that mocks curl)
verifies the invocation includes the flags and variables now used by the script
(the curl call with --progress-bar -L "${backup_url}" -o
"${file_name_compressed}"), or change the test harness to compare full argv
rather than only headers; locate the curl invocation in
scripts/vortex/download-db-acquia.sh and mirror its exact arguments (including
--progress-bar, -L, and the output variable file_name_compressed) in the test
assertion so future changes to curl args fail the test.
In `@web/sites/default/includes/providers/settings.acquia.php`:
- Around line 71-74: The conditional uses getenv('DRUPAL_TMP_PATH_IS_SHARED') in
a boolean context which treats the string "false" as truthy; change the check to
an explicit comparison so the override only runs when the env is intentionally
enabled (e.g., compare getenv('DRUPAL_TMP_PATH_IS_SHARED') === 'true' or use
in_array(getenv('DRUPAL_TMP_PATH_IS_SHARED'), ['1','true'], true)). Update the
if condition that references $ah_site_group and
getenv('DRUPAL_TMP_PATH_IS_SHARED') so it explicitly matches the allowed truthy
values before setting $settings['file_temp_path'].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f88c7b8d-be47-4ba4-aef9-967116c28ddf
⛔ Files ignored due to path filters (5)
.vortex/installer/tests/Fixtures/handler_process/_baseline/web/sites/default/includes/providers/settings.container.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/sites/default/includes/providers/settings.acquia.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/sites/default/includes/providers/settings.container.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/sites/default/includes/providers/settings.acquia.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/sites/default/includes/providers/settings.container.phpis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (4)
.vortex/docs/content/hosting/acquia.mdxscripts/vortex/download-db-acquia.shweb/sites/default/includes/providers/settings.acquia.phpweb/sites/default/includes/providers/settings.container.php
| if (!empty($ah_site_group) && getenv('DRUPAL_TMP_PATH_IS_SHARED')) { | ||
| // @see https://acquia.my.site.com/s/article/360054835954-Bulk-Upload-Not-Working-Correctly | ||
| $settings['file_temp_path'] = "/mnt/gfs/{$ah_site_group}.{$ah_site_env}/tmp"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PHP, what does getenv()return for set and unset variables, and how are the strings"0"and"false" evaluated in boolean conditions?
💡 Result:
In PHP, getenv($name) returns the string value of the environment variable if it is set, or false if it is unset (does not exist). This behavior is unchanged in PHP 8+. In boolean conditions (e.g., if (getenv('VAR'))), PHP converts the result to bool: - If unset: false → false - If set to non-empty string: true (truthy) - If set to empty string "": false - If set to "0": false (specifically listed as falsy) The string "false" (or any other non-empty string except "0") evaluates to true in boolean contexts. Examples: $unset = getenv('UNSET_VAR'); // false → if($unset) is false $set_empty = getenv('EMPTY'); // "" (if VAR=) → if($set_empty) is false $set_zero = getenv('ZERO'); // "0" → if($set_zero) is false $set_false = getenv('FALSE'); // "false" → if($set_false) is true $set_one = getenv('ONE'); // "1" → if($set_one) is true
Citations:
- 1: https://www.php.net/manual/en/function.getenv.php
- 2: https://www.php.net/manual/en/function.getenv
- 3: https://www.php.net/manual/en/types.comparisons.php
- 4: https://www.php.net/manual/en/language.types.boolean.php
Use explicit comparison for DRUPAL_TMP_PATH_IS_SHARED instead of relying on PHP truthiness.
getenv() returns a string when set and false when unset. In boolean context, only the strings "" and "0" are falsey; the string "false" is truthy. This means setting DRUPAL_TMP_PATH_IS_SHARED=false would unexpectedly enable the temporary path override. Explicitly compare to the intended value to prevent misconfiguration.
Suggested fix
- if (!empty($ah_site_group) && getenv('DRUPAL_TMP_PATH_IS_SHARED')) {
+ if (!empty($ah_site_group) && getenv('DRUPAL_TMP_PATH_IS_SHARED') === '1') {
// `@see` https://acquia.my.site.com/s/article/360054835954-Bulk-Upload-Not-Working-Correctly
$settings['file_temp_path'] = "/mnt/gfs/{$ah_site_group}.{$ah_site_env}/tmp";
}📝 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.
| if (!empty($ah_site_group) && getenv('DRUPAL_TMP_PATH_IS_SHARED')) { | |
| // @see https://acquia.my.site.com/s/article/360054835954-Bulk-Upload-Not-Working-Correctly | |
| $settings['file_temp_path'] = "/mnt/gfs/{$ah_site_group}.{$ah_site_env}/tmp"; | |
| } | |
| if (!empty($ah_site_group) && getenv('DRUPAL_TMP_PATH_IS_SHARED') === '1') { | |
| // `@see` https://acquia.my.site.com/s/article/360054835954-Bulk-Upload-Not-Working-Correctly | |
| $settings['file_temp_path'] = "/mnt/gfs/{$ah_site_group}.{$ah_site_env}/tmp"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/sites/default/includes/providers/settings.acquia.php` around lines 71 -
74, The conditional uses getenv('DRUPAL_TMP_PATH_IS_SHARED') in a boolean
context which treats the string "false" as truthy; change the check to an
explicit comparison so the override only runs when the env is intentionally
enabled (e.g., compare getenv('DRUPAL_TMP_PATH_IS_SHARED') === 'true' or use
in_array(getenv('DRUPAL_TMP_PATH_IS_SHARED'), ['1','true'], true)). Update the
if condition that references $ah_site_group and
getenv('DRUPAL_TMP_PATH_IS_SHARED') so it explicitly matches the allowed truthy
values before setting $settings['file_temp_path'].
There was a problem hiding this comment.
This is intentional. The convention in this project is to use truthiness checks for environment variable flags, consistent with how other env vars are handled in these settings files. Setting DRUPAL_TMP_PATH_IS_SHARED=false is not an expected usage pattern — the variable is either set (to any non-empty value) or unset.
There was a problem hiding this comment.
@AlexSkrypnyk, understood — thanks for the clarification. I'll keep this convention in mind for future reviews of this project's settings files.
✏️ Learnings added
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 2445
File: web/sites/default/includes/providers/settings.acquia.php:71-74
Timestamp: 2026-04-02T07:27:42.181Z
Learning: In the `drevops/vortex` project, boolean environment variable flags (e.g., `DRUPAL_TMP_PATH_IS_SHARED`) are intentionally checked using PHP truthiness (e.g., `getenv('VAR')`) rather than explicit comparisons like `=== '1'`. The convention is that such variables are either set to any non-empty value or left unset; setting them to `"false"` is not an expected usage pattern. This applies to settings files under `web/sites/default/includes/providers/`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
This comment has been minimized.
This comment has been minimized.
AlexSkrypnyk
force-pushed
the
feature/fix-acquia-integrations
branch
from
e4e682a to
582b621
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2445 +/- ##
==========================================
- Coverage 79.78% 79.31% -0.47%
==========================================
Files 127 120 -7
Lines 6825 6666 -159
Branches 44 0 -44
==========================================
- Hits 5445 5287 -158
+ Misses 1380 1379 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/vortex/download-db-acquia.sh (1)
265-269:⚠️ Potential issue | 🟠 MajorDon't rely on
$?after thiscurlunderset -e.A failing
curlexits the script before Lines 266-269 run, so the custom error message is unreachable and a partial.gzcan be left behind. Bash's-eoption exits on a non-zero simple command unless the command is in anif/&&/||exception context. (gnu.org)🛠️ Proposed fix
- curl --progress-bar -L "${backup_url}" -o "${file_name_compressed}" - download_result=$? - - # shellcheck disable=SC2181 - [ "${download_result}" -ne 0 ] && fail "Unable to download database ${VORTEX_DOWNLOAD_DB_ACQUIA_DB_NAME}. curl exit code: ${download_result}" && exit 1 + download_result=0 + curl --progress-bar -L "${backup_url}" -o "${file_name_compressed}" || download_result=$? + if [ "${download_result}" -ne 0 ]; then + rm -f "${file_name_compressed}" + fail "Unable to download database ${VORTEX_DOWNLOAD_DB_ACQUIA_DB_NAME}. curl exit code: ${download_result}" + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/vortex/download-db-acquia.sh` around lines 265 - 269, Under set -e you must not rely on checking $? after curl; change the curl invocation in the download block to an explicit conditional (e.g. if ! curl --progress-bar -L "${backup_url}" -o "${file_name_compressed}"; then ...) so the failure handling runs, call fail with the download exit code (capture $?), remove any partial "${file_name_compressed}" before exiting, and remove the separate download_result variable; update the block that currently references download_result and VORTEX_DOWNLOAD_DB_ACQUIA_DB_NAME to use this conditional flow so the custom error message and cleanup always execute on curl failure.
♻️ Duplicate comments (1)
web/sites/default/includes/providers/settings.acquia.php (1)
71-74:⚠️ Potential issue | 🟡 MinorGate
DRUPAL_TMP_PATH_IS_SHAREDon an explicit value.
getenv()returnsfalsewhen the variable is unset, and PHP boolean conversion treats only""and"0"as falsey strings. That means values likeDRUPAL_TMP_PATH_IS_SHARED=falsestill enable the shared-path override here. Compare against the intended value ('1') and mirror that exact value in the docs wording. (php.net)🛠️ Proposed fix
- if (!empty($ah_site_group) && getenv('DRUPAL_TMP_PATH_IS_SHARED')) { + if (!empty($ah_site_group) && getenv('DRUPAL_TMP_PATH_IS_SHARED') === '1') { // `@see` https://acquia.my.site.com/s/article/360054835954-Bulk-Upload-Not-Working-Correctly $settings['file_temp_path'] = sprintf('/mnt/gfs/%s.%s/tmp', $ah_site_group, $ah_site_env); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/sites/default/includes/providers/settings.acquia.php` around lines 71 - 74, The current check uses getenv('DRUPAL_TMP_PATH_IS_SHARED') which treats many string values (e.g., "false") as truthy; change the condition to explicitly compare the env value to the expected value (e.g., getenv('DRUPAL_TMP_PATH_IS_SHARED') === '1') so the shared temp path override only runs when the variable is set to the intended value; update the conditional around $ah_site_group and getenv('DRUPAL_TMP_PATH_IS_SHARED') and keep the same assignment to $settings['file_temp_path'] using $ah_site_env.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/vortex/download-db-acquia.sh`:
- Around line 265-269: Under set -e you must not rely on checking $? after curl;
change the curl invocation in the download block to an explicit conditional
(e.g. if ! curl --progress-bar -L "${backup_url}" -o "${file_name_compressed}";
then ...) so the failure handling runs, call fail with the download exit code
(capture $?), remove any partial "${file_name_compressed}" before exiting, and
remove the separate download_result variable; update the block that currently
references download_result and VORTEX_DOWNLOAD_DB_ACQUIA_DB_NAME to use this
conditional flow so the custom error message and cleanup always execute on curl
failure.
---
Duplicate comments:
In `@web/sites/default/includes/providers/settings.acquia.php`:
- Around line 71-74: The current check uses getenv('DRUPAL_TMP_PATH_IS_SHARED')
which treats many string values (e.g., "false") as truthy; change the condition
to explicitly compare the env value to the expected value (e.g.,
getenv('DRUPAL_TMP_PATH_IS_SHARED') === '1') so the shared temp path override
only runs when the variable is set to the intended value; update the conditional
around $ah_site_group and getenv('DRUPAL_TMP_PATH_IS_SHARED') and keep the same
assignment to $settings['file_temp_path'] using $ah_site_env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 179d17d9-f43b-4899-bcd2-ecdbc6d094e7
⛔ Files ignored due to path filters (5)
.vortex/installer/tests/Fixtures/handler_process/_baseline/web/sites/default/includes/providers/settings.container.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/sites/default/includes/providers/settings.acquia.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/docroot/sites/default/includes/providers/settings.container.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/sites/default/includes/providers/settings.acquia.phpis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/docroot/sites/default/includes/providers/settings.container.phpis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (4)
.vortex/docs/content/hosting/acquia.mdxscripts/vortex/download-db-acquia.shweb/sites/default/includes/providers/settings.acquia.phpweb/sites/default/includes/providers/settings.container.php
…rom download curl.
|
Code coverage (threshold: 90%) Per-class coverage |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
|
Code coverage (threshold: 90%) Per-class coverage |
1 participant
Summary
Improved the Acquia settings integration by extracting shared variables, adding support for overriding the Acquia settings
.incfile path viaDRUPAL_ACQUIA_SETTINGS_FILE, and introducing a three-tier temp path configuration (/tmpdefault → shared GFS mount viaDRUPAL_TMP_PATH_IS_SHARED→ explicitDRUPAL_TMP_PATHoverride). The@codeCoverageIgnoreannotations were narrowed to wrap only the file-system operations, and$drupal_config_pathwas renamed to$config_pathto align with the variable namespacing convention introduced in the previous commit. Documentation inacquia.mdxwas expanded to cover the new environment variables.Changes
web/sites/default/includes/providers/settings.acquia.php$ah_site_groupand$ah_site_envvariables to the top of the block so they can be reused throughout the file without repetition.DRUPAL_ACQUIA_SETTINGS_FILEenv var support: when set, its value is used as the path to the Acquia-provided settings.incfile; otherwise the default path is constructed as before.@codeCoverageIgnoreStart/@codeCoverageIgnoreEndannotations to wrap only thefile_existscheck andrequirecall, excluding surrounding logic from the ignore scope.$drupal_config_path→$config_pathfor consistency with the namespacing convention applied to other variables./tmpDRUPAL_TMP_PATH_IS_SHARED=1DRUPAL_TMP_PATH.vortex/docs/content/hosting/acquia.mdxDRUPAL_ACQUIA_SETTINGS_FILE,DRUPAL_TMP_PATH, andDRUPAL_TMP_PATH_IS_SHAREDto the onboarding environment variables table.Before / After
Acquia settings file path resolution
Temporary directory resolution (three-tier)
Code coverage annotation scope
Summary by CodeRabbit
Documentation
New Features
Bug Fixes