Converted provision script to PHP. #2222
Draft
+1,314
−0
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
WalkthroughA new PHP-based Drupal site provisioning script is introduced with comprehensive unit test coverage. The script supports provisioning from database dumps or Drupal profiles, with configurable options for sanitization, custom scripts, and maintenance mode management via environment variables. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Provision Script
participant Env as Environment
participant Drush as Drush
participant DB as Database
participant Config as Config System
Script->>Env: Read VORTEX_ variables
Env-->>Script: Config: path, type, skip, etc.
Script->>Drush: drush status (check installed)
Drush-->>Script: Site status
alt Provision from Database
Script->>DB: Check dump exists
Script->>Drush: sql:drop (if existing)
Script->>Drush: sql:import (from dump)
Script->>DB: DB imported
else Provision from Profile
Script->>Drush: site:install (profile)
Drush-->>Script: Site installed
end
Script->>Script: Enable maintenance mode (if configured)
rect rgb(200, 220, 240)
Note over Script,Config: Post-Provision Operations
Script->>Drush: deployment/updatedb
Script->>Config: Import config_split (if available)
Script->>Drush: cache:rebuild
end
Script->>DB: Sanitize database (unless skipped)
Script->>Script: Run custom provision scripts
Script->>Script: Disable maintenance mode
Script-->>Script: Provisioning complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom Pre-merge checks in the settings. Comment |
AlexSkrypnyk
force-pushed
the
project/2.x
branch
from
388ca6b to
350145a
Compare
github-actions
bot
added
the
CONFLICT
Member
Author
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 71
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.vortex/CLAUDE.md (2)
759-766: Inconsistent system count in directory structure summary.The header and description still reference "Three Separate Systems" and "three completely independent subsystems", but the document has been updated elsewhere to reflect four subsystems (docs, installer, tooling, tests).
🔎 Proposed fix
-### Test Harness (.vortex/) - Three Separate Systems +### Test Harness (.vortex/) - Four Separate Systems -**Critical Understanding**: The `.vortex/` directory contains three **completely independent subsystems**: +**Critical Understanding**: The `.vortex/` directory contains four **completely independent subsystems**: 1. **`.vortex/docs/`** - Docusaurus website (Node.js/React) 2. **`.vortex/installer/`** - PHP installer application (Symfony Console) -3. **`.vortex/tests/`** - Template testing harness (PHPUnit + BATS) +3. **`.vortex/tooling/`** - PHP helpers and notification scripts +4. **`.vortex/tests/`** - Template testing harness (PHPUnit + BATS)
774-812: Directory tree missing tooling subsystem.The directory tree structure in this section lists only three subsystems (docs, installer, tests) but should include the new tooling subsystem to match the updated documentation.
🔎 Proposed fix
Add the tooling directory between installer and tests in the tree structure:
│ ├── installer/ # 2. TEMPLATE INSTALLER │ ├── src/ # Installer source code (PHP) ... │ └── installer.php # Main installer entry point │ +├── tooling/ # 3. TOOLING PACKAGE +│ ├── src/ # PHP scripts and helpers +│ │ ├── helpers.php # Shared helper functions +│ │ └── notify-* # Notification scripts +│ ├── tests/ # Tooling tests +│ │ ├── Unit/ # Unit tests +│ │ ├── Self/ # Self tests for mocking infrastructure +│ │ └── Fixtures/ # Test fixtures +│ └── CLAUDE.md # Tooling documentation +│ -└── tests/ # 3. TEMPLATE TESTING +└── tests/ # 4. TEMPLATE TESTING
🤖 Fix all issues with AI agents
In @.vortex/tooling/.gitattributes:
- Around line 1-2: Remove the duplicated export-ignore entry by keeping a single
"/.gitattributes export-ignore" line and deleting the second identical line
so the file contains only one export-ignore rule for .gitattributes.
In @.vortex/tooling/check-no-exit.php:
- Around line 75-78: The file defines and enforces using quit() but still calls
exit(1) and exit(0) at the end; add a quit() implementation (or import)
immediately after the declare statement so it exists in this script, then
replace the two exit(...) calls (the exit(1) and exit(0) near the end) with
quit(1) and quit(0) respectively to make the script consistent with the rule it
enforces.
In @.vortex/tooling/CLAUDE.md:
- Around line 77-79: The doc signatures for request_get, request_post, and
request use `$deploy_webhook_url` but the actual implementations expect `$url`;
update all occurrences in the documentation (including the signatures at lines
shown and the mock function signatures and examples elsewhere) to use `$url` so
the parameter name matches the implementation, ensuring consistency in
request_get, request_post, and request and any related examples or mocks that
call them.
In @.vortex/tooling/phpcs.xml:
- Around line 17-24: Remove the redundant child <file> entries under the same
directory: since the XML contains a <file>src</file> entry that already
recursively includes subdirectories, delete the explicit
<file>src/notify</file>, <file>src/notify-email</file>,
<file>src/notify-github</file>, <file>src/notify-jira</file>,
<file>src/notify-newrelic</file>, <file>src/notify-slack</file>, and
<file>src/notify-webhook</file> lines from the phpcs.xml snippet so only the
top-level <file>src</file> remains, avoiding duplicate scanning.
In @.vortex/tooling/playground/.env:
- Line 25: The PLAYGROUND_MULTIWORD env entry contains unquoted spaces which can
be parsed inconsistently by different .env parsers; update the
PLAYGROUND_MULTIWORD assignment so the multi-word value is wrapped in quotes
(or, if the intent is to demonstrate parser differences, add an inline comment
clarifying the expected behavior) to ensure predictable parsing across tools.
In @.vortex/tooling/playground/request.php:
- Around line 19-26: The demo prints a body preview for Test 1 without checking
success, which can show a misleading string when request_get returns false;
update the Test 1 block to check $result['ok'] (like Tests 2 and 3) before
accessing or previewing $result['body'], and print an error message or the
status when not ok so the script only casts and substr the body on successful
responses.
In @.vortex/tooling/rector.php:
- Around line 49-72: The skip list in withSkip([...]) currently excludes many
Rector rules (e.g., CatchExceptionNameMatchingTypeRector,
CompleteDynamicPropertiesRector, RenameParamToMatchTypeRector); add brief
rationale comments next to any non-obvious skips and a top-level TODO comment
referencing a review cadence (e.g., "review skip list quarterly" with owner or
issue number) so future maintainers know why rules were disabled and when to
re-evaluate them; ensure the comments reference the specific rule class names
and update project documentation or create a tracking issue if one does not
exist.
- Around line 37-48: The withPaths configuration currently lists several
non-existent directories (the entries under '/src/notify*') and omits the actual
PHP source file; update the array passed to withPaths so it removes the invalid
'/src/notify', '/src/notify-email', '/src/notify-github', '/src/notify-jira',
'/src/notify-newrelic', '/src/notify-slack', and '/src/notify-webhook' entries
and include the real source file '/src/helpers.php' instead (keeping existing
entries like '/check-no-exit.php', '/playground', and '/tests'), so the
withPaths(...) call references only existing files and the actual code to be
analyzed.
- Around line 73-75: The comment states the config targets PHP 8.3 but the code
uses withPhpSets(php82: TRUE); either update that call to withPhpSets(php83:
TRUE) to match the comment (and enable Rector rules up to PHP 8.3) or change the
comment to reference PHP 8.2 and “up to 8.2”; locate the withPhpSets(php82:
TRUE) invocation and modify it or update the surrounding comment accordingly so
the code and documentation match.
In @.vortex/tooling/src/deploy:
- Around line 69-79: The code uses explode(',', ...) on
VORTEX_DEPLOY_SKIP_BRANCHES and VORTEX_DEPLOY_SKIP_PRS producing [''] when the
env var is empty, which can cause unintended matches; update the processing of
$deploy_allow_skip_branches and $deploy_allow_skip_prs (the lines that call
getenv_default(...) then array_map('trim', explode(',', ...))) to trim and then
filter out empty strings (e.g., via array_filter with a non-empty predicate) so
the resulting arrays are truly empty when the env var is empty before they are
used in the subsequent in_array checks.
In @.vortex/tooling/src/deploy-artifact:
- Around line 121-130: The current construction of $git_artifact_cmd uses a
hardcoded Composer bin path (~/.composer/vendor/bin/git-artifact) which is not
portable; replace that literal with a dynamically resolved Composer bin
directory by invoking "composer global config bin-dir --absolute" (e.g., via
shell_exec) and validating the result, falling back to getenv('COMPOSER_HOME') .
'/vendor/bin', getenv('HOME') . '/.config/composer/vendor/bin', then
getenv('HOME') . '/.composer/vendor/bin' if the command fails; use the resolved
path + '/git-artifact' (wrapped in escapeshellarg) when building
$git_artifact_cmd and ensure you handle an empty or non-existent bin dir by
throwing/logging an error before composing the final command.
In @.vortex/tooling/src/deploy-lagoon:
- Around line 261-265: Duplicate logic sets the VORTEX_PROVISION_OVERRIDE_DB
variable to 0 in two places; extract that logic into a single helper function
(e.g., lagoon_reset_db_override_flag(string $environment): void) that calls
task('Setting a DB overwrite flag to 0.') and invokes lagoon_run_cli(...) with
the escaped environment, then replace both inline blocks that use lagoon_run_cli
with calls to lagoon_reset_db_override_flag($deploy_pr_full) and
lagoon_reset_db_override_flag($deploy_branch) respectively.
- Line 287: Replace the hardcoded sleep(10) calls with a configurable wait
variable: add a new integer variable (e.g., $deploy_lagoon_queue_wait)
initialized from getenv_default('VORTEX_DEPLOY_LAGOON_QUEUE_WAIT', '10') near
the other env var parsing, then change both occurrences of sleep(10) to
sleep($deploy_lagoon_queue_wait) so the deployment queue wait time can be tuned
via the environment.
- Line 198: The function signature for lagoon_get_existing_environments is
missing spacing; change "function lagoon_get_existing_environments(bool
$is_branch):array{" to use proper spacing around the return type and brace, e.g.
"function lagoon_get_existing_environments(bool $is_branch): array {" (ensure a
space after the closing parenthesis and a space between the colon and return
type and before the opening brace).
- Line 234: The current check using str_contains($output, 'exceed') is too
generic; change it to detect a specific Lagoon error message or code
instead—e.g., replace the substring check on 'exceed' with a check for the exact
phrase like 'environment limit exceeded' (case-insensitive) or use a regex
(preg_match) to match the precise Lagoon error text, or better yet parse and
check the Lagoon API error code if available; update the conditional that
references str_contains($output, 'exceed') to use the more specific match so
unrelated output (e.g., "exceeded expectations") won't trigger this branch.
In @.vortex/tooling/src/deploy-webhook:
- Around line 38-44: The failure handling uses identical messages making root
cause unclear; update the two fail() calls to include diagnostic details by
including response error/body when $response['ok'] is false (e.g., append
$response['error'] or json_encode($response) so you capture the failure reason)
and by including expected vs actual status for the status-mismatch branch (use
$deploy_webhook_expected_status and (string)$response['status'] to format
"expected X, got Y"); keep using the existing fail() call but pass a descriptive
message that includes these variables to aid debugging.
In @.vortex/tooling/src/helpers.php:
- Around line 287-291: The command_exists function is vulnerable to command
injection because it interpolates unescaped $command into the shell string;
update the function to sanitize the input by wrapping the command argument with
escapeshellarg($command) when building the exec string (e.g., use
sprintf('command -v %s 2>/dev/null', escapeshellarg($command))) and optionally
validate the input with a strict regex (e.g., allow only [A-Za-z0-9._-]+) before
calling exec to further reduce risk.
- Around line 31-33: helpers.php currently calls load_dotenv(...) at require
time which causes side effects; change this to guard against repeated or
implicit loading by either 1) wrapping the call in a new explicit function
(e.g., ensure_dotenv_loaded() or initEnv()) that calls load_dotenv(['.env',
'.env.local']) only when invoked, or 2) add an idempotent guard around the
top-level call that checks a sentinel (e.g., defined('DOTENV_LOADED') or
getenv('DOTENV_LOADED')) before calling load_dotenv and sets that sentinel after
loading; update any code that relied on auto-loading to call the new function
explicitly.
- Around line 274-276: The term_supports_color function uses a nested ternary
that reduces readability; refactor it to use a clear if/return structure: first
capture getenv('TERM') into a variable, return false immediately if that
variable is 'dumb' or === false, otherwise return the result of
function_exists('posix_isatty') && @posix_isatty(STDOUT); this preserves
behavior but makes the logic in term_supports_color explicit and easier to read.
In @.vortex/tooling/src/login-container-registry:
- Around line 38-40: The check using file_exists($config_file) &&
str_contains(file_get_contents($config_file), $registry) can yield false
positives for partial matches; instead read and json_decode the config file and
test for an exact key in the auths object (or, if you prefer a simpler change,
perform a stricter check against quoted JSON keys like '"'.$registry.'"' ), i.e.
replace the str_contains-based check with logic that decodes the file content
into an array and verifies isset($decoded['auths'][$registry]) (or uses the
stricter quoted-key string match) before emitting the "Already logged in" note.
- Around line 37-40: The check uses file_get_contents(...) directly inside
str_contains, which can pass false to str_contains and cause a TypeError; first
read the config into a variable (e.g. $contents =
file_get_contents($config_file)) or use is_readable($config_file), then verify
$contents !== false before calling str_contains($contents, $registry); only call
note('Already logged in...') when the safe str_contains check returns true.
In @.vortex/tooling/src/notify:
- Line 66: The assignment to $script uses unnecessary parentheses around the
concatenation; update the statement that sets $script (the line creating __DIR__
. ('/notify-' . $channel)) to remove the outer parentheses so it reads a simple
concatenation of __DIR__ and '/notify-' . $channel, keeping the same variables
($script, $channel) and behavior.
In @.vortex/tooling/src/notify-email:
- Around line 119-133: The timestamp string is generated twice when calling
replace_tokens for $email_subject and $email_message; extract the timestamp into
a single variable (e.g., $timestamp = date('d/m/Y H:i:s T')) and use that
variable in both replace_tokens calls so both replacements reference the same
value and duplication is removed (update the arrays passed to replace_tokens for
$email_subject and $email_message to use the $timestamp variable).
- Around line 90-107: The parse_email_recipients function must reject invalid
emails and strip header-injection chars from names: validate each $email with
FILTER_VALIDATE_EMAIL and skip entries that fail, and sanitize the $name by
removing/control-filtering CRLF, quotes and angle brackets (or replace them with
safe characters) before formatting; ensure you never include unsanitized input
in the '"Name" <email>' output and only return entries for validated emails
(update parse_email_recipients to perform these checks on $email and $name and
skip or log malformed recipients).
- Around line 158-179: The current loop sends one mail per $to recipient,
exposes BCC to each TO and causes duplicate CC deliveries, and passes an array
to mail() instead of a header string; fix by removing the per-recipient loop and
send a single email: build $to = implode(', ', $to_list), construct $headers as
a single string (start with "From: $email_from\r\nContent-Type: text/plain;
charset=UTF-8\r\n" and append "Cc: " . implode(', ', $cc_list) . "\r\n" and
"Bcc: " . implode(', ', $bcc_list) . "\r\n" only when non-empty), call mail($to,
$email_subject, $email_message, $headers_string) and then populate $sent with
parsed addresses from $to_list (and include CC/BCC lists if you track them) so
headers are correctly formatted and recipients receive one message each without
BCC/CC duplication or privacy leaks.
In @.vortex/tooling/src/notify-github:
- Around line 108-127: The code currently picks the first element from the
deployments payload (reset($payload)) which can be wrong if GitHub returns items
out of order; instead sort or scan the decoded $payload for the most recent
deployment (compare the deployment's "created_at" or "updated_at" timestamp) and
choose the newest one before assigning $deployment_id, or alternatively validate
the chosen deployment's timestamp is within an acceptable window (N minutes) and
fail if not; update the logic around request_get/$response processing and the
assignment of $deployment_id (and keep using deployment_id_is_valid to gate the
final check) and add a short comment explaining the chosen strategy.
- Around line 56-66: The function deployment_id_is_valid currently rejects valid
GitHub deployment IDs by enforcing a 9–11 digit length; update it to accept any
non-empty numeric ID by removing the length checks and returning a boolean based
solely on the string being non-empty and entirely digits (i.e., use the existing
$id_str and ctype_digit($id_str) and drop the $length >= 9 && $length <= 11
condition).
In @.vortex/tooling/src/notify-jira:
- Around line 137-139: Extract the magic number 24 into a named constant (e.g.,
JIRA_ACCOUNT_ID_MIN_LENGTH = 24) placed near the top of the script after the
configuration section, then replace both occurrences of the literal 24 used for
account ID length checks (the if that checks strlen((string) $account_id) and
the other check around lines 249-251) with that constant so the validation reads
like strlen((string) $account_id) < JIRA_ACCOUNT_ID_MIN_LENGTH and the
fail('Unable to authenticate') logic remains unchanged.
- Around line 155-159: The ADF document assigned to $comment_body uses 'content'
=> $jira_message_adf but text_to_adf() returns a single node object rather than
an array; change the assignment so 'content' is an array of nodes (e.g., wrap
$jira_message_adf in an array) or alter text_to_adf() to return an array of
content blocks so $comment_body['content'] is always an array.
- Around line 255-268: The PUT request that assigns the Jira issue (building
$payload_data and $url and calling request(...)) currently ignores the response,
so failures are silent; update the code that calls request for assignment to
capture and validate the response (like the transition request does), check HTTP
status / response body for errors, and call fail(...) or log/process the error
before returning so unsuccessful assignments are reported instead of proceeding
to pass('Assigned issue to ' . $jira_assignee_email);.
In @.vortex/tooling/src/notify-newrelic:
- Around line 107-128: The "Discovered application ID" message is printed
unconditionally when $newrelic_appid is initially empty and $newrelic_app_name
is set, even if discovery failed; update the logic so the pass('Discovered
application ID: %s', $newrelic_appid) call only runs after a successful
discovery (i.e., when $newrelic_appid is truthy), by moving that log into the
block where $data['applications'][0]['id'] is set or by wrapping it with an if
($newrelic_appid) check; keep existing failure handling (pass(... skip ...) +
quit()) unchanged.
In @.vortex/tooling/src/notify-slack:
- Around line 97-102: The array_splice call is inserting a nested array into
$fields, producing a wrong nested element; fix by inserting the two field
objects as separate elements instead of a wrapped container — either use PHP
array unpacking (e.g. $fields = [...$before, $envField, $loginField, ...$after])
or pass the insertion array correctly to array_splice so it inserts the two
associative arrays as distinct entries (build an $insert array with the two
field arrays and call array_splice($fields, 1, 0, $insert)); update the code
around the array_splice($fields, 1, 0, ...) usage and keep the $notify_event !==
'pre_deployment' condition intact.
In @.vortex/tooling/src/notify-webhook:
- Line 93: The payload preview unconditionally appends "..."; change the note
call around the expression using $webhook_payload and substr (or better
mb_substr/mb_strlen) so the ellipsis is only added when the payload length
exceeds 200 characters (e.g. check mb_strlen($webhook_payload) > 200 and use
mb_substr($webhook_payload, 0, 200) when true, otherwise use the full payload
without "..."). Ensure you reference the existing note(...) invocation and the
$webhook_payload/ substr usage when making the change.
In @.vortex/tooling/src/provision:
- Around line 361-432: The nested provisioning branch is hard to maintain due to
duplication between the "database" and "profile" flows; extract the logic into
focused helper functions (e.g., provision_database_with_existing_site(),
provision_database_fresh(), provision_profile_with_existing_site(),
provision_profile_fresh()) and move the shared actions (setting
$provision_override_db, putenv('VORTEX_PROVISION_OVERRIDE_DB=1'), setting
$provision_sanitize_db_skip and putenv('VORTEX_PROVISION_SANITIZE_DB_SKIP=1'),
and calls to provision_from_db() / provision_from_profile()) into those helpers;
replace the large if/else block with a small dispatcher that calls the
appropriate helper based on $provision_type and $site_is_installed (and checks
$db_image and $provision_override_db), preserving existing behavior and the same
variable side-effects.
- Line 436: The drush php eval string uses a wrong class name casing: replace
the incorrect "\Drupal\core\Site\Settings" with the correctly cased
"\Drupal\Core\Site\Settings" inside the drush("php:eval ...") call that assigns
$environment so the call to Site\Settings::get('environment') resolves properly.
- Line 231: The current fragile replacement
str_replace('./','../',$additional_file) should be replaced with a robust
resolution: resolve $additional_file and the base (current working dir or Drupal
root) with realpath() (e.g., $absFile = realpath($additional_file) and $base =
realpath(getcwd() or DRUPAL_ROOT)), then compute the relative path from $base to
$absFile by removing the common prefix or using a small helper to walk up
directories until a common ancestor is found; if realpath() fails, fall back to
the original $additional_file or normalize with rtrim/trim and ensure you only
replace a leading './' (e.g., check strpos($additional_file, './') === 0) to
avoid accidental replacements — update the code that sets $relative_file
accordingly so you stop using str_replace('./','../',$additional_file).
In @.vortex/tooling/src/setup-ssh:
- Around line 69-90: The conversion block that handles SHA256 fingerprints (the
if (str_starts_with($fingerprint, 'SHA256:')) { ... } section) silences
ssh-keygen errors with "2>/dev/null" and does not log when no matching id_rsa*
file is found; update the code to capture or surface ssh-keygen stderr (remove
or redirect the "2>/dev/null" and capture output) and after the foreach over
$id_rsa_files add an explicit log call (e.g., task() or fail()) indicating that
no matching key file was found during SHA256→MD5 conversion for the given
$fingerprint so callers can distinguish conversion failure from a missing key
file.
In @.vortex/tooling/tests/Fixtures/test-shell-exec-passing:
- Around line 22-24: The elseif branch checking "($result === FALSE)" is
unreachable because shell_exec() returns string|null, so remove the entire
elseif block and its echo; instead ensure only the null check remains (e.g., if
($result === null) { ... }) and leave handling of $result as the returned string
otherwise; reference: the call to shell_exec() that sets $result and the
unreachable "elseif ($result === FALSE)" branch should be deleted.
In @.vortex/tooling/tests/Self/CheckNoExitSelfTest.php:
- Line 49: The exec() invocation builds a shell command with sprintf() using
$this->testDir and $this->scriptPath, which can lead to command injection if
those contain shell metacharacters; update the command construction to wrap
these variables with escapeshellarg() before passing to exec() (i.e., use
escapeshellarg($this->testDir) and escapeshellarg($this->scriptPath) when
constructing the string passed to exec()), and apply the same change to every
exec() usage in this test class that currently formats paths via sprintf().
- Around line 29-36: Add a tearDown() method to remove the temporary test
directory created by setUp(); specifically, implement tearDown() in the same
test class to check for and recursively delete $this->testDir (and its
subdirectories) after each test so tests don’t leak files under
self::$tmp—ensure it calls parent::tearDown() and safely handles missing
directories/errors to avoid failing tests.
In @.vortex/tooling/tests/Self/MockInfrastructureSelfTest.php:
- Around line 18-25: In testAssertMockConsumedWithNonExistentMock move the
PHPUnit call expectNotToPerformAssertions() to the very start of the test method
so it is registered before any test actions; keep the three mock assertion calls
(mockPassthruAssertAllMocksConsumed, mockMailAssertAllMocksConsumed,
mockRequestAssertAllMocksConsumed) exactly as they are but after
expectNotToPerformAssertions().
In @.vortex/tooling/tests/Self/MockMailSelfTest.php:
- Around line 18-30: Move the require_once __DIR__ . '/../../src/helpers.php'
call out of testMockMailSuccess() and into the test class setUp() method so
helpers.php is included once consistently for all tests; update the class to add
or modify setUp() to call require_once before tests run and remove the inline
require_once from testMockMailSuccess to match other tests like
NotifyNewrelicTest and DeployTest.
In @.vortex/tooling/tests/Self/MockPassthruSelfTest.php:
- Around line 162-206: In testMockPassthruMultipleMoreCallsFailure the variables
$return3 and $output3 are assigned but never used; remove the unused assignments
by calling passthru('echo "success3"', $exit_code3); (no $return3) and call
ob_get_clean(); (no $output3) or ob_end_clean() to clear the buffer—keep the
ob_start()/passthru() invocation to trigger the exception but avoid storing the
unused $return3/$output3 variables.
In @.vortex/tooling/tests/Self/MockQuitSelfTest.php:
- Around line 61-107: The four test methods (testMockQuitScript0Success,
testMockQuitScript0Failure, testMockQuitScript1Success,
testMockQuitScript1Failure) contain assertions after $this->runScript(...) but
you call $this->expectException(...) so the thrown Quit* exception makes those
assertions unreachable; either remove the post-run assertions or catch the
exception around the runScript call to capture $output and then assert on it.
Locate the tests by name and modify each to either (a) remove the
assertStringContainsString/assertStringNotContainsString lines, or (b) wrap
$this->runScript(...) in a try/catch for the expected exception class, capture
the output in the try block and perform the string assertions in the catch or
after rethrowing, ensuring you still assert the exception code/type via explicit
assertions instead of expectException.
In @.vortex/tooling/tests/Self/MockRequestSelfTest.php:
- Around line 470-477: The manual call to mockRequestAssertAllMocksConsumed()
after runScript() is redundant and unreachable because the expected exception is
thrown by the automatic assertion in tearDown (mockTearDown); remove the
explicit mockRequestAssertAllMocksConsumed() invocation so the test relies on
the expectException/expectExceptionMessage and the automatic tearDown assertion
triggered after runScript() completes.
In @.vortex/tooling/tests/Traits/MockTrait.php:
- Around line 613-626: The mockRequestGet method currently declares unused
parameters $headers and $timeout; remove these parameters from the signature of
mockRequestGet (so it only accepts string $url, ?array $response = NULL, string
$namespace = 'DrevOps\\VortexTooling') and update any callers/tests accordingly,
leaving the body to call mockRequestMultiple exactly as it does now;
alternatively, if you must keep them for API symmetry, add a short docblock or
inline comment on mockRequestGet explaining that $headers and $timeout are
intentionally unused and will not affect the mock, so callers are not misled.
In @.vortex/tooling/tests/Unit/DeployContainerRegistryTest.php:
- Around line 222-236: The testLoginFailure currently calls mockPassthru(...)
then mockQuit(1) and sets expectException(QuitErrorException::class) and
expectExceptionCode(1) before invoking
runScript('src/deploy-container-registry'); replace that manual setup with the
runScriptError helper: keep the mockPassthru(...) call but remove mockQuit(...)
and the expectException/...Code lines and call
runScriptError('src/deploy-container-registry', 1) (or the appropriate overload)
so the test uses the same helper pattern as the other failure tests.
- Around line 238-265: The testPushFailure method duplicates manual quit and
exception assertions; replace the manual mockQuit(1),
expectException(QuitErrorException::class) and expectExceptionCode(1) plus the
direct runScript('src/deploy-container-registry') call with the runScriptError
helper to keep behavior consistent—invoke runScriptError for
'src/deploy-container-registry' with the expected exit code (1) so the helper
handles the quit mocking and exception expectations for you.
In @.vortex/tooling/tests/Unit/DeployLagoonTest.php:
- Around line 429-432: The test currently calls runScript which throws
QuitErrorException (expected via expectException), so the subsequent
assertStringContainsString('[FAIL] Lagoon CLI command', $output) is unreachable;
either remove that assertion, or restructure the test to catch the
QuitErrorException instead of using expectException so you can capture $output
and then assert on it. Specifically, change the test to wrap the
runScript('src/deploy-lagoon') call in a try/catch that catches
QuitErrorException, assigns the script output to $output inside the try (or
captures output before rethrowing), and then call assertStringContainsString
with that $output (or alternatively delete the unreachable assert if you keep
expectException).
- Around line 512-515: The test currently calls runScript('src/deploy-lagoon')
while expecting QuitErrorException, which makes the subsequent
assertStringContainsString unreachable; change the test to catch the
QuitErrorException from runScript and then assert the expected message (or
remove the expectException and assert the output/message). Specifically, wrap
the runScript call in a try/catch for QuitErrorException, capture the exception
message (e.g. $e->getMessage()) or any returned output, and then use
assertStringContainsString('Lagoon environment limit exceeded.', ...) to verify
the message; reference runScript and QuitErrorException and the existing
assertStringContainsString when making the change.
In @.vortex/tooling/tests/Unit/DeployTest.php:
- Around line 321-335: The helpers getDeployArtifactPath, getDeployWebhookPath,
getDeployContainerRegistryPath, and getDeployLagoonPath use (string)
realpath(...) which silently yields '' on failure; update each to capture the
realpath result into a variable, check that it !== false, and if it is false
throw a clear RuntimeException (or fail the test) including the original
expected path in the message so missing paths are obvious and tests fail fast.
In @.vortex/tooling/tests/Unit/DeployWebhookTest.php:
- Around line 83-87: The test testUnexpectedResponseStatus currently only
asserts the generic error via runScriptError('src/deploy-webhook', 'Unable to
complete webhook deployment.'); update the test to assert the more specific
error message the script emits (e.g. include expected vs actual HTTP status) by
replacing or adding an assertion that matches the detailed message (for example
using runScriptError with 'Expected status 200 but received 201' or a regex
matching that pattern); locate the test by the test method name
testUnexpectedResponseStatus and the mockRequest call to adjust the expected
error text to reflect the actual error produced by the deploy-webhook script.
In @.vortex/tooling/tests/Unit/HelpersCopyDirTest.php:
- Around line 144-165: The test testCopyDirDestinationAlreadyExists checks that
files are preserved but doesn't explicitly assert that the source directory is
merged rather than replacing the destination; update the
testCopyDirDestinationAlreadyExists test (and optionally reuse
createDirectoryStructure and copy_dir) to add a clear comment like "Verify merge
behavior: existing files are preserved, new files are added" and include
explicit assertions that (1) files present only in the source appear in the
destination after copy, (2) files present only in the destination remain
unchanged, and (3) nested source subdirectories and files are added (e.g.,
create a nested dir in the source, call copy_dir, then assert the nested file
exists in dst and its contents match).
In @.vortex/tooling/tests/Unit/HelpersRequestTest.php:
- Around line 85-90: The test testRequestNotFound calls
\DrevOps\VortexTooling\request('http://example.com/nonexistent-page-12345') and
asserts an exact 404 which is brittle; change the assertion on $result['status']
to verify it represents an HTTP error range instead (e.g., assert that it's an
integer between 400 and 599 or assertGreaterThanOrEqual(400) and
assertLessThan(600)), and keep the existing $this->assertFalse($result['ok'])
check; update the assertion in testRequestNotFound accordingly so it no longer
depends on example.com returning exactly 404.
In @.vortex/tooling/tests/Unit/HelpersStringMapTest.php:
- Around line 35-45: The test only checks for the '[FAIL]' marker when
$expected_error is set but doesn't assert the actual error message; modify the
error branch to validate the specific message by adding an expectation for the
exception message and/or output: when $expected_error !== NULL keep
$this->expectException(QuitErrorException::class) and add
$this->expectExceptionMessage($expected_error) (or $this->expectOutputRegex('/'
. preg_quote($expected_error, '/') . '/')) so the failure text from
string_map_to_array is verified exactly.
In @.vortex/tooling/tests/Unit/HelpersTokenTest.php:
- Around line 32-55: Add edge-case entries to dataProviderReplaceTokens to cover
JSON-escaping and encoding behaviors used by replace_tokens: include cases where
replacement values contain quotes, newlines, backslashes, and mixed special
chars; add a case with an empty replacements array []; add a case with an empty
template string ''; and add cases with Unicode characters in both token names
and values so replace_tokens behavior with UTF-8 is validated. Reference the
data provider method dataProviderReplaceTokens and the replace_tokens helper
when adding these new test rows.
In @.vortex/tooling/tests/Unit/LoginContainerRegistryTest.php:
- Around line 69-81: The login script (src/login-container-registry) should
validate the VORTEX_CONTAINER_REGISTRY value before calling docker: detect if
getenv('VORTEX_CONTAINER_REGISTRY') is null or consists only of whitespace, and
if so print a clear error like "Error: container registry name is empty" to
stderr and exit non‑zero instead of attempting docker login; update the script's
registry variable handling to trim the value, perform the check, and
short‑circuit with the error and exit code so tests and users get a helpful
message rather than a docker failure.
In @.vortex/tooling/tests/Unit/NotifyEmailTest.php:
- Around line 34-51: The test testSuccessfulNotificationSingleRecipient is flaky
because mockMail sets the full 'message' with date('d/m/Y H:i:s T') at setup
time which can differ when runScript() generates the actual email; change the
mockMail call to avoid exact timestamp matching by replacing the 'message' value
with a flexible check (e.g., use the mockMail 'message' =>
$this->callback(function($msg) { return str_contains($msg, '## This is an
automated message ##') && str_contains($msg, 'https://example.com'); }) or a
regex match) or alternatively freeze time before building the expected message;
keep the rest of the assertions the same and update other tests in this file
that use date() similarly.
In @.vortex/tooling/tests/Unit/NotifyJiraTest.php:
- Around line 200-205: The test method testPreDeploymentEventSkipped contains an
extra blank line before its closing brace; remove the stray empty line so the
closing brace follows immediately after the last statement to match the
formatting of other test methods (locate the method named
testPreDeploymentEventSkipped and delete the blank line before its final `}`).
In @.vortex/tooling/tests/Unit/NotifySlackTest.php:
- Around line 212-223: The test method testHttpRequestFailureWrongStatus
contains an extra blank line before the end of the method; remove the redundant
empty line between the final assertion/statement and the closing brace so the
method ends immediately with the closing brace (adjust spacing in
testHttpRequestFailureWrongStatus to match surrounding tests).
In @.vortex/tooling/tests/Unit/ProvisionTest.php:
- Around line 761-768: The path rewrite using str_replace('./', '../', $file) in
the test is fragile because it replaces every './' occurrence and assumes $file
begins with './'; update the logic that computes $relative_file so it only
adjusts a leading './' (e.g., check if $file starts with './' and remove that
prefix then prepend '../'), or otherwise compute a normalized relative path
(using PHP's realpath/dirname logic or an explicit conditional) before passing
it into the mockPassthru cmd string; ensure you update the $relative_file
calculation referenced in the mockPassthru call so it no longer mutates internal
'./' segments or mis-handles paths that don't start with './'.
In @.vortex/tooling/tests/Unit/SetupSshTest.php:
- Around line 202-208: The test reads the SSH config using file_get_contents;
replace that call with the project's File utility for cross-platform consistency
by using the File class to read $config_file (e.g., use File::get($config_file)
instead of file_get_contents) in the test that sets $config_file and assigns
$config_content, ensuring any necessary import/alias for the File class is
present in SetupSshTest.
- Around line 51-55: The test uses native PHP file functions; update it to use
AlexSkrypnyk\File\File for cross-platform compatibility: add the import for
AlexSkrypnyk\File\File and replace mkdir($ssh_dir,...),
file_put_contents($key_file,...), and chmod($key_file,...) with the File class
equivalents when setting up $ssh_dir and $key_file in SetupSshTest (and mirror
the same changes across other test methods in this file); ensure you call the
File API to create the directory with the desired mode, write the key contents
to id_rsa, and set the file permissions using File methods rather than native
functions.
In @scripts/vortex/deploy-lagoon.sh:
- Around line 159-163: The TODO comment referencing uselagoon/lagoon#1922 is
invalid; update the comment above the task "Setting a DB overwrite flag to 0."
and the lagoon update variable command to either: 1) replace the nonexistent
issue reference with the correct Lagoon issue number if you can find it, or 2)
remove the issue link and add a short description of the exact condition that
will allow removal of this workaround (e.g., Lagoon PR/merge, specific bug
behavior fixed, or a test that proves overwrite works), or 3) if the workaround
must remain permanently, document why it is required and when it can be
revisited; ensure changes are in the same comment block that mentions
VORTEX_PROVISION_OVERRIDE_DB and the lagoon update variable command.
Comment on lines
1
to
2
| /.gitattributes export-ignore | ||
| /.gitattributes export-ignore |
There was a problem hiding this comment.
Remove duplicate export-ignore rule.
The /.gitattributes export-ignore rule appears twice on lines 1 and 2.
🔎 Proposed fix
/.gitattributes export-ignore
-/.gitattributes export-ignore
/.gitignore export-ignoreCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @.vortex/tooling/.gitattributes around lines 1 - 2, Remove the duplicated
export-ignore entry by keeping a single "/.gitattributes export-ignore" line
and deleting the second identical line so the file contains only one
export-ignore rule for .gitattributes.
Comment on lines
75
to
78
| exit(1); | ||
| } | ||
|
|
||
| exit(0); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using quit() for consistency.
The script enforces the use of quit() instead of exit() for testability, but uses exit() itself on lines 75 and 78. For consistency with the rule it enforces, consider using quit() here as well—even though this is a utility script, it demonstrates the pattern you're advocating.
🔎 Proposed refactor
Add at the top of the file after the declare statement:
declare(strict_types=1);
+require_once __DIR__ . '/src/helpers.php';
+
// Directories to scan (relative to current working directory).Then replace the exit calls:
if ($errors !== []) {
foreach ($errors as $error) {
echo $error . PHP_EOL;
}
- exit(1);
+ quit(1);
}
-exit(0);
+quit(0);📝 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
| exit(1); | |
| } | |
| exit(0); | |
| declare(strict_types=1); | |
| require_once __DIR__ . '/src/helpers.php'; | |
| // Directories to scan (relative to current working directory). | |
| if ($errors !== []) { | |
| foreach ($errors as $error) { | |
| echo $error . PHP_EOL; | |
| } | |
| quit(1); | |
| } | |
| quit(0); |
🤖 Prompt for AI Agents
In @.vortex/tooling/check-no-exit.php around lines 75 - 78, The file defines and
enforces using quit() but still calls exit(1) and exit(0) at the end; add a
quit() implementation (or import) immediately after the declare statement so it
exists in this script, then replace the two exit(...) calls (the exit(1) and
exit(0) near the end) with quit(1) and quit(0) respectively to make the script
consistent with the rule it enforces.
Comment on lines
1
to
1032
| # Vortex Tooling Package Development Guide | ||
|
|
||
| This document describes the Vortex tooling package - a collection of PHP helper functions and scripts for Vortex notification and utility operations. | ||
|
|
||
| ## Package Overview | ||
|
|
||
| **Location**: `.vortex/tooling/` | ||
|
|
||
| **Purpose**: Provides reusable PHP functions for Vortex scripts, including: | ||
| - Environment variable loading | ||
| - Output formatting (info, task, pass, fail, note) | ||
| - Variable and command validation | ||
| - HTTP requests | ||
| - Token replacement utilities | ||
|
|
||
| **Key Principle**: This is a **standalone PHP package** that can be used by Vortex scripts without requiring Drupal or Composer dependencies at runtime. | ||
|
|
||
| ## Project Structure | ||
|
|
||
| ```text | ||
| .vortex/tooling/ | ||
| ├── src/ | ||
| │ └── helpers.php # Core helper functions | ||
| ├── tests/ | ||
| │ ├── Fixtures/ # Test fixture scripts | ||
| │ │ ├── test-passthru-* # Passthru test fixtures | ||
| │ │ ├── test-quit-* # Quit test fixtures | ||
| │ │ ├── test-request-* # HTTP request test fixtures | ||
| │ │ └── test-exit-* # Exit test fixtures | ||
| │ ├── Self/ # Self-tests for mock infrastructure | ||
| │ │ ├── MockPassthruSelfTest.php | ||
| │ │ ├── MockQuitSelfTest.php | ||
| │ │ └── MockRequestSelfTest.php | ||
| │ ├── Unit/ # Unit tests | ||
| │ │ ├── UnitTestCase.php # Base test case | ||
| │ │ ├── ExitException.php # Exit exception for testing | ||
| │ │ ├── ExitMockTest.php # Exit mocking tests | ||
| │ │ ├── HelpersDotenvTest.php # Environment tests | ||
| │ │ └── SelfTest.php # Self-tests for core functions | ||
| │ └── Traits/ | ||
| │ └── MockTrait.php # Mock infrastructure (passthru, quit, request) | ||
| ├── composer.json # Dev dependencies only | ||
| ├── phpunit.xml # PHPUnit configuration | ||
| └── CLAUDE.md # This file | ||
| ``` | ||
|
|
||
| ## Core Helper Functions | ||
|
|
||
| ### Environment Management | ||
|
|
||
| ```php | ||
| load_dotenv(array $env_files = ['.env']): void | ||
| ``` | ||
| Loads environment variables from .env files. Supports quoted values and comments. | ||
|
|
||
| ### Output Formatters | ||
|
|
||
| ```php | ||
| note(string $format, ...$args): void // Plain note output | ||
| task(string $format, ...$args): void // [TASK] Blue output | ||
| info(string $format, ...$args): void // [INFO] Cyan output | ||
| pass(string $format, ...$args): void // [ OK ] Green output | ||
| fail_no_exit(string $format, ...$args): void // [FAIL] Red output (no exit) | ||
| fail(string $format, ...$args): void // [FAIL] Red output + exit(1) | ||
| ``` | ||
|
|
||
| ### Validation Functions | ||
|
|
||
| ```php | ||
| validate_variable(string $name, ?string $message = NULL): void | ||
| validate_command(string $command): void | ||
| ``` | ||
|
|
||
| ### HTTP Request Functions | ||
|
|
||
| ```php | ||
| request_get(string $deploy_webhook_url, array $headers = [], int $timeout = 10): array | ||
| request_post(string $deploy_webhook_url, $body = NULL, array $headers = [], int $timeout = 10): array | ||
| request(string $deploy_webhook_url, array $options = []): array | ||
| ``` | ||
|
|
||
| **Return Format**: | ||
| ```php | ||
| [ | ||
| 'ok' => bool, // TRUE if HTTP < 400 | ||
| 'status' => int, // HTTP status code | ||
| 'body' => string|false, // Response body | ||
| 'error' => ?string, // cURL error message | ||
| 'info' => array, // cURL info array | ||
| ] | ||
| ``` | ||
|
|
||
| ### Environment Variable Functions | ||
|
|
||
| ```php | ||
| getenv_required(...$var_names): string | ||
| getenv_default(...$args): string | ||
| ``` | ||
|
|
||
| **getenv_required()** - Get value from environment variables with fallback chain: | ||
| - Tries each variable name in order | ||
| - Returns first non-empty value found | ||
| - Calls `fail()` and `quit(1)` if no value found | ||
| - Example: `getenv_required('VAR1', 'VAR2', 'VAR3')` | ||
|
|
||
| **getenv_default()** - Get value with fallback chain and default: | ||
| - Last argument is the default value | ||
| - Tries each variable name in order | ||
| - Returns first non-empty value found, or default if none found | ||
| - Example: `getenv_default('VAR1', 'VAR2', 'default_value')` | ||
|
|
||
| ### Utility Functions | ||
|
|
||
| ```php | ||
| replace_tokens(string $template, array $replacements): string | ||
| is_debug(): bool | ||
| quit(int $code = 0): void // Wrapper around exit() for testing | ||
| ``` | ||
|
|
||
| ## Script Structure Guidelines | ||
|
|
||
| All scripts in `src/` must follow a consistent structure for maintainability and clarity: | ||
|
|
||
| ### Standard Script Structure | ||
|
|
||
| ```php | ||
| #!/usr/bin/env php | ||
| <?php | ||
|
|
||
| /** | ||
| * @file | ||
| * Brief description of what the script does. | ||
| * | ||
| * Additional details about the script's purpose or requirements. | ||
| * | ||
| * IMPORTANT! This script runs outside the container on the host system. | ||
| */ | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace DrevOps\VortexTooling; | ||
|
|
||
| require_once __DIR__ . '/helpers.php'; | ||
|
|
||
| execute_override(basename(__FILE__)); | ||
|
|
||
| // ----------------------------------------------------------------------------- | ||
|
|
||
| // Variable description. | ||
| // | ||
| // Additional details about format, usage, or valid values. | ||
| $var1 = getenv_required('PRIMARY_VAR', 'FALLBACK_VAR'); | ||
|
|
||
| // Another variable description. | ||
| // | ||
| // Can be 'value1', 'value2', etc. | ||
| $var2 = getenv_default('VAR_NAME', 'default_value'); | ||
|
|
||
| // Optional variable with detailed format explanation. | ||
| // | ||
| // Format: key1=value1,key2=value2 | ||
| // Example: web=myorg/myapp,db=myorg/mydb | ||
| $var3 = getenv_default('OPTIONAL_VAR', ''); | ||
|
|
||
| // ----------------------------------------------------------------------------- | ||
|
|
||
| info('Started operation.'); | ||
|
|
||
| // Main script logic here... | ||
|
|
||
| pass('Finished operation.'); | ||
| ``` | ||
|
|
||
| ### Environment Variable Best Practices | ||
|
|
||
| 1. **Use VARIABLES Section**: All environment variable declarations at the top | ||
| 2. **Document Each Variable**: Include comment explaining purpose and format | ||
| 3. **Use Fallback Chains**: `getenv_required()` and `getenv_default()` with multiple variable names | ||
| 4. **Separate Sections**: Use comment dividers to separate VARIABLES from EXECUTION | ||
| 5. **Explicit Validation**: Check for explicitly empty values when needed: | ||
|
|
||
| ```php | ||
| // Check if variable is explicitly set to empty (different from not set). | ||
| if (($check = getenv('VAR_NAME')) !== FALSE && empty(trim($check))) { | ||
| fail('VAR_NAME should not be empty.'); | ||
| quit(1); | ||
| } | ||
| ``` | ||
|
|
||
| ### Variable Documentation Format | ||
|
|
||
| Each variable should have: | ||
| - **Single-line comment**: Brief description | ||
| - **Multi-line comment** (optional): Format details, examples, valid values | ||
| - **Variable assignment**: Using getenv_required() or getenv_default() | ||
|
|
||
| **Example**: | ||
|
|
||
| ```php | ||
| // Email notification recipients. | ||
| // | ||
| // Multiple names can be specified as a comma-separated list of email addresses | ||
| // with optional names in the format "email|name". | ||
| // Example: "[email protected]|Jane Doe, [email protected]|John Doe". | ||
| $email_recipients = getenv_required('VORTEX_NOTIFY_EMAIL_RECIPIENTS'); | ||
|
|
||
| // Email notification subject template. | ||
| // | ||
| // Available tokens: | ||
| // - %project% - Project name | ||
| // - %label% - Deployment label | ||
| // - %timestamp% - Deployment timestamp | ||
| $email_subject = getenv_default('VORTEX_NOTIFY_EMAIL_SUBJECT', '%project% deployment notification'); | ||
| ``` | ||
|
|
||
| ### Real-World Example | ||
|
|
||
| See `src/notify-email` for a complete example following this structure. | ||
|
|
||
| ## Testing Architecture | ||
|
|
||
| ### Test Organization | ||
|
|
||
| The package uses **three types of tests**: | ||
|
|
||
| 1. **Unit Tests** (`tests/Unit/`) - Test individual helper functions | ||
| 2. **Self Tests** (`tests/Self/`) - Test the mock infrastructure itself | ||
| 3. **Fixture Scripts** (`tests/Fixtures/`) - External scripts for integration testing | ||
|
|
||
| ### Test Development Best Practices | ||
|
|
||
| **Environment Variable Management**: | ||
| - Use `$this->envSet('VAR', 'value')` for setting single variables | ||
| - Use `$this->envSetMultiple(['VAR1' => 'value1', 'VAR2' => 'value2'])` for multiple variables | ||
| - Use `$this->envUnset('VAR')` for unsetting single variables | ||
| - Use `$this->envUnsetMultiple(['VAR1', 'VAR2'])` for unsetting multiple variables | ||
| - Use `$this->envUnsetPrefix('PREFIX_')` for unsetting all variables with a prefix | ||
| - NEVER use `putenv()` directly - always use EnvTrait methods for automatic cleanup | ||
|
|
||
| **Data Providers - Use Them Extensively**: | ||
| - **ALWAYS use data providers** to reduce test duplication when testing the same logic with different inputs | ||
| - Use `#[DataProvider('dataProviderMethodName')]` attribute on test methods | ||
| - Data provider method names must start with `dataProvider` prefix (e.g., `dataProviderHttpMethods`, not `providerHttpMethods`) | ||
| - Data provider methods must be `public static` and return an array | ||
|
|
||
| **Advanced Data Provider Patterns**: | ||
|
|
||
| When tests need setup or additional assertions, use closures in data provider arrays: | ||
|
|
||
| ```php | ||
| public static function dataProviderWithSetupAndAssertions(): array { | ||
| return [ | ||
| 'scenario name' => [ | ||
| 'input' => 'test value', | ||
| 'expected' => 'expected value', | ||
| 'before' => function(self $test): void { | ||
| // Setup code executed before the test | ||
| $test->envSet('SOME_VAR', 'value'); | ||
| $test->mockShellExec('output'); | ||
| }, | ||
| 'after' => function(self $test, $result): void { | ||
| // Additional assertions executed after the test | ||
| $test->assertStringContainsString('expected', $result); | ||
| $test->assertFileExists('/tmp/test.txt'); | ||
| }, | ||
| ], | ||
| ]; | ||
| } | ||
|
|
||
| #[DataProvider('dataProviderWithSetupAndAssertions')] | ||
| public function testWithSetupAndAssertions( | ||
| string $input, | ||
| string $expected, | ||
| ?\Closure $before = NULL, | ||
| ?\Closure $after = NULL | ||
| ): void { | ||
| // Execute before closure if provided | ||
| if ($before !== NULL) { | ||
| $before($this); | ||
| } | ||
|
|
||
| // Main test logic | ||
| $result = someFunction($input); | ||
| $this->assertEquals($expected, $result); | ||
|
|
||
| // Execute after closure if provided | ||
| if ($after !== NULL) { | ||
| $after($this, $result); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **When to Use Before/After Closures**: | ||
| - **Before**: When different test cases need different mock setups, environment variables, or file preparations | ||
| - **After**: When different test cases need additional scenario-specific assertions beyond the main test logic | ||
| - **Avoid**: Don't use closures for simple cases - keep data providers simple when possible | ||
|
|
||
| **Real-World Example from Codebase**: | ||
|
|
||
| See `tests/Self/MockShellExecSelfTest.php` for simple data provider usage: | ||
|
|
||
| ```php | ||
| #[DataProvider('dataProviderMockShellExec')] | ||
| public function testMockShellExec(string|null|false $mock_value): void { | ||
| $this->mockShellExec($mock_value); | ||
| $result = shell_exec('echo "test"'); | ||
|
|
||
| if ($mock_value === NULL) { | ||
| $this->assertNull($result); | ||
| } | ||
| elseif ($mock_value === FALSE) { | ||
| $this->assertFalse($result); | ||
| } | ||
| else { | ||
| $this->assertEquals($mock_value, $result); | ||
| } | ||
| } | ||
|
|
||
| public static function dataProviderMockShellExec(): array { | ||
| return [ | ||
| 'string output' => ['command output'], | ||
| 'null output' => [NULL], | ||
| 'false output' => [FALSE], | ||
| 'empty string output' => [''], | ||
| ]; | ||
| } | ||
| ``` | ||
|
|
||
| This replaces 4 separate test methods with a single parameterized test. | ||
|
|
||
| **Documentation**: | ||
| - Block comments (PHPDoc /** ... */) are ONLY allowed on test classes, NOT on methods | ||
| - Do NOT add block comments to test methods, data provider methods, or helper methods | ||
| - Inline comments (// ...) are acceptable for explaining logic within method bodies | ||
| - Keep test method names descriptive enough that block comments aren't needed | ||
|
|
||
| **Allowed Comments**: | ||
| ```php | ||
| // ✅ CORRECT - Block comment on class only | ||
| /** | ||
| * Tests for webhook notification script. | ||
| */ | ||
| #[RunTestsInSeparateProcesses] | ||
| class NotifyWebhookTest extends UnitTestCase { | ||
|
|
||
| // ✅ CORRECT - No block comment on methods, only inline comments | ||
| public function testSuccessfulNotification(): void { | ||
| // Mock HTTP request | ||
| $this->mockRequest('https://example.com', [], ['status' => 200]); | ||
| // ... test code ... | ||
| } | ||
|
|
||
| // ✅ CORRECT - No block comment on data provider | ||
| public static function dataProviderHttpMethods(): array { | ||
| return ['POST' => ['POST'], 'GET' => ['GET']]; | ||
| } | ||
| } | ||
|
|
||
| // ❌ INCORRECT - Block comments on methods | ||
| /** | ||
| * Test successful notification. // <-- Remove this | ||
| */ | ||
| public function testSuccessfulNotification(): void { | ||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| **Example**: | ||
| ```php | ||
| protected function setUp(): void { | ||
| parent::setUp(); | ||
| require_once __DIR__ . '/../../src/helpers.php'; | ||
|
|
||
| // ✅ CORRECT - Use envSetMultiple for multiple variables | ||
| $this->envSetMultiple([ | ||
| 'VORTEX_NOTIFY_PROJECT' => 'test-project', | ||
| 'VORTEX_NOTIFY_LABEL' => 'main', | ||
| 'VORTEX_NOTIFY_URL' => 'https://example.com', | ||
| ]); | ||
| } | ||
|
|
||
| public function testCustomConfiguration(): void { | ||
| // ✅ CORRECT - Use envSet for single variable | ||
| $this->envSet('VORTEX_NOTIFY_CUSTOM', 'value'); | ||
|
|
||
| // ... test code ... | ||
| } | ||
|
|
||
| public function testFallbackVariables(): void { | ||
| // ✅ CORRECT - Use envUnsetMultiple for unsetting multiple variables | ||
| $this->envUnsetMultiple([ | ||
| 'VORTEX_NOTIFY_SPECIFIC', | ||
| 'VORTEX_NOTIFY_ANOTHER', | ||
| ]); | ||
|
|
||
| // ✅ CORRECT - Set fallback variable | ||
| $this->envSet('VORTEX_NOTIFY_GENERIC', 'fallback-value'); | ||
|
|
||
| // ... test code ... | ||
| } | ||
|
|
||
| public static function dataProviderHttpMethods(): array { | ||
| return [ | ||
| 'POST' => ['POST'], | ||
| 'GET' => ['GET'], | ||
| ]; | ||
| } | ||
| ``` | ||
|
|
||
| ### Mock System (MockTrait.php) | ||
|
|
||
| The package provides a comprehensive mocking system for testing scripts that use: | ||
| - `passthru()` - Shell command execution | ||
| - `quit()` / `exit()` - Script termination | ||
| - `request_*()` - HTTP requests | ||
|
|
||
| #### Mock Architecture | ||
|
|
||
| **Key Principle**: Use queue-based mock responses that are consumed sequentially. | ||
|
|
||
| Each mock system maintains: | ||
| - `$mock[Function]Responses` - Array of queued responses | ||
| - `$mock[Function]Index` - Current response index | ||
| - `$mock[Function]Checked` - Flag to prevent duplicate teardown checks | ||
|
|
||
| **Pattern**: | ||
| ```php | ||
| protected function mockPassthru(array $response): void | ||
| protected function mockPassthruMultiple(array $responses): void | ||
| protected function mockPassthruAssertAllMocksConsumed(): void | ||
|
|
||
| protected function mockQuit(int $code = 0): void | ||
|
|
||
| protected function mockCurl(array $response): void | ||
| protected function mockCurlMultiple(array $responses): void | ||
| protected function mockCurlAssertAllMocksConsumed(): void | ||
| ``` | ||
|
|
||
| #### Passthru Mocking | ||
|
|
||
| **Response Structure**: | ||
| ```php | ||
| [ | ||
| 'cmd' => 'echo "hello"', // Required: Expected command | ||
| 'output' => 'command output', // Optional: Output to echo | ||
| 'result_code' => 0, // Optional: Exit code (default: 0) | ||
| 'return' => NULL, // Optional: Return value (NULL or FALSE) | ||
| ] | ||
| ``` | ||
|
|
||
| **Example**: | ||
| ```php | ||
| $this->mockPassthru([ | ||
| 'cmd' => 'ls -la', | ||
| 'output' => 'file.txt', | ||
| 'result_code' => 0, | ||
| ]); | ||
|
|
||
| passthru('ls -la', $exit_code); // Returns mocked output | ||
| ``` | ||
|
|
||
| #### Quit Mocking | ||
|
|
||
| **Behavior**: Throws exceptions instead of exiting. | ||
|
|
||
| - Exit code 0 → `QuitSuccessException` | ||
| - Exit code != 0 → `QuitErrorException` | ||
|
|
||
| **Example**: | ||
| ```php | ||
| $this->mockQuit(1); | ||
|
|
||
| $this->expectException(QuitErrorException::class); | ||
| $this->expectExceptionCode(1); | ||
|
|
||
| \DrevOps\VortexTooling\quit(1); // Throws exception | ||
| ``` | ||
|
|
||
| #### Request Mocking | ||
|
|
||
| The mock API mirrors the actual request function signatures for consistency and better ergonomics. | ||
|
|
||
| **Mock Methods**: | ||
|
|
||
| ```php | ||
| // Mock request() - matches request() signature + $response parameter | ||
| mockRequest( | ||
| string $deploy_webhook_url, | ||
| array $options = [], | ||
| array $response = [], | ||
| string $namespace = 'DrevOps\\VortexTooling' | ||
| ): void | ||
|
|
||
| // Mock request_get() - matches request_get() signature + $response parameter | ||
| mockRequestGet( | ||
| string $deploy_webhook_url, | ||
| array $headers = [], | ||
| int $timeout = 10, | ||
| array $response = [], | ||
| string $namespace = 'DrevOps\\VortexTooling' | ||
| ): void | ||
|
|
||
| // Mock request_post() - matches request_post() signature + $response parameter | ||
| mockRequestPost( | ||
| string $deploy_webhook_url, | ||
| $body = NULL, | ||
| array $headers = [], | ||
| int $timeout = 10, | ||
| array $response = [], | ||
| string $namespace = 'DrevOps\\VortexTooling' | ||
| ): void | ||
| ``` | ||
|
|
||
| **Response Structure** (for all mock methods): | ||
| ```php | ||
| [ | ||
| 'ok' => true, // Optional: Success flag (default: TRUE) | ||
| 'status' => 200, // Required: HTTP status code | ||
| 'body' => 'response body', // Optional: Response body (default: '') | ||
| 'error' => null, // Optional: cURL error (default: NULL) | ||
| 'info' => ['http_code' => 200], // Optional: cURL info (default: []) | ||
| ] | ||
| ``` | ||
|
|
||
| **Examples**: | ||
|
|
||
| ```php | ||
| // GET request | ||
| $this->mockRequestGet( | ||
| 'https://api.example.com/data', | ||
| [], | ||
| 10, | ||
| ['status' => 200, 'body' => '{"id": 123}'] | ||
| ); | ||
|
|
||
| $result = request_get('https://api.example.com/data'); | ||
| // Returns mocked response | ||
|
|
||
| // POST request | ||
| $this->mockRequestPost( | ||
| 'https://api.example.com/data', | ||
| '{"data":"value"}', | ||
| ['Content-Type: application/json'], | ||
| 10, | ||
| ['status' => 201, 'body' => '{"id": 456}'] | ||
| ); | ||
|
|
||
| $result = request_post( | ||
| 'https://api.example.com/data', | ||
| '{"data":"value"}', | ||
| ['Content-Type: application/json'] | ||
| ); | ||
| // Returns mocked response | ||
|
|
||
| // Custom method (PUT, DELETE, etc.) | ||
| $this->mockRequest( | ||
| 'https://api.example.com/resource/123', | ||
| ['method' => 'PUT'], | ||
| ['status' => 200, 'body' => 'updated'] | ||
| ); | ||
|
|
||
| $result = request('https://api.example.com/resource/123', ['method' => 'PUT']); | ||
| // Returns mocked response | ||
| ``` | ||
|
|
||
| **Implementation Details**: | ||
|
|
||
| The request mock intercepts low-level curl functions used by the HTTP request functions: | ||
| - `curl_init()` - Stores URL, returns mock handle | ||
| - `curl_setopt_array()` - Extracts HTTP method from options | ||
| - `curl_exec()` - Validates URL/method, returns mocked body, increments index | ||
| - `curl_errno()` - Returns 0 or 1 based on error presence | ||
| - `curl_error()` - Returns mocked error message | ||
| - `curl_getinfo()` - Returns mocked info array with http_code | ||
|
|
||
| Note: `curl_close()` is no longer used (deprecated in PHP 8.0+). CurlHandle objects are automatically freed when they go out of scope. | ||
|
|
||
| ### Resource Management | ||
|
|
||
| **Critical**: `request()` uses try-finally to ensure cleanup: | ||
|
|
||
| ```php | ||
| function request(string $url, array $options = []): array { | ||
| $ch = curl_init($url); | ||
|
|
||
| try { | ||
| // All curl operations here | ||
| return [...]; | ||
| } | ||
| finally { | ||
| // CurlHandle objects are automatically freed when they go out of scope | ||
| // (PHP 8.0+), so explicit curl_close() is no longer needed. | ||
| // The unset here ensures the handle goes out of scope immediately. | ||
| unset($ch); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| This ensures that: | ||
| - The CurlHandle is freed immediately in the finally block | ||
| - Mock index is incremented in `curl_exec()` before validation | ||
| - Exception tests properly mark mocks as consumed | ||
|
|
||
| ## Test Naming Conventions | ||
|
|
||
| ### Critical Pattern Rules | ||
|
|
||
| **Direct Tests** (testing functions directly): | ||
| ```php | ||
| testMock[Function][Description][Outcome] | ||
| ``` | ||
|
|
||
| **Script Tests** (testing through fixture scripts): | ||
|
|
||
| For **single-function** mocks (passthru, quit): | ||
| ```php | ||
| testMock[Function]Script[Description][Outcome] | ||
| ``` | ||
|
|
||
| For **multi-function** mocks (request with get/post/generic): | ||
| ```php | ||
| testMock[Function][Method]Script[Description][Outcome] | ||
| ``` | ||
|
|
||
| ### Examples | ||
|
|
||
| **Passthru Tests**: | ||
| - ✅ `testMockPassthruSuccess` (direct) | ||
| - ✅ `testMockPassthruScriptPassingSuccess` (script) | ||
| - ✅ `testMockPassthruScriptFailingSuccess` (script) | ||
|
|
||
| **Quit Tests**: | ||
| - ✅ `testMockQuit0Success` (direct, exit code in name) | ||
| - ✅ `testMockQuitScript0Success` (script, exit code in name) | ||
|
|
||
| **Request Tests**: | ||
| - ✅ `testMockRequestGetSuccess` (direct) | ||
| - ✅ `testMockRequestGetScriptPassingSuccess` (script - method before "Script") | ||
| - ✅ `testMockRequestPostScriptPassingSuccess` (script - method before "Script") | ||
| - ✅ `testMockRequestMultipleScriptSuccess` (script - testing multiple calls) | ||
|
|
||
| **Why method comes before "Script" for request**: | ||
| - Single function: `testMockPassthruScript...` (no ambiguity) | ||
| - Multiple functions: `testMockRequestGetScript...` (disambiguates which request function) | ||
|
|
||
| ### Naming Convention Components | ||
|
|
||
| - **[Function]**: Passthru, Quit, Request, RequestGet, RequestPost, RequestMultiple | ||
| - **[Method]**: Get, Post (for request tests only) | ||
| - **[Description]**: Passing, Failing, Custom, Defaults, NetworkError, Multiple, MoreCalls, LessCalls, etc. | ||
| - **[Outcome]**: Success (test passes), Failure (test expects failure) | ||
|
|
||
| **Success vs Failure**: | ||
| - `Success` - Test pathway succeeds (expected behavior occurs) | ||
| - `Failure` - Test pathway fails (testing error conditions) | ||
|
|
||
| **Passing vs Failing** (fixtures only): | ||
| - `test-request-get-passing` - Fixture that expects successful execution | ||
| - `test-request-get-failing` - Fixture that expects failure execution | ||
|
|
||
| ## Test Fixtures | ||
|
|
||
| ### Fixture Naming Convention | ||
|
|
||
| Pattern: `test-[function]-[behavior]` | ||
|
|
||
| Examples: | ||
| - `test-passthru-passing` - Passthru script expecting success | ||
| - `test-passthru-failing` - Passthru script expecting failure | ||
| - `test-quit-passing` - Quit script with exit code 0 | ||
| - `test-quit-failing` - Quit script with exit code 1 | ||
| - `test-request-get-passing` - GET request expecting success | ||
| - `test-request-get-failing` - GET request expecting failure | ||
| - `test-request-post-passing` - POST request expecting success | ||
| - `test-request-post-failing` - POST request expecting failure | ||
| - `test-request-multiple` - Multiple HTTP requests | ||
|
|
||
| ### Fixture Structure | ||
|
|
||
| All fixtures must: | ||
| 1. Start with shebang: `#!/usr/bin/env php` | ||
| 2. Use strict types: `declare(strict_types=1);` | ||
| 3. Use namespace: `namespace DrevOps\VortexTooling;` | ||
| 4. Load helpers: `require_once __DIR__ . '/../../src/helpers.php';` | ||
| 5. Print output for test assertions | ||
| 6. Be executable: `chmod +x tests/Fixtures/test-*` | ||
|
|
||
| **Example**: | ||
| ```php | ||
| #!/usr/bin/env php | ||
| <?php | ||
|
|
||
| /** | ||
| * @file | ||
| * GET request fixture for request_get testing. | ||
| */ | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace DrevOps\VortexTooling; | ||
|
|
||
| require_once __DIR__ . '/../../src/helpers.php'; | ||
|
|
||
| echo 'Script will call request_get' . PHP_EOL; | ||
|
|
||
| $result = request_get('https://example.com/api'); | ||
|
|
||
| echo 'Response status: ' . $result['status'] . PHP_EOL; | ||
| echo 'Response ok: ' . ($result['ok'] ? 'true' : 'false') . PHP_EOL; | ||
|
|
||
| if ($result['ok']) { | ||
| echo 'Response body: ' . $result['body'] . PHP_EOL; | ||
| } | ||
| ``` | ||
|
|
||
| ## Development Workflow | ||
|
|
||
| ### Adding New Helper Functions | ||
|
|
||
| 1. **Add to src/helpers.php**: | ||
| ```php | ||
| /** | ||
| * Function description. | ||
| * | ||
| * @param string $param | ||
| * Parameter description. | ||
| * | ||
| * @return mixed | ||
| * Return description. | ||
| */ | ||
| function my_new_function(string $param) { | ||
| // Implementation | ||
| } | ||
| ``` | ||
|
|
||
| 2. **Create unit test in tests/Unit/**: | ||
| ```php | ||
| class MyNewFunctionTest extends UnitTestCase { | ||
| public function testMyNewFunctionSuccess(): void { | ||
| $result = \DrevOps\VortexTooling\my_new_function('input'); | ||
| $this->assertEquals('expected', $result); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| 3. **Run tests**: | ||
| ```bash | ||
| ./vendor/bin/phpunit tests/Unit/MyNewFunctionTest.php | ||
| ``` | ||
|
|
||
| ### Adding New Mock Functionality | ||
|
|
||
| 1. **Add to MockTrait.php**: | ||
| ```php | ||
| protected function mockMyFunction(array $response): void { | ||
| $this->mockMyFunctionMultiple([$response]); | ||
| } | ||
|
|
||
| protected function mockMyFunctionMultiple(array $responses): void { | ||
| // Store responses | ||
| $this->mockMyFunctionResponses = array_merge( | ||
| $this->mockMyFunctionResponses, | ||
| $responses | ||
| ); | ||
|
|
||
| // Create mock if not exists | ||
| if ($this->mockMyFunction === NULL) { | ||
| $this->mockMyFunction = $this->getFunctionMock('DrevOps\\VortexTooling', 'my_function'); | ||
| $this->mockMyFunction | ||
| ->expects($this->any()) | ||
| ->willReturnCallback(function () { | ||
| // Mock implementation | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| protected function mockMyFunctionAssertAllMocksConsumed(): void { | ||
| // Teardown validation | ||
| } | ||
| ``` | ||
|
|
||
| 2. **Add to mockTearDown()**: | ||
| ```php | ||
| protected function mockTearDown(): void { | ||
| // ... existing teardown ... | ||
|
|
||
| // Add new mock teardown | ||
| $this->mockMyFunctionAssertAllMocksConsumed(); | ||
| $this->mockMyFunction = NULL; | ||
| $this->mockMyFunctionResponses = []; | ||
| $this->mockMyFunctionIndex = 0; | ||
| $this->mockMyFunctionChecked = FALSE; | ||
| } | ||
| ``` | ||
|
|
||
| 3. **Create self-tests in tests/Self/**: | ||
| - Direct tests (14+ tests) | ||
| - Script tests (12+ tests) | ||
| - Follow naming conventions | ||
|
|
||
| 4. **Create fixtures in tests/Fixtures/**: | ||
| - Passing fixture | ||
| - Failing fixture | ||
| - Multiple calls fixture (if applicable) | ||
|
|
||
| ### Running Tests | ||
|
|
||
| ```bash | ||
| # All tests | ||
| ./vendor/bin/phpunit | ||
|
|
||
| # Specific test suite | ||
| ./vendor/bin/phpunit tests/Unit/ | ||
| ./vendor/bin/phpunit tests/Self/ | ||
|
|
||
| # Specific test file | ||
| ./vendor/bin/phpunit tests/Self/MockRequestSelfTest.php | ||
|
|
||
| # With testdox output | ||
| ./vendor/bin/phpunit --testdox | ||
|
|
||
| # With coverage | ||
| ./vendor/bin/phpunit --coverage-html coverage/ | ||
| ``` | ||
|
|
||
| ## Common Patterns | ||
|
|
||
| ### Multiple Mock Calls | ||
|
|
||
| ```php | ||
| // Queue multiple responses - just call mock methods multiple times | ||
| $this->mockRequestGet( | ||
| 'https://example.com/first', | ||
| [], | ||
| 10, | ||
| ['status' => 200, 'body' => 'first'] | ||
| ); | ||
|
|
||
| $this->mockRequestGet( | ||
| 'https://example.com/second', | ||
| [], | ||
| 10, | ||
| ['status' => 200, 'body' => 'second'] | ||
| ); | ||
|
|
||
| // Both calls return mocked responses in order | ||
| $result1 = request_get('https://example.com/first'); | ||
| $result2 = request_get('https://example.com/second'); | ||
| ``` | ||
|
|
||
| ### Testing Too Many Calls | ||
|
|
||
| ```php | ||
| // Mock only 1 response | ||
| $this->mockRequestGet( | ||
| 'https://example.com/api', | ||
| [], | ||
| 10, | ||
| ['status' => 200] | ||
| ); | ||
|
|
||
| // Expect exception on second call | ||
| $this->expectException(\RuntimeException::class); | ||
| $this->expectExceptionMessage('curl_init() called more times than mocked responses. Expected 1 request(s), but attempting request #2'); | ||
|
|
||
| request_get('https://example.com/api'); // OK | ||
| request_get('https://example.com/api'); // Throws RuntimeException | ||
| ``` | ||
|
|
||
| ### Testing Too Few Calls | ||
|
|
||
| ```php | ||
| // Mock 3 responses but only make 2 calls | ||
| $this->mockRequestGet('https://example.com/1', [], 10, ['status' => 200]); | ||
| $this->mockRequestGet('https://example.com/2', [], 10, ['status' => 200]); | ||
| $this->mockRequestGet('https://example.com/3', [], 10, ['status' => 200]); | ||
|
|
||
| request_get('https://example.com/1'); | ||
| request_get('https://example.com/2'); | ||
|
|
||
| // Expect assertion failure in tearDown | ||
| $this->expectException(\PHPUnit\Framework\AssertionFailedError::class); | ||
| $this->expectExceptionMessage('Not all mocked request responses were consumed. Expected 3 request(s), but only 2 request(s) were made.'); | ||
|
|
||
| // Manually trigger teardown check | ||
| $this->mockRequestAssertAllMocksConsumed(); | ||
| ``` | ||
|
|
||
| ### Testing URL Mismatch | ||
|
|
||
| ```php | ||
| // Mock expects different URL | ||
| $this->mockRequestGet( | ||
| 'https://expected.com/api', | ||
| [], | ||
| 10, | ||
| ['status' => 200] | ||
| ); | ||
|
|
||
| $this->expectException(\RuntimeException::class); | ||
| $this->expectExceptionMessage('request made to unexpected URL. Expected "https://expected.com/api", got "https://actual.com/api".'); | ||
|
|
||
| request_get('https://actual.com/api'); // URL mismatch | ||
| ``` | ||
|
|
||
| ### Script Testing with runScript() | ||
|
|
||
| ```php | ||
| // Mock the function | ||
| $this->mockRequestGet( | ||
| 'https://example.com/api', | ||
| [], | ||
| 10, | ||
| ['status' => 200, 'body' => 'success'] | ||
| ); | ||
|
|
||
| // Run fixture script | ||
| $output = $this->runScript('test-request-get-passing', 'tests/Fixtures'); | ||
|
|
||
| // Assert on script output | ||
| $this->assertStringContainsString('Response status: 200', $output); | ||
| $this->assertStringContainsString('Response ok: true', $output); | ||
| $this->assertStringContainsString('Response body: success', $output); | ||
| ``` | ||
|
|
||
| ## Debugging | ||
|
|
||
| ### Enable Debug Output | ||
|
|
||
| ```bash | ||
| # In tests | ||
| TEST_VORTEX_DEBUG=1 ./vendor/bin/phpunit | ||
|
|
||
| # In scripts | ||
| VORTEX_DEBUG=1 php script.php | ||
| ``` | ||
|
|
||
| ### Common Issues | ||
|
|
||
| **Issue**: "Function already enabled" error | ||
|
|
||
| **Cause**: Trying to create multiple mocks for the same function | ||
|
|
||
| **Solution**: The mock methods automatically handle queueing, just call them multiple times: | ||
| ```php | ||
| // ✅ Correct - each call queues a response | ||
| $this->mockRequestGet('https://example.com/api', [], 10, ['status' => 200]); // Creates mock + queues first response | ||
| $this->mockRequestGet('https://example.com/api2', [], 10, ['status' => 200]); // Queues second response (mock already exists) | ||
| ``` | ||
|
|
||
| **Issue**: "Not all mocked responses were consumed" | ||
|
|
||
| **Cause**: Mocked more responses than actual function calls | ||
|
|
||
| **Solution**: Ensure test makes all expected function calls | ||
|
|
||
| **Issue**: Test hangs waiting for input | ||
|
|
||
| **Cause**: Script requires interactive input and wasn't mocked | ||
|
|
||
| **Solution**: Mock the interactive function call | ||
|
|
||
| ## Best Practices | ||
|
|
||
| ### DO | ||
|
|
||
| - ✅ Use queue-based mocking for sequential calls | ||
| - ✅ Validate all required fields in mock responses | ||
| - ✅ Test both success and error pathways | ||
| - ✅ Use descriptive test names following conventions | ||
| - ✅ Create fixtures for integration testing | ||
| - ✅ Always validate that all mocks are consumed | ||
| - ✅ Use try-finally for resource cleanup | ||
| - ✅ Test too many calls, too few calls, and mismatches | ||
|
|
||
| ### DON'T | ||
|
|
||
| - ❌ Create new mocks for each call (use queue pattern) | ||
| - ❌ Skip teardown validation | ||
| - ❌ Mix direct and script tests in same method | ||
| - ❌ Forget to load helpers.php in setUp() | ||
| - ❌ Use inconsistent naming conventions | ||
| - ❌ Create fixtures without proper namespace | ||
| - ❌ Test without asserting on output | ||
|
|
||
| ## Reference | ||
|
|
||
| ### PHPUnit Attributes | ||
|
|
||
| ```php | ||
| #[CoversClass(UnitTestCase::class)] // Coverage declaration | ||
| ``` | ||
|
|
||
| ### UnitTestCase Methods | ||
|
|
||
| ```php | ||
| protected function setUp(): void // Setup before each test | ||
| protected function tearDown(): void // Cleanup after each test | ||
| protected function runScript(string $name, string $dir) // Run fixture script | ||
| ``` | ||
|
|
||
| ### MockTrait Methods | ||
|
|
||
| ```php | ||
| // Passthru | ||
| protected function mockPassthru(array $response): void | ||
| protected function mockPassthruMultiple(array $responses): void | ||
| protected function mockPassthruAssertAllMocksConsumed(): void | ||
|
|
||
| // Quit | ||
| protected function mockQuit(int $code = 0): void | ||
|
|
||
| // Request | ||
| protected function mockRequest(string $url, array $options = [], array $response = [], string $namespace = 'DrevOps\\VortexTooling'): void | ||
| protected function mockRequestGet(string $url, array $headers = [], int $timeout = 10, array $response = [], string $namespace = 'DrevOps\\VortexTooling'): void | ||
| protected function mockRequestPost(string $url, $body = NULL, array $headers = [], int $timeout = 10, array $response = [], string $namespace = 'DrevOps\\VortexTooling'): void | ||
| protected function mockRequestMultiple(array $responses, string $namespace = 'DrevOps\\VortexTooling'): void | ||
| protected function mockRequestAssertAllMocksConsumed(): void | ||
|
|
||
| // Teardown | ||
| protected function mockTearDown(): void | ||
| ``` | ||
|
|
||
| ## Additional Resources | ||
|
|
||
| - PHPUnit Documentation: https://phpunit.de/documentation.html | ||
| - php-mock Documentation: https://github.com/php-mock/php-mock-phpunit | ||
| - Vortex Template: https://github.com/drevops/vortex | ||
|
|
||
| --- | ||
|
|
||
| *This documentation should be updated whenever significant changes are made to the tooling package structure, mock system, or testing conventions.* |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider addressing markdown formatting issues.
Static analysis identified several markdown formatting inconsistencies:
- Missing blank lines around fenced code blocks (lines 53, 83, 338, 369, 427, 442, 452, 470, 515, 609, 616, 621, 689, 721, 737, 747, 754, 783, 945) - affects readability
- Inconsistent ordered list numbering (lines 736-747, 782-801) - use consistent "1." prefix style
- Bare URLs (lines 1026-1028) - should be formatted as proper markdown links
These don't affect functionality but would improve consistency with markdown best practices.
Example fix for bare URLs:
-- PHPUnit Documentation: https://phpunit.de/documentation.html
-- php-mock Documentation: https://github.com/php-mock/php-mock-phpunit
-- Vortex Template: https://github.com/drevops/vortex
+- [PHPUnit Documentation](https://phpunit.de/documentation.html)
+- [php-mock Documentation](https://github.com/php-mock/php-mock-phpunit)
+- [Vortex Template](https://github.com/drevops/vortex)🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
53-53: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
83-83: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
338-338: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
369-369: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
427-427: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
442-442: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
452-452: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
470-470: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
515-515: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
609-609: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
616-616: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
621-621: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
689-689: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
721-721: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
736-736: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
737-737: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
746-746: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1
(MD029, ol-prefix)
747-747: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
754-754: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
782-782: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
783-783: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
796-796: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1
(MD029, ol-prefix)
801-801: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/1/1
(MD029, ol-prefix)
945-945: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
1026-1026: Bare URL used
(MD034, no-bare-urls)
1027-1027: Bare URL used
(MD034, no-bare-urls)
1028-1028: Bare URL used
(MD034, no-bare-urls)
Comment on lines
77
to
79
| request_get(string $deploy_webhook_url, array $headers = [], int $timeout = 10): array | ||
| request_post(string $deploy_webhook_url, $body = NULL, array $headers = [], int $timeout = 10): array | ||
| request(string $deploy_webhook_url, array $options = []): array |
There was a problem hiding this comment.
Correct the parameter name in function signatures.
The documentation uses $deploy_webhook_url as the parameter name, but the actual function implementations use $url. This inconsistency appears throughout the documentation (lines 77-79, 488-491, 495-500, 504-510, and many examples).
🔎 Proposed fix for lines 77-79
-request_get(string $deploy_webhook_url, array $headers = [], int $timeout = 10): array
-request_post(string $deploy_webhook_url, $body = NULL, array $headers = [], int $timeout = 10): array
-request(string $deploy_webhook_url, array $options = []): array
+request_get(string $url, array $headers = [], int $timeout = 10): array
+request_post(string $url, $body = NULL, array $headers = [], int $timeout = 10): array
+request(string $url, array $options = []): arraySimilar corrections are needed throughout the file in:
- Mock function signatures (lines 488-491, 495-500, 504-510)
- Code examples using these parameters
📝 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
| request_get(string $deploy_webhook_url, array $headers = [], int $timeout = 10): array | |
| request_post(string $deploy_webhook_url, $body = NULL, array $headers = [], int $timeout = 10): array | |
| request(string $deploy_webhook_url, array $options = []): array | |
| request_get(string $url, array $headers = [], int $timeout = 10): array | |
| request_post(string $url, $body = NULL, array $headers = [], int $timeout = 10): array | |
| request(string $url, array $options = []): array |
🤖 Prompt for AI Agents
In @.vortex/tooling/CLAUDE.md around lines 77 - 79, The doc signatures for
request_get, request_post, and request use `$deploy_webhook_url` but the actual
implementations expect `$url`; update all occurrences in the documentation
(including the signatures at lines shown and the mock function signatures and
examples elsewhere) to use `$url` so the parameter name matches the
implementation, ensuring consistency in request_get, request_post, and request
and any related examples or mocks that call them.
Comment on lines
17
to
24
| <file>src</file> | ||
| <file>src/notify</file> | ||
| <file>src/notify-email</file> | ||
| <file>src/notify-github</file> | ||
| <file>src/notify-jira</file> | ||
| <file>src/notify-newrelic</file> | ||
| <file>src/notify-slack</file> | ||
| <file>src/notify-webhook</file> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove redundant file entries.
Lines 18-24 explicitly list subdirectories under src/, but line 17 already includes <file>src</file>, which recursively scans all files within that directory. The explicit subdirectory listings are redundant and may cause files to be scanned multiple times.
🔎 Proposed fix
<file>check-no-exit.php</file>
<file>src</file>
-<file>src/notify</file>
-<file>src/notify-email</file>
-<file>src/notify-github</file>
-<file>src/notify-jira</file>
-<file>src/notify-newrelic</file>
-<file>src/notify-slack</file>
-<file>src/notify-webhook</file>
<file>playground</file>
<file>tests</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
| <file>src</file> | |
| <file>src/notify</file> | |
| <file>src/notify-email</file> | |
| <file>src/notify-github</file> | |
| <file>src/notify-jira</file> | |
| <file>src/notify-newrelic</file> | |
| <file>src/notify-slack</file> | |
| <file>src/notify-webhook</file> | |
| <file>src</file> |
🤖 Prompt for AI Agents
In @.vortex/tooling/phpcs.xml around lines 17 - 24, Remove the redundant child
<file> entries under the same directory: since the XML contains a
<file>src</file> entry that already recursively includes subdirectories, delete
the explicit <file>src/notify</file>, <file>src/notify-email</file>,
<file>src/notify-github</file>, <file>src/notify-jira</file>,
<file>src/notify-newrelic</file>, <file>src/notify-slack</file>, and
<file>src/notify-webhook</file> lines from the phpcs.xml snippet so only the
top-level <file>src</file> remains, avoiding duplicate scanning.
Comment on lines
35
to
45
| if ($expected_error !== NULL) { | ||
| $this->mockQuit(1); | ||
| $this->expectException(QuitErrorException::class); | ||
| $this->expectOutputRegex('/' . preg_quote('[FAIL]', '/') . '/'); | ||
| } | ||
|
|
||
| $result = \DrevOps\VortexTooling\string_map_to_array($map, $separator, $key_value_separator); | ||
|
|
||
| if ($expected !== NULL) { | ||
| $this->assertEquals($expected, $result); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating the specific error message, not just [FAIL] marker.
The test asserts [FAIL] appears in output but doesn't verify the actual error message matches $expected_error. This could miss regressions in error messaging.
🔎 Proposed enhancement
if ($expected_error !== NULL) {
$this->mockQuit(1);
$this->expectException(QuitErrorException::class);
- $this->expectOutputRegex('/' . preg_quote('[FAIL]', '/') . '/');
+ $this->expectOutputRegex('/' . preg_quote($expected_error, '/') . '/');
}🤖 Prompt for AI Agents
In @.vortex/tooling/tests/Unit/HelpersStringMapTest.php around lines 35 - 45,
The test only checks for the '[FAIL]' marker when $expected_error is set but
doesn't assert the actual error message; modify the error branch to validate the
specific message by adding an expectation for the exception message and/or
output: when $expected_error !== NULL keep
$this->expectException(QuitErrorException::class) and add
$this->expectExceptionMessage($expected_error) (or $this->expectOutputRegex('/'
. preg_quote($expected_error, '/') . '/')) so the failure text from
string_map_to_array is verified exactly.
Comment on lines
32
to
55
| public static function dataProviderReplaceTokens(): array { | ||
| return [ | ||
| 'single token' => [ | ||
| 'template' => 'Hello %name%', | ||
| 'replacements' => ['name' => 'World'], | ||
| 'expected' => 'Hello World', | ||
| ], | ||
| 'multiple tokens' => [ | ||
| 'template' => '%greeting% %name%, you have %count% messages', | ||
| 'replacements' => ['greeting' => 'Hello', 'name' => 'Alice', 'count' => '5'], | ||
| 'expected' => 'Hello Alice, you have 5 messages', | ||
| ], | ||
| 'no tokens' => [ | ||
| 'template' => 'No tokens here', | ||
| 'replacements' => ['token' => 'value'], | ||
| 'expected' => 'No tokens here', | ||
| ], | ||
| 'repeated tokens' => [ | ||
| 'template' => '%token% and %token% again', | ||
| 'replacements' => ['token' => 'TEST'], | ||
| 'expected' => 'TEST and TEST again', | ||
| ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding edge case tests for special characters and encoding.
The data provider covers core scenarios well. Given that replace_tokens uses json_encode internally (per helpers.php), consider adding tests for:
- Values containing quotes, newlines, or backslashes (which get JSON-escaped)
- Empty replacements array
[] - Empty template string
'' - Unicode characters in tokens/values
🔎 Suggested additional test cases
'repeated tokens' => [
'template' => '%token% and %token% again',
'replacements' => ['token' => 'TEST'],
'expected' => 'TEST and TEST again',
],
+ 'empty template' => [
+ 'template' => '',
+ 'replacements' => ['key' => 'value'],
+ 'expected' => '',
+ ],
+ 'empty replacements' => [
+ 'template' => 'Hello %name%',
+ 'replacements' => [],
+ 'expected' => 'Hello %name%',
+ ],
+ 'value with quotes' => [
+ 'template' => 'Message: %msg%',
+ 'replacements' => ['msg' => 'He said "hello"'],
+ 'expected' => 'Message: He said \"hello\"',
+ ],
];🤖 Prompt for AI Agents
In @.vortex/tooling/tests/Unit/HelpersTokenTest.php around lines 32 - 55, Add
edge-case entries to dataProviderReplaceTokens to cover JSON-escaping and
encoding behaviors used by replace_tokens: include cases where replacement
values contain quotes, newlines, backslashes, and mixed special chars; add a
case with an empty replacements array []; add a case with an empty template
string ''; and add cases with Unicode characters in both token names and values
so replace_tokens behavior with UTF-8 is validated. Reference the data provider
method dataProviderReplaceTokens and the replace_tokens helper when adding these
new test rows.
Comment on lines
51
to
55
| $ssh_dir = self::$tmp . '/.ssh'; | ||
| mkdir($ssh_dir, 0700, TRUE); | ||
| $key_file = $ssh_dir . '/id_rsa'; | ||
| file_put_contents($key_file, "fake ssh key\n"); | ||
| chmod($key_file, 0600); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use File class for cross-platform compatibility.
According to the coding guidelines in CLAUDE.md, tests should use AlexSkrypnyk\File\File class instead of native PHP functions for file operations to ensure cross-platform compatibility.
🔎 Proposed fix
Add the import at the top of the file:
use AlexSkrypnyk\File\File;Then replace native functions throughout the test file:
- $ssh_dir = self::$tmp . '/.ssh';
- mkdir($ssh_dir, 0700, TRUE);
- $key_file = $ssh_dir . '/id_rsa';
- file_put_contents($key_file, "fake ssh key\n");
- chmod($key_file, 0600);
+ $ssh_dir = self::$tmp . '/.ssh';
+ File::mkdir($ssh_dir);
+ $key_file = $ssh_dir . '/id_rsa';
+ File::dump($key_file, "fake ssh key\n");
+ chmod($key_file, 0600);Apply similar changes to all other test methods using mkdir() and file_put_contents().
Based on coding guidelines from CLAUDE.md.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @.vortex/tooling/tests/Unit/SetupSshTest.php around lines 51 - 55, The test
uses native PHP file functions; update it to use AlexSkrypnyk\File\File for
cross-platform compatibility: add the import for AlexSkrypnyk\File\File and
replace mkdir($ssh_dir,...), file_put_contents($key_file,...), and
chmod($key_file,...) with the File class equivalents when setting up $ssh_dir
and $key_file in SetupSshTest (and mirror the same changes across other test
methods in this file); ensure you call the File API to create the directory with
the desired mode, write the key contents to id_rsa, and set the file permissions
using File methods rather than native functions.
Comment on lines
202
to
208
| // Verify config file was created. | ||
| $config_file = $ssh_dir . '/config'; | ||
| $this->assertFileExists($config_file); | ||
| $config_content = (string) file_get_contents($config_file); | ||
| $this->assertStringContainsString('StrictHostKeyChecking no', $config_content); | ||
| $this->assertStringContainsString('UserKnownHostsFile /dev/null', $config_content); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using File class for reading config file content.
For consistency with cross-platform guidelines, consider using File utilities for reading file content as well, though file_get_contents is generally safe for reading.
🤖 Prompt for AI Agents
In @.vortex/tooling/tests/Unit/SetupSshTest.php around lines 202 - 208, The test
reads the SSH config using file_get_contents; replace that call with the
project's File utility for cross-platform consistency by using the File class to
read $config_file (e.g., use File::get($config_file) instead of
file_get_contents) in the test that sets $config_file and assigns
$config_content, ensuring any necessary import/alias for the File class is
present in SetupSshTest.
Comment on lines
159
to
163
| # Explicitly set DB overwrite flag to 0 due to a bug in Lagoon. | ||
| # @see https://github.com/uselagoon/lagoon/issues/1922 | ||
| # @todo Review and remove this workaround as the issue is fixed. | ||
| task "Setting a DB overwrite flag to 0." | ||
| lagoon update variable --environment "${deploy_pr_full}" --name VORTEX_PROVISION_OVERRIDE_DB --value 0 --scope global || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/vortex/deploy-lagoon.sh | sed -n '155,165p'Repository: drevops/vortex
Length of output: 659
🌐 Web query:
uselagoon lagoon issue 1922 VORTEX_PROVISION_OVERRIDE_DB
💡 Result:
I couldn't find an open uselagoon/lagoon GitHub issue #1922 mentioning VORTEX_PROVISION_OVERRIDE_DB. I did find the Vortex provisioning docs that define VORTEX_PROVISION_OVERRIDE_DB (drops existing DB before import) and the uselagoon/lagoon repository/docs where Lagoon environment handling is described:
- Vortex provisioning docs (defines VORTEX_PROVISION_OVERRIDE_DB and related vars). [1]
- uselagoon/lagoon GitHub repository (project home). [2]
- Lagoon docs on environment variables. [3]
If you want, I can:
- search specifically inside the uselagoon/lagoon issues/prs for “1922” (or other repos) and return any matches, or
- open a specific repo/issue URL you have in mind.
Sources:
[1] Vortex provision docs — VORTEX_PROVISION_OVERRIDE_DB.
[2] uselagoon/lagoon GitHub repository.
[3] Lagoon docs — Environment Variables.
🏁 Script executed:
# Try to access the GitHub issue directly and check if it exists or is closed
curl -s "https://api.github.com/repos/uselagoon/lagoon/issues/1922" | jq '.state, .title, .closed_at, .status' 2>/dev/null || echo "Issue not found or API error"Repository: drevops/vortex
Length of output: 77
🌐 Web query:
site:github.com/uselagoon/lagoon issue 1922
💡 Result:
I couldn't find an Issue numbered 1922 in the uselagoon/lagoon GitHub repository. I searched the repository and its issue list but no matching issue page appeared. Source: uselagoon/lagoon repository homepage on GitHub. [1]
Would you like me to (a) search all uselagoon repos for "issue 1922", (b) try broader web search terms (e.g., issue title or keywords), or (c) open a specific URL you have?
🏁 Script executed:
# Check git history for this TODO addition
git log --oneline -p scripts/vortex/deploy-lagoon.sh | grep -A 5 -B 5 "1922\|VORTEX_PROVISION_OVERRIDE_DB" | head -40Repository: drevops/vortex
Length of output: 2245
🏁 Script executed:
# Search for any other references to issue 1922 or this workaround
rg "1922|VORTEX_PROVISION_OVERRIDE_DB" --type sh -A 2 -B 2Repository: drevops/vortex
Length of output: 6230
Remove or correct the invalid Lagoon issue reference in the TODO comment.
The referenced Lagoon issue (uselagoon/lagoon#1922) does not exist. The TODO comment cannot be meaningfully tracked or acted upon without a valid issue reference. Either provide the correct issue number, replace it with a description of the conditions required to remove this workaround, or document why the workaround is necessary if the issue is no longer trackable.
🤖 Prompt for AI Agents
In @scripts/vortex/deploy-lagoon.sh around lines 159 - 163, The TODO comment
referencing uselagoon/lagoon#1922 is invalid; update the comment above the task
"Setting a DB overwrite flag to 0." and the lagoon update variable command to
either: 1) replace the nonexistent issue reference with the correct Lagoon issue
number if you can find it, or 2) remove the issue link and add a short
description of the exact condition that will allow removal of this workaround
(e.g., Lagoon PR/merge, specific bug behavior fixed, or a test that proves
overwrite works), or 3) if the workaround must remain permanently, document why
it is required and when it can be revisited; ensure changes are in the same
comment block that mentions VORTEX_PROVISION_OVERRIDE_DB and the lagoon update
variable command.
| return $subcommand_output ?: ''; | ||
| } | ||
|
|
||
| function lagoon_get_existing_environments(bool $is_branch):array{ |
There was a problem hiding this comment.
Fix syntax: missing space after function signature.
Missing space after the closing parenthesis in the function signature.
🔎 Proposed fix
-function lagoon_get_existing_environments(bool $is_branch):array{
+function lagoon_get_existing_environments(bool $is_branch): array {📝 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
| function lagoon_get_existing_environments(bool $is_branch):array{ | |
| function lagoon_get_existing_environments(bool $is_branch): array { |
🤖 Prompt for AI Agents
In @.vortex/tooling/src/deploy-lagoon at line 198, The function signature for
lagoon_get_existing_environments is missing spacing; change "function
lagoon_get_existing_environments(bool $is_branch):array{" to use proper spacing
around the return type and brace, e.g. "function
lagoon_get_existing_environments(bool $is_branch): array {" (ensure a space
after the closing parenthesis and a space between the colon and return type and
before the opening brace).
| function lagoon_handle_environment_limit_exceeded(string $output): int { | ||
| global $deploy_lagoon_fail_when_env_limit_exceeded; | ||
|
|
||
| if (str_contains($output, 'exceed')) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improve error detection specificity.
The string contains check for 'exceed' is overly broad and could match unintended strings like "exceeded expectations" in unrelated output. Consider using a more specific pattern such as 'environment limit exceeded' or checking for specific Lagoon API error codes.
🔎 Proposed fix
- if (str_contains($output, 'exceed')) {
+ if (str_contains($output, 'environment limit') && str_contains($output, 'exceed')) {📝 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
| if (str_contains($output, 'exceed')) { | |
| if (str_contains($output, 'environment limit') && str_contains($output, 'exceed')) { |
🤖 Prompt for AI Agents
In @.vortex/tooling/src/deploy-lagoon at line 234, The current check using
str_contains($output, 'exceed') is too generic; change it to detect a specific
Lagoon error message or code instead—e.g., replace the substring check on
'exceed' with a check for the exact phrase like 'environment limit exceeded'
(case-insensitive) or use a regex (preg_match) to match the precise Lagoon error
text, or better yet parse and check the Lagoon API error code if available;
update the conditional that references str_contains($output, 'exceed') to use
the more specific match so unrelated output (e.g., "exceeded expectations")
won't trigger this branch.
Comment on lines
261
to
265
| # Explicitly set DB overwrite flag to 0 due to a bug in Lagoon. | ||
| # @see https://github.com/uselagoon/lagoon/issues/1922 | ||
| # @todo Review and remove this workaround as the issue is fixed. | ||
| task('Setting a DB overwrite flag to 0.'); | ||
| lagoon_run_cli(sprintf('update variable --environment %s --name VORTEX_PROVISION_OVERRIDE_DB --value 0 --scope global', escapeshellarg($deploy_pr_full))); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Eliminate code duplication for DB override flag handling.
The logic for setting the DB override flag to 0 (lines 261-265 and 316-320) is duplicated between PR and branch deployments. Consider extracting this into a helper function to improve maintainability.
🔎 Proposed refactor
Add a helper function:
function lagoon_reset_db_override_flag(string $environment): void {
# Explicitly set DB overwrite flag to 0 due to a bug in Lagoon.
# @see https://github.com/uselagoon/lagoon/issues/1922
# @todo Review and remove this workaround as the issue is fixed.
task('Setting a DB overwrite flag to 0.');
lagoon_run_cli(sprintf('update variable --environment %s --name VORTEX_PROVISION_OVERRIDE_DB --value 0 --scope global', escapeshellarg($environment)));
}Then replace both occurrences with:
lagoon_reset_db_override_flag($deploy_pr_full); // or $deploy_branchAlso applies to: 316-320
🤖 Prompt for AI Agents
In @.vortex/tooling/src/deploy-lagoon around lines 261 - 265, Duplicate logic
sets the VORTEX_PROVISION_OVERRIDE_DB variable to 0 in two places; extract that
logic into a single helper function (e.g., lagoon_reset_db_override_flag(string
$environment): void) that calls task('Setting a DB overwrite flag to 0.') and
invokes lagoon_run_cli(...) with the escaped environment, then replace both
inline blocks that use lagoon_run_cli with calls to
lagoon_reset_db_override_flag($deploy_pr_full) and
lagoon_reset_db_override_flag($deploy_branch) respectively.
|
|
||
| if ($deploy_action === 'deploy_override_db') { | ||
| task('Waiting for deployment to be queued.'); | ||
| sleep(10); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make deployment queue wait time configurable.
The hardcoded sleep(10) calls (lines 287 and 334) wait for deployments to be queued before clearing the DB override flag. Consider making this wait time configurable via an environment variable (e.g., VORTEX_DEPLOY_LAGOON_QUEUE_WAIT) to accommodate different Lagoon instance behaviors or testing scenarios.
🔎 Proposed enhancement
At the top of the file with other environment variables:
// Time to wait (in seconds) for deployment to be queued before clearing flags.
$deploy_lagoon_queue_wait = (int) getenv_default('VORTEX_DEPLOY_LAGOON_QUEUE_WAIT', '10');Then replace the hardcoded values:
- sleep(10);
+ sleep($deploy_lagoon_queue_wait);Also applies to: 334-334
🤖 Prompt for AI Agents
In @.vortex/tooling/src/deploy-lagoon at line 287, Replace the hardcoded
sleep(10) calls with a configurable wait variable: add a new integer variable
(e.g., $deploy_lagoon_queue_wait) initialized from
getenv_default('VORTEX_DEPLOY_LAGOON_QUEUE_WAIT', '10') near the other env var
parsing, then change both occurrences of sleep(10) to
sleep($deploy_lagoon_queue_wait) so the deployment queue wait time can be tuned
via the environment.
Comment on lines
31
to
33
| // @codeCoverageIgnoreStart | ||
| load_dotenv(['.env', '.env.local']); | ||
| // @codeCoverageIgnoreEnd |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Auto-loading .env files on require has side effects.
Loading .env files automatically when helpers.php is included can cause unexpected behavior:
- Multiple includes could re-process env files
- Tests may unintentionally load production
.envfiles - Order of operations matters when env vars are needed before helpers are loaded
Consider making this explicit or adding a guard.
🔎 Proposed change
// @codeCoverageIgnoreStart
-load_dotenv(['.env', '.env.local']);
+if (getenv('VORTEX_TOOLING_SKIP_DOTENV') !== '1') {
+ load_dotenv(['.env', '.env.local']);
+}
// @codeCoverageIgnoreEnd📝 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
| // @codeCoverageIgnoreStart | |
| load_dotenv(['.env', '.env.local']); | |
| // @codeCoverageIgnoreEnd | |
| // @codeCoverageIgnoreStart | |
| if (getenv('VORTEX_TOOLING_SKIP_DOTENV') !== '1') { | |
| load_dotenv(['.env', '.env.local']); | |
| } | |
| // @codeCoverageIgnoreEnd |
🤖 Prompt for AI Agents
In @.vortex/tooling/src/helpers.php around lines 31 - 33, helpers.php currently
calls load_dotenv(...) at require time which causes side effects; change this to
guard against repeated or implicit loading by either 1) wrapping the call in a
new explicit function (e.g., ensure_dotenv_loaded() or initEnv()) that calls
load_dotenv(['.env', '.env.local']) only when invoked, or 2) add an idempotent
guard around the top-level call that checks a sentinel (e.g.,
defined('DOTENV_LOADED') or getenv('DOTENV_LOADED')) before calling load_dotenv
and sets that sentinel after loading; update any code that relied on
auto-loading to call the new function explicitly.
Comment on lines
69
to
81
| public function testEmptyRegistryName(): void { | ||
| $this->envSet('VORTEX_CONTAINER_REGISTRY', ' '); | ||
|
|
||
| $this->mockPassthru([ | ||
| 'cmd' => "echo 'testpass' | docker login --username 'testuser' --password-stdin ' '", | ||
| 'output' => 'Login Succeeded', | ||
| 'result_code' => 0, | ||
| ]); | ||
|
|
||
| $output = $this->runScript('src/login-container-registry'); | ||
|
|
||
| $this->assertStringContainsString('Logging in to registry " ".', $output); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating empty registry names.
The test allows a registry name with only whitespace (' '), which would likely cause docker login to fail with a confusing error. Consider adding validation in the login-container-registry script to reject empty or whitespace-only registry names and provide a clear error message.
🤖 Prompt for AI Agents
In @.vortex/tooling/tests/Unit/LoginContainerRegistryTest.php around lines 69 -
81, The login script (src/login-container-registry) should validate the
VORTEX_CONTAINER_REGISTRY value before calling docker: detect if
getenv('VORTEX_CONTAINER_REGISTRY') is null or consists only of whitespace, and
if so print a clear error like "Error: container registry name is empty" to
stderr and exit non‑zero instead of attempting docker login; update the script's
registry variable handling to trim the value, perform the check, and
short‑circuit with the error and exit code so tests and users get a helpful
message rather than a docker failure.
Comment on lines
34
to
51
| public function testSuccessfulNotificationSingleRecipient(): void { | ||
| $this->mockMail([ | ||
| 'to' => '[email protected]', | ||
| 'subject' => 'test-project deployment notification of main', | ||
| 'message' => "## This is an automated message ##\nSite test-project main has been deployed at " . date('d/m/Y H:i:s T') . " and is available at https://example.com.\nLogin at: https://example.com/login", | ||
| 'result' => TRUE, | ||
| ]); | ||
|
|
||
| $output = $this->runScript('src/notify-email'); | ||
|
|
||
| $this->assertStringContainsString('Started email notification', $output); | ||
| $this->assertStringContainsString('Project : test-project', $output); | ||
| $this->assertStringContainsString('From : [email protected]', $output); | ||
| $this->assertStringContainsString('Recipients : [email protected]', $output); | ||
| $this->assertStringContainsString('Sending email notification', $output); | ||
| $this->assertStringContainsString('Email notification sent successfully to 1 recipient(s)', $output); | ||
| $this->assertStringContainsString('Finished email notification', $output); | ||
| } |
There was a problem hiding this comment.
Potential flaky test due to timestamp matching.
The expected message on line 38 includes date('d/m/Y H:i:s T') which is evaluated at mock setup time. If there's any delay before runScript() executes (even crossing a second boundary), the timestamp in the actual email message won't match the expected value.
This pattern repeats throughout the file. Consider using a more flexible assertion approach.
🔎 Suggested approaches
Option 1: Use a regex or partial match in the mock callback:
$this->mockMail([
'to' => '[email protected]',
'subject' => 'test-project deployment notification of main',
'message' => $this->callback(function ($msg) {
return str_contains($msg, '## This is an automated message ##')
&& str_contains($msg, 'https://example.com');
}),
'result' => TRUE,
]);Option 2: Freeze time using a time mocking library if precise timestamp matching is critical.
Option 3: If the mock infrastructure doesn't support callbacks for individual fields, document that tests may be timing-sensitive and ensure the test environment is fast enough that second-boundary crossings are rare.
🤖 Prompt for AI Agents
In @.vortex/tooling/tests/Unit/NotifyEmailTest.php around lines 34 - 51, The
test testSuccessfulNotificationSingleRecipient is flaky because mockMail sets
the full 'message' with date('d/m/Y H:i:s T') at setup time which can differ
when runScript() generates the actual email; change the mockMail call to avoid
exact timestamp matching by replacing the 'message' value with a flexible check
(e.g., use the mockMail 'message' => $this->callback(function($msg) { return
str_contains($msg, '## This is an automated message ##') && str_contains($msg,
'https://example.com'); }) or a regex match) or alternatively freeze time before
building the expected message; keep the rest of the assertions the same and
update other tests in this file that use date() similarly.
Comment on lines
200
to
205
| public function testPreDeploymentEventSkipped(): void { | ||
| $this->envSet('VORTEX_NOTIFY_JIRA_EVENT', 'pre_deployment'); | ||
|
|
||
| $this->runScriptEarlyPass('src/notify-jira', 'Skipping JIRA notification for pre_deployment event'); | ||
|
|
||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Empty line after method body.
There's an extra blank line before the closing brace of testPreDeploymentEventSkipped. This is inconsistent with other test methods.
🔎 Proposed fix
public function testPreDeploymentEventSkipped(): void {
$this->envSet('VORTEX_NOTIFY_JIRA_EVENT', 'pre_deployment');
$this->runScriptEarlyPass('src/notify-jira', 'Skipping JIRA notification for pre_deployment event');
-
}📝 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
| public function testPreDeploymentEventSkipped(): void { | |
| $this->envSet('VORTEX_NOTIFY_JIRA_EVENT', 'pre_deployment'); | |
| $this->runScriptEarlyPass('src/notify-jira', 'Skipping JIRA notification for pre_deployment event'); | |
| } | |
| public function testPreDeploymentEventSkipped(): void { | |
| $this->envSet('VORTEX_NOTIFY_JIRA_EVENT', 'pre_deployment'); | |
| $this->runScriptEarlyPass('src/notify-jira', 'Skipping JIRA notification for pre_deployment event'); | |
| } |
🤖 Prompt for AI Agents
In @.vortex/tooling/tests/Unit/NotifyJiraTest.php around lines 200 - 205, The
test method testPreDeploymentEventSkipped contains an extra blank line before
its closing brace; remove the stray empty line so the closing brace follows
immediately after the last statement to match the formatting of other test
methods (locate the method named testPreDeploymentEventSkipped and delete the
blank line before its final `}`).
Comment on lines
212
to
223
| public function testHttpRequestFailureWrongStatus(): void { | ||
| $this->mockRequestPost( | ||
| 'https://hooks.slack.com/services/T00/B00/XXXX', | ||
| $this->callback(fn(): true => TRUE), | ||
| ['Content-Type: application/json'], | ||
| 10, | ||
| ['status' => 500, 'body' => 'Internal Server Error'] | ||
| ); | ||
|
|
||
| $this->runScriptError('src/notify-slack', 'Unable to send notification to Slack'); | ||
|
|
||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove extra blank line.
Minor style issue - there's an extra blank line at line 222-223.
🔎 Suggested fix
$this->runScriptError('src/notify-slack', 'Unable to send notification to Slack');
-
}📝 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
| public function testHttpRequestFailureWrongStatus(): void { | |
| $this->mockRequestPost( | |
| 'https://hooks.slack.com/services/T00/B00/XXXX', | |
| $this->callback(fn(): true => TRUE), | |
| ['Content-Type: application/json'], | |
| 10, | |
| ['status' => 500, 'body' => 'Internal Server Error'] | |
| ); | |
| $this->runScriptError('src/notify-slack', 'Unable to send notification to Slack'); | |
| } | |
| public function testHttpRequestFailureWrongStatus(): void { | |
| $this->mockRequestPost( | |
| 'https://hooks.slack.com/services/T00/B00/XXXX', | |
| $this->callback(fn(): true => TRUE), | |
| ['Content-Type: application/json'], | |
| 10, | |
| ['status' => 500, 'body' => 'Internal Server Error'] | |
| ); | |
| $this->runScriptError('src/notify-slack', 'Unable to send notification to Slack'); | |
| } |
🤖 Prompt for AI Agents
In @.vortex/tooling/tests/Unit/NotifySlackTest.php around lines 212 - 223, The
test method testHttpRequestFailureWrongStatus contains an extra blank line
before the end of the method; remove the redundant empty line between the final
assertion/statement and the closing brace so the method ends immediately with
the closing brace (adjust spacing in testHttpRequestFailureWrongStatus to match
surrounding tests).
Comment on lines
+761
to
+768
| // Mock additional file import. | ||
| // Note: The file path is converted from ./ to ../ for drush. | ||
| $relative_file = str_replace('./', '../', $file); | ||
| $this->mockPassthru([ | ||
| 'cmd' => "./vendor/bin/drush -y sql:query --file='" . $relative_file . "'", | ||
| 'output' => '', | ||
| 'result_code' => 0, | ||
| ]); |
There was a problem hiding this comment.
Fragile path transformation logic.
The str_replace('./', '../', $file) transformation assumes the file path starts with ./ and needs adjustment for Drush's working directory. This could break if:
- The file path doesn't start with
./ - The path contains
./elsewhere (e.g.,/path/to/./file)
Consider a more robust approach or document the assumption.
🔎 Proposed improvement
// Mock additional file import.
- // Note: The file path is converted from ./ to ../ for drush.
- $relative_file = str_replace('./', '../', $file);
+ // Note: The file path needs adjustment for drush's working directory (web/).
+ // Only transform leading ./ to ../ to avoid breaking paths with ./ elsewhere.
+ $relative_file = preg_replace('/^\.\//', '../', $file);
$this->mockPassthru([
'cmd' => "./vendor/bin/drush -y sql:query --file='" . $relative_file . "'",🤖 Prompt for AI Agents
In @.vortex/tooling/tests/Unit/ProvisionTest.php around lines 761 - 768, The
path rewrite using str_replace('./', '../', $file) in the test is fragile
because it replaces every './' occurrence and assumes $file begins with './';
update the logic that computes $relative_file so it only adjusts a leading './'
(e.g., check if $file starts with './' and remove that prefix then prepend
'../'), or otherwise compute a normalized relative path (using PHP's
realpath/dirname logic or an explicit conditional) before passing it into the
mockPassthru cmd string; ensure you update the $relative_file calculation
referenced in the mockPassthru call so it no longer mutates internal './'
segments or mis-handles paths that don't start with './'.
82e131c to
38f33ba
Compare
github-actions
bot
removed
the
CONFLICT
AlexSkrypnyk
changed the title
AlexSkrypnyk
added
the
Requires more work
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.vortex/tooling/src/provision:
- Line 436: The drush php:eval call uses the wrong-cased class name
"\Drupal\core\Site\Settings"; update the namespace to
"\Drupal\Core\Site\Settings" (uppercase C) in the drush invocation that assigns
$environment so the correct Drupal Settings class is referenced; ensure the
string passed to drush(...) reflects the corrected fully-qualified class name
used in the expression print \Drupal\Core\Site\Settings::get('environment');.
- Around line 361-432: The large nested if/else in the provisioning block
duplicates logic between the 'database' and 'profile' branches; extract two
helper functions (e.g., handleProvisionFromDatabase(...) and
handleProvisionFromProfile(...)) that accept the relevant inputs ($provision_db,
$db_image, $drupal_profile, $drupal_site_name, $drupal_site_email,
$site_has_config_files, $site_is_installed) and encapsulate the
site_is_installed branches, calls to provision_from_db / provision_from_profile,
and the shared environment flag handling ($provision_override_db,
putenv('VORTEX_PROVISION_OVERRIDE_DB=1'), $provision_sanitize_db_skip,
putenv('VORTEX_PROVISION_SANITIZE_DB_SKIP=1')); then replace the original nested
blocks with a simple call to the appropriate helper based on $provision_type to
remove duplication and improve testability while preserving messages like
note(...) and quit(1).
- Line 243: The current call drush(sprintf("sql:query \"UPDATE
\\`users_field_data\\` SET mail = '%s' WHERE uid = '1';\"", $admin_email))
interpolates $admin_email directly and risks SQL injection; remove the sprintf
interpolation and instead pass the email as a separate argument to the drush
command so Drush/shell escaping is applied (or, if Drush wiring does not support
argumentized SQL, explicitly escape the value with escapeshellarg before
inclusion). Ensure you update the call site (the drush(...) invocation) to stop
injecting $admin_email into the SQL string and provide it as a safely
escaped/parameterized argument.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.vortex/tooling/src/provision
⏰ 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). (11)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (4)
🔇 Additional comments (5)
.vortex/tooling/src/provision (5)
107-120: LGTM!The
drush()function properly captures output and handles exit codes. The command construction throughout the script consistently usesescapeshellarg()where needed, making the passthru call safe.
135-155: LGTM!The database import logic is sound with proper file existence checks, secure shell escaping via
escapeshellarg(), and appropriate error handling.
171-195: LGTM!Profile installation logic correctly handles all options with proper shell escaping and appropriate flags for existing configuration.
256-281: LGTM!Custom script execution is well-implemented with directory checks, proper glob pattern matching, and comprehensive error reporting for each script.
458-465: Validate UUID format before using it.The regex captures a UUID pattern but doesn't validate that it's a properly formatted UUID before setting it. Consider adding validation.
Likely an incorrect or invalid review comment.
|
|
||
| // User email could have been sanitized - setting it back to a pre-defined email. | ||
| if (!empty($admin_email)) { | ||
| drush(sprintf("sql:query \"UPDATE \\`users_field_data\\` SET mail = '%s' WHERE uid = '1';\"", $admin_email)); |
There was a problem hiding this comment.
Fix SQL injection vulnerability in admin email update.
The $admin_email variable is interpolated directly into the SQL query via sprintf() without SQL escaping. If the email contains single quotes or other SQL metacharacters, it could break out of the string literal and execute arbitrary SQL.
While the attack surface is limited (requires control of environment variables), this remains a security vulnerability.
🔎 Proposed fix using parameterized Drush command
// User email could have been sanitized - setting it back to a pre-defined email.
if (!empty($admin_email)) {
- drush(sprintf("sql:query \"UPDATE \\`users_field_data\\` SET mail = '%s' WHERE uid = '1';\"", $admin_email));
+ drush(sprintf("sql:query \"UPDATE \\`users_field_data\\` SET mail = %s WHERE uid = '1';\"", escapeshellarg($admin_email)));
pass('Updated user 1 email.');
}Note: Drush will handle the SQL escaping when the email is passed as a shell argument rather than interpolated into the SQL string.
🤖 Prompt for AI Agents
In @.vortex/tooling/src/provision at line 243, The current call
drush(sprintf("sql:query \"UPDATE \\`users_field_data\\` SET mail = '%s' WHERE
uid = '1';\"", $admin_email)) interpolates $admin_email directly and risks SQL
injection; remove the sprintf interpolation and instead pass the email as a
separate argument to the drush command so Drush/shell escaping is applied (or,
if Drush wiring does not support argumentized SQL, explicitly escape the value
with escapeshellarg before inclusion). Ensure you update the call site (the
drush(...) invocation) to stop injecting $admin_email into the SQL string and
provide it as a safely escaped/parameterized argument.
Comment on lines
+361
to
+432
| if ($provision_type === 'database') { | ||
| info('Provisioning site from the database dump file.'); | ||
| note('Dump file path: %s', $provision_db); | ||
|
|
||
| if ($site_is_installed) { | ||
| note('Existing site was found.'); | ||
|
|
||
| if (!empty($db_image)) { | ||
| note('Database is baked into the container image.'); | ||
| note('Site content will be preserved.'); | ||
| // Container image restarts with a fresh database. Let the downstream | ||
| // scripts know that the database is fresh. | ||
| $provision_override_db = '1'; | ||
| putenv('VORTEX_PROVISION_OVERRIDE_DB=1'); | ||
| } | ||
| elseif ($provision_override_db === '1') { | ||
| note('Existing site content will be removed and fresh content will be imported from the database dump file.'); | ||
| provision_from_db($provision_db); | ||
| } | ||
| else { | ||
| note('Site content will be preserved.'); | ||
| note('Sanitization will be skipped for an existing database.'); | ||
| $provision_sanitize_db_skip = '1'; | ||
| putenv('VORTEX_PROVISION_SANITIZE_DB_SKIP=1'); | ||
| } | ||
| } | ||
| else { | ||
| note('Existing site was not found.'); | ||
|
|
||
| if (!empty($db_image)) { | ||
| note('Database is baked into the container image.'); | ||
| note('Looks like the database in the container image is corrupted.'); | ||
| note('Site content was not changed.'); | ||
| quit(1); | ||
| } | ||
|
|
||
| note('Fresh site content will be imported from the database dump file.'); | ||
| provision_from_db($provision_db); | ||
| // Let the downstream scripts know that the database is fresh. | ||
| $provision_override_db = '1'; | ||
| putenv('VORTEX_PROVISION_OVERRIDE_DB=1'); | ||
| } | ||
| } | ||
| else { | ||
| info('Provisioning site from the profile.'); | ||
| note('Profile: %s.', $drupal_profile); | ||
|
|
||
| if ($site_is_installed) { | ||
| note('Existing site was found.'); | ||
|
|
||
| if ($provision_override_db === '1') { | ||
| note('Existing site content will be removed and new content will be created from the profile.'); | ||
| provision_from_profile($drupal_profile, $drupal_site_name, $drupal_site_email, $drupal_admin_email, $site_has_config_files); | ||
| // Let the downstream scripts know that the database is fresh. | ||
| $provision_override_db = '1'; | ||
| putenv('VORTEX_PROVISION_OVERRIDE_DB=1'); | ||
| } | ||
| else { | ||
| note('Site content will be preserved.'); | ||
| note('Sanitization will be skipped for an existing database.'); | ||
| $provision_sanitize_db_skip = '1'; | ||
| putenv('VORTEX_PROVISION_SANITIZE_DB_SKIP=1'); | ||
| } | ||
| } | ||
| else { | ||
| note('Existing site was not found.'); | ||
| note('Fresh site content will be created from the profile.'); | ||
| provision_from_profile($drupal_profile, $drupal_site_name, $drupal_site_email, $drupal_admin_email, $site_has_config_files); | ||
| $provision_override_db = '1'; | ||
| putenv('VORTEX_PROVISION_OVERRIDE_DB=1'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider refactoring the provisioning logic for better maintainability.
This large nested if/else block (70+ lines) has significant code duplication between database and profile provisioning paths. Extracting separate functions for each scenario would improve maintainability and testability.
🤖 Prompt for AI Agents
In @.vortex/tooling/src/provision around lines 361 - 432, The large nested
if/else in the provisioning block duplicates logic between the 'database' and
'profile' branches; extract two helper functions (e.g.,
handleProvisionFromDatabase(...) and handleProvisionFromProfile(...)) that
accept the relevant inputs ($provision_db, $db_image, $drupal_profile,
$drupal_site_name, $drupal_site_email, $site_has_config_files,
$site_is_installed) and encapsulate the site_is_installed branches, calls to
provision_from_db / provision_from_profile, and the shared environment flag
handling ($provision_override_db, putenv('VORTEX_PROVISION_OVERRIDE_DB=1'),
$provision_sanitize_db_skip, putenv('VORTEX_PROVISION_SANITIZE_DB_SKIP=1'));
then replace the original nested blocks with a simple call to the appropriate
helper based on $provision_type to remove duplication and improve testability
while preserving messages like note(...) and quit(1).
|
|
||
| echo PHP_EOL; | ||
|
|
||
| $environment = trim(drush("php:eval \"print \\Drupal\\core\\Site\\Settings::get('environment');\"")); |
There was a problem hiding this comment.
Fix typo in Settings class name.
The class name uses lowercase \Drupal\core\Site\Settings but should be \Drupal\Core\Site\Settings with uppercase 'C'.
🔎 Proposed fix
-$environment = trim(drush("php:eval \"print \\Drupal\\core\\Site\\Settings::get('environment');\""));
+$environment = trim(drush("php:eval \"print \\Drupal\\Core\\Site\\Settings::get('environment');\""));📝 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
| $environment = trim(drush("php:eval \"print \\Drupal\\core\\Site\\Settings::get('environment');\"")); | |
| $environment = trim(drush("php:eval \"print \\Drupal\\Core\\Site\\Settings::get('environment');\"")); |
🤖 Prompt for AI Agents
In @.vortex/tooling/src/provision at line 436, The drush php:eval call uses the
wrong-cased class name "\Drupal\core\Site\Settings"; update the namespace to
"\Drupal\Core\Site\Settings" (uppercase C) in the drush invocation that assigns
$environment so the correct Drupal Settings class is referenced; ensure the
string passed to drush(...) reflects the corrected fully-qualified class name
used in the expression print \Drupal\Core\Site\Settings::get('environment');.
|
Member
Author
|
Member
Author
|
Member
Author
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## project/2.x #2222 +/- ##
============================================
Coverage 76.20% 76.20%
============================================
Files 105 105
Lines 5683 5683
============================================
Hits 4331 4331
Misses 1352 1352 ☔ 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
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 #
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.