[#1913] Allow to modify Drush PHP runtime configuration #2268
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
WalkthroughThis PR introduces customizable PHP runtime configuration for Drush CLI by adding a drush.ini file, setting PHP_INI_SCAN_DIR in Docker and provisioning scripts to discover it, updating documentation, and adding tests that exercise modifying and applying the drush.ini. A duplicate test method definition was also added in the trait. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/1913-drush-php-ini
branch
from
c516c61 to
a19b80c
Compare
This comment has been minimized.
This comment has been minimized.
Member
Author
|
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
🤖 Fix all issues with AI agents
In @.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php:
- Around line 278-321: Wrap the parts of subtestDockerComposeDrushPhpIni that
modify drush/php-ini/drush.ini and run assertions in a try/finally so the
original file is always restored: call fileBackup($ini_file) before
modifications, then in a finally block call fileRestore($ini_file) and
syncToContainer($ini_file) to ensure restoration and container sync even if an
assertion (or any exception) fails; keep existing cmd/assert checks inside the
try and ensure the final logStepFinish() remains after restoration.
In `@hooks/library/provision.sh`:
- Around line 14-18: The PHP_INI_SCAN_DIR assignment can drop the default scan
dirs when the existing value lacks a leading colon; update the logic in
hooks/library/provision.sh around the PHP_INI_SCAN_DIR assignment so that you
normalize the existing PHP_INI_SCAN_DIR by ensuring it begins with a colon (if
non-empty and not already starting with ':') before appending
:$(pwd)/drush/php-ini, then export PHP_INI_SCAN_DIR; reference the
PHP_INI_SCAN_DIR variable in your change.
Comment on lines
+278
to
+321
| protected function subtestDockerComposeDrushPhpIni(): void { | ||
| $this->logStepStart(); | ||
|
|
||
| $ini_file = 'drush/php-ini/drush.ini'; | ||
|
|
||
| $this->logSubstep('Assert default PHP ini values from drush.ini are applied.'); | ||
| $this->assertFileExists($ini_file, 'Drush PHP ini file should exist'); | ||
| $this->assertFileContainsString($ini_file, 'memory_limit = 512M', 'Drush PHP ini file should contain default memory_limit'); | ||
| $this->cmd( | ||
| 'docker compose exec -T cli php -r "echo ini_get(\'memory_limit\');"', | ||
| '512M', | ||
| 'PHP memory_limit should be 512M from drush.ini' | ||
| ); | ||
|
|
||
| $this->logSubstep('Assert PHP ini values are updated when drush.ini is changed.'); | ||
| $this->fileBackup($ini_file); | ||
| File::dump($ini_file, "memory_limit = 256M\n"); | ||
| $this->syncToContainer($ini_file); | ||
| $this->cmd( | ||
| 'docker compose exec -T cli php -r "echo ini_get(\'memory_limit\');"', | ||
| '256M', | ||
| 'PHP memory_limit should be 256M after changing drush.ini' | ||
| ); | ||
|
|
||
| $this->logSubstep('Assert multiple PHP ini directives are applied.'); | ||
| File::dump($ini_file, "memory_limit = 1024M\nerror_reporting = E_ALL\n"); | ||
| $this->syncToContainer($ini_file); | ||
| $this->cmd( | ||
| 'docker compose exec -T cli php -r "echo ini_get(\'memory_limit\');"', | ||
| '1024M', | ||
| 'PHP memory_limit should be 1024M after second change' | ||
| ); | ||
| $this->cmd( | ||
| 'docker compose exec -T cli php -r "echo ini_get(\'error_reporting\');"', | ||
| '32767', | ||
| 'PHP error_reporting should be 32767 (E_ALL) from drush.ini' | ||
| ); | ||
|
|
||
| $this->logSubstep('Restore drush.ini to original state.'); | ||
| $this->fileRestore($ini_file); | ||
| $this->syncToContainer($ini_file); | ||
|
|
||
| $this->logStepFinish(); | ||
| } |
There was a problem hiding this comment.
Ensure drush.ini is always restored (even on assertion failures).
A failing assertion before restore will leave the file modified and can cascade into later tests.
✅ Suggested fix (try/finally restore)
- $this->logSubstep('Assert PHP ini values are updated when drush.ini is changed.');
- $this->fileBackup($ini_file);
- File::dump($ini_file, "memory_limit = 256M\n");
- $this->syncToContainer($ini_file);
- $this->cmd(
- 'docker compose exec -T cli php -r "echo ini_get(\'memory_limit\');"',
- '256M',
- 'PHP memory_limit should be 256M after changing drush.ini'
- );
-
- $this->logSubstep('Assert multiple PHP ini directives are applied.');
- File::dump($ini_file, "memory_limit = 1024M\nerror_reporting = E_ALL\n");
- $this->syncToContainer($ini_file);
- $this->cmd(
- 'docker compose exec -T cli php -r "echo ini_get(\'memory_limit\');"',
- '1024M',
- 'PHP memory_limit should be 1024M after second change'
- );
- $this->cmd(
- 'docker compose exec -T cli php -r "echo ini_get(\'error_reporting\');"',
- '32767',
- 'PHP error_reporting should be 32767 (E_ALL) from drush.ini'
- );
-
- $this->logSubstep('Restore drush.ini to original state.');
- $this->fileRestore($ini_file);
- $this->syncToContainer($ini_file);
-
- $this->logStepFinish();
+ $this->fileBackup($ini_file);
+ try {
+ $this->logSubstep('Assert PHP ini values are updated when drush.ini is changed.');
+ File::dump($ini_file, "memory_limit = 256M\n");
+ $this->syncToContainer($ini_file);
+ $this->cmd(
+ 'docker compose exec -T cli php -r "echo ini_get(\'memory_limit\');"',
+ '256M',
+ 'PHP memory_limit should be 256M after changing drush.ini'
+ );
+
+ $this->logSubstep('Assert multiple PHP ini directives are applied.');
+ File::dump($ini_file, "memory_limit = 1024M\nerror_reporting = E_ALL\n");
+ $this->syncToContainer($ini_file);
+ $this->cmd(
+ 'docker compose exec -T cli php -r "echo ini_get(\'memory_limit\');"',
+ '1024M',
+ 'PHP memory_limit should be 1024M after second change'
+ );
+ $this->cmd(
+ 'docker compose exec -T cli php -r "echo ini_get(\'error_reporting\');"',
+ '32767',
+ 'PHP error_reporting should be 32767 (E_ALL) from drush.ini'
+ );
+ }
+ finally {
+ $this->logSubstep('Restore drush.ini to original state.');
+ $this->fileRestore($ini_file);
+ $this->syncToContainer($ini_file);
+ $this->logStepFinish();
+ }🤖 Prompt for AI Agents
In @.vortex/tests/phpunit/Traits/Subtests/SubtestDockerComposeTrait.php around
lines 278 - 321, Wrap the parts of subtestDockerComposeDrushPhpIni that modify
drush/php-ini/drush.ini and run assertions in a try/finally so the original file
is always restored: call fileBackup($ini_file) before modifications, then in a
finally block call fileRestore($ini_file) and syncToContainer($ini_file) to
ensure restoration and container sync even if an assertion (or any exception)
fails; keep existing cmd/assert checks inside the try and ensure the final
logStepFinish() remains after restoration.
Comment on lines
+14
to
+18
| # Allow custom PHP runtime configuration for Drush CLI commands. | ||
| # The leading colon appends to the default scan directories. | ||
| # @see https://github.com/drevops/vortex/issues/1913 | ||
| PHP_INI_SCAN_DIR="${PHP_INI_SCAN_DIR:-}:$(pwd)/drush/php-ini" | ||
| export PHP_INI_SCAN_DIR |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat docs/deployment.mdRepository: drevops/vortex
Length of output: 1704
🏁 Script executed:
cat -n hooks/library/provision.shRepository: drevops/vortex
Length of output: 848
Ensure PHP_INI_SCAN_DIR preserves default scan directories when already set.
If PHP_INI_SCAN_DIR is already defined in the environment without a leading colon (possible from Acquia), the current assignment concatenates without a leading colon, causing PHP to skip its default configuration directories. Normalize the value to ensure a leading colon is always present before appending.
Suggested fix
-# Allow custom PHP runtime configuration for Drush CLI commands.
-# The leading colon appends to the default scan directories.
-# `@see` https://github.com/drevops/vortex/issues/1913
-PHP_INI_SCAN_DIR="${PHP_INI_SCAN_DIR:-}:$(pwd)/drush/php-ini"
-export PHP_INI_SCAN_DIR
+# Allow custom PHP runtime configuration for Drush CLI commands.
+# The leading colon appends to the default scan directories.
+# `@see` https://github.com/drevops/vortex/issues/1913
+ini_scan_dir="${PHP_INI_SCAN_DIR:-}"
+if [ -n "${ini_scan_dir}" ] && [[ "${ini_scan_dir}" != :* ]]; then
+ ini_scan_dir=":${ini_scan_dir}"
+fi
+PHP_INI_SCAN_DIR="${ini_scan_dir:-:}:$(pwd)/drush/php-ini"
+export PHP_INI_SCAN_DIR📝 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
| # Allow custom PHP runtime configuration for Drush CLI commands. | |
| # The leading colon appends to the default scan directories. | |
| # @see https://github.com/drevops/vortex/issues/1913 | |
| PHP_INI_SCAN_DIR="${PHP_INI_SCAN_DIR:-}:$(pwd)/drush/php-ini" | |
| export PHP_INI_SCAN_DIR | |
| # Allow custom PHP runtime configuration for Drush CLI commands. | |
| # The leading colon appends to the default scan directories. | |
| # `@see` https://github.com/drevops/vortex/issues/1913 | |
| ini_scan_dir="${PHP_INI_SCAN_DIR:-}" | |
| if [ -n "${ini_scan_dir}" ] && [[ "${ini_scan_dir}" != :* ]]; then | |
| ini_scan_dir=":${ini_scan_dir}" | |
| fi | |
| PHP_INI_SCAN_DIR="${ini_scan_dir:-:}:$(pwd)/drush/php-ini" | |
| export PHP_INI_SCAN_DIR |
🤖 Prompt for AI Agents
In `@hooks/library/provision.sh` around lines 14 - 18, The PHP_INI_SCAN_DIR
assignment can drop the default scan dirs when the existing value lacks a
leading colon; update the logic in hooks/library/provision.sh around the
PHP_INI_SCAN_DIR assignment so that you normalize the existing PHP_INI_SCAN_DIR
by ensuring it begins with a colon (if non-empty and not already starting with
':') before appending :$(pwd)/drush/php-ini, then export PHP_INI_SCAN_DIR;
reference the PHP_INI_SCAN_DIR variable in your change.
Member
Author
|
Member
Author
|
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #2268 +/- ##
==========================================
- Coverage 84.19% 76.46% -7.73%
==========================================
Files 75 107 +32
Lines 3751 5834 +2083
Branches 44 0 -44
==========================================
+ Hits 3158 4461 +1303
- Misses 593 1373 +780 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/1913-drush-php-ini
branch
from
a19b80c to
275b793
Compare
|
Member
Author
|
Member
Author
|
Member
Author
|
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.
Summary
drush/php-ini/drush.iniwith configurable PHP runtime settings (default:memory_limit=512M) that are auto-discovered viaPHP_INI_SCAN_DIR..docker/cli.dockerfileto setPHP_INI_SCAN_DIRso the CLI container automatically picks up the custom ini file.hooks/library/provision.shto exportPHP_INI_SCAN_DIRfor Acquia deployments..vortex/docs/content/tools/drush.mdxexplaining how to modify Drush PHP runtime configuration.SubtestDockerComposeTraitthat verifies ini values are applied and can be changed.Closes #1913
Test plan
drush/php-ini/drush.iniexists withmemory_limit = 512M.php -r "echo ini_get('memory_limit');"returns512Minside the CLI container.memory_limitindrush.ini, sync to container, and verify the new value is picked up.subtestDockerComposeDrushPhpIni()passes in CI.Summary by CodeRabbit
New Features
Documentation
Tests