[#2202] Updated Acquia settings to use env variables to include the cloud setting file. #2260
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughReplaces a hardcoded Acquia settings include with a path built from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
AlexSkrypnyk
force-pushed
the
feature/2202-acquia-settings
branch
from
1802c0e to
db68431
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web/sites/default/includes/providers/settings.acquia.php`:
- Around line 22-33: The code currently reads $ah_site_group from
getenv('AH_SITE_GROUP') and silently skips requiring the Acquia settings file
when it's empty; update the logic so that if getenv('AH_SITE_ENVIRONMENT') is
non-empty but $ah_site_group is empty, you throw a RuntimeException indicating
missing AH_SITE_GROUP (before attempting to build $ah_settings_file or require
it). In practice, add a check referencing getenv('AH_SITE_ENVIRONMENT') and
$ah_site_group and throw a descriptive RuntimeException when AH_SITE_ENVIRONMENT
is present but AH_SITE_GROUP is not, keeping the existing
$ah_settings_file/file_exists/require flow unchanged otherwise.
Comment on lines
22
to
33
| // The path is built dynamically from the AH_SITE_GROUP environment variable | ||
| // provided by Acquia Cloud. | ||
| // @codeCoverageIgnoreStart | ||
| $ah_site_group = getenv('AH_SITE_GROUP'); | ||
| if (!empty($ah_site_group)) { | ||
| $ah_settings_file = sprintf('/var/www/site-php/%s/%s-settings.inc', $ah_site_group, $ah_site_group); | ||
| if (!file_exists($ah_settings_file)) { | ||
| throw new \RuntimeException(sprintf('Acquia settings file "%s" not found. Check Acquia Cloud environment configuration.', $ah_settings_file)); | ||
| } | ||
| // @phpstan-ignore-next-line | ||
| require '/var/www/site-php/your_site/your_site-settings.inc'; | ||
| // @codeCoverageIgnoreEnd | ||
| require $ah_settings_file; | ||
| } |
There was a problem hiding this comment.
Fail fast when AH_SITE_GROUP is missing.
When AH_SITE_ENVIRONMENT is set but AH_SITE_GROUP is empty, this silently skips the include and doesn’t signal misconfiguration. The objective calls for a hard failure on misconfigured Acquia env vars—please throw if AH_SITE_GROUP is not set.
✅ Suggested fix
// `@codeCoverageIgnoreStart`
$ah_site_group = getenv('AH_SITE_GROUP');
- if (!empty($ah_site_group)) {
- $ah_settings_file = sprintf('/var/www/site-php/%s/%s-settings.inc', $ah_site_group, $ah_site_group);
- if (!file_exists($ah_settings_file)) {
- throw new \RuntimeException(sprintf('Acquia settings file "%s" not found. Check Acquia Cloud environment configuration.', $ah_settings_file));
- }
- // `@phpstan-ignore-next-line`
- require $ah_settings_file;
- }
+ if (empty($ah_site_group)) {
+ throw new \RuntimeException('AH_SITE_GROUP is not set. Check Acquia Cloud environment configuration.');
+ }
+ $ah_settings_file = sprintf('/var/www/site-php/%s/%s-settings.inc', $ah_site_group, $ah_site_group);
+ if (!file_exists($ah_settings_file)) {
+ throw new \RuntimeException(sprintf('Acquia settings file "%s" not found. Check Acquia Cloud environment configuration.', $ah_settings_file));
+ }
+ // `@phpstan-ignore-next-line`
+ require $ah_settings_file;📝 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.
Suggested change
| // The path is built dynamically from the AH_SITE_GROUP environment variable | |
| // provided by Acquia Cloud. | |
| // @codeCoverageIgnoreStart | |
| $ah_site_group = getenv('AH_SITE_GROUP'); | |
| if (!empty($ah_site_group)) { | |
| $ah_settings_file = sprintf('/var/www/site-php/%s/%s-settings.inc', $ah_site_group, $ah_site_group); | |
| if (!file_exists($ah_settings_file)) { | |
| throw new \RuntimeException(sprintf('Acquia settings file "%s" not found. Check Acquia Cloud environment configuration.', $ah_settings_file)); | |
| } | |
| // @phpstan-ignore-next-line | |
| require '/var/www/site-php/your_site/your_site-settings.inc'; | |
| // @codeCoverageIgnoreEnd | |
| require $ah_settings_file; | |
| } | |
| // The path is built dynamically from the AH_SITE_GROUP environment variable | |
| // provided by Acquia Cloud. | |
| // `@codeCoverageIgnoreStart` | |
| $ah_site_group = getenv('AH_SITE_GROUP'); | |
| if (empty($ah_site_group)) { | |
| throw new \RuntimeException('AH_SITE_GROUP is not set. Check Acquia Cloud environment configuration.'); | |
| } | |
| $ah_settings_file = sprintf('/var/www/site-php/%s/%s-settings.inc', $ah_site_group, $ah_site_group); | |
| if (!file_exists($ah_settings_file)) { | |
| throw new \RuntimeException(sprintf('Acquia settings file "%s" not found. Check Acquia Cloud environment configuration.', $ah_settings_file)); | |
| } | |
| // `@phpstan-ignore-next-line` | |
| require $ah_settings_file; |
🤖 Prompt for AI Agents
In `@web/sites/default/includes/providers/settings.acquia.php` around lines 22 -
33, The code currently reads $ah_site_group from getenv('AH_SITE_GROUP') and
silently skips requiring the Acquia settings file when it's empty; update the
logic so that if getenv('AH_SITE_ENVIRONMENT') is non-empty but $ah_site_group
is empty, you throw a RuntimeException indicating missing AH_SITE_GROUP (before
attempting to build $ah_settings_file or require it). In practice, add a check
referencing getenv('AH_SITE_ENVIRONMENT') and $ah_site_group and throw a
descriptive RuntimeException when AH_SITE_ENVIRONMENT is present but
AH_SITE_GROUP is not, keeping the existing $ah_settings_file/file_exists/require
flow unchanged otherwise.
…loud setting file.
AlexSkrypnyk
force-pushed
the
feature/2202-acquia-settings
branch
from
db68431 to
3c0a000
Compare
|
Member
Author
|
Member
Author
|
Member
Author
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2260 +/- ##
==========================================
- Coverage 77.05% 76.44% -0.61%
==========================================
Files 114 107 -7
Lines 5988 5829 -159
Branches 44 0 -44
==========================================
- Hits 4614 4456 -158
+ Misses 1374 1373 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2202
Summary by CodeRabbit
Refactor
Bug Fixes
Tests