feat: add database grants and connectivity check#19
Merged
konradmichalik merged 7 commits intomainfrom Feb 19, 2026
Merged
Conversation
…irements-database-grants
…ment' into feature/requirements-database-grants
📝 WalkthroughWalkthroughAdds a new database-grants requirement check: credential resolution, grant parsing, and connectivity verification across Root, Simple (pool), and Mittwald API modes, plus config toggle, autoload registration, requirements-list display, and documentation updates. Changes
Sequence DiagramsequenceDiagram
participant Task as Database Grants Task
participant Creds as Credential Resolver
participant MySQL as MySQL Client
participant Parser as Grant Parser
participant Report as Requirements Reporter
Task->>Creds: resolveDatabaseCredentials()
Creds->>Creds: check Deployer config, env vars, shared .env
Creds-->>Task: credentials or null
alt credentials resolved
Task->>MySQL: connect (binary/host/port/user)
MySQL-->>Task: connection/result
Task->>MySQL: SHOW GRANTS FOR CURRENT_USER()
MySQL-->>Task: grants output
Task->>Parser: parseGlobalGrants(output)
Parser-->>Task: missing grants / ok
Task->>Report: mark success or failure with details
else no creds
Task->>Report: mark skipped
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
deployer/requirements/config/set.php (1)
20-20: Nit: Move the toggle into the existing block for consistency.All other
_enabledtoggles are grouped together (lines 11–19). This new one is separated by a blank line, breaking the visual grouping.📝 Proposed fix
set('requirements_check_env_enabled', true); set('requirements_check_eol_enabled', true); - -set('requirements_check_database_grants_enabled', true); +set('requirements_check_database_grants_enabled', true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployer/requirements/config/set.php` at line 20, The new toggle set('requirements_check_database_grants_enabled', true) is separated from the other _enabled toggles; move this set call into the existing block where the other `_enabled` settings are declared (the grouped toggle block containing the other set(..., true/false) entries) so all `_enabled` flags are visually adjacent and consistent.deployer/requirements/task/check_database_grants.php (1)
106-118:array_filter($config, 'is_string')silently drops non-string values, which may mask misconfiguration.If a pool entry has e.g.
'database_name' => nullor'database_name' => 123, it will be reported as "Missing config: database_name" rather than "Invalid type for database_name". The current behavior is acceptable (a non-string value is effectively missing for this purpose), but worth noting if you want more precise diagnostics in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployer/requirements/task/check_database_grants.php` around lines 106 - 118, The loop over $pool uses array_filter($config, 'is_string') which removes keys with non-string values and causes all non-string entries to be reported as "missing"; update the validation in the foreach that builds $missingKeys to explicitly check each required key for existence and type (e.g., use isset/array_key_exists and is_string) so you can distinguish missing keys from invalid-typed keys, and then call addRequirementRow with either "Missing config: ..." or "Invalid type for: ..." as appropriate; refer to the $pool foreach, $requiredKeys, $missingKeys, and addRequirementRow locations to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployer/requirements/functions.php`:
- Around line 408-464: In resolveDatabaseCredentials(): when credentials are
populated from getSharedEnvVars() for TYPO3 or Symfony also resolve host (and
port for Symfony) so remote DBs work; for TYPO3 read
'TYPO3_CONF_VARS__DB__Connections__Default__host' from $envVars if $host is
still default and keep port at 3306, and for Symfony parse DATABASE_URL with
parse_url(..., PHP_URL_HOST) and parse_url(..., PHP_URL_PORT) (apply
urldecode/appropriate casts) to set $host and, if present, $port; ensure
existing logic for $user/$password (including env_key_database_passwort
handling) remains unchanged and that you only override $host/$port when they are
unset or still the default.
In `@docs/DATABASE.md`:
- Line 9: The document has inconsistent section headings: the root section uses
"Prerequisites" but the "Simple" and "Mittwald API" subsections are still titled
"Prerequirements"; update those subsection headings to read "Prerequisites"
instead of "Prerequirements" so all sections use the same spelling (look for the
headings under "Simple" and "Mittwald API" and replace "Prerequirements" with
"Prerequisites").
In `@docs/REQUIREMENTS.md`:
- Around line 68-72: Update the fenced code block that lists SQL privilege names
to include a language identifier; change the opening triple backticks to ```sql
so the block becomes a SQL code block (this will satisfy markdownlint MD040 for
the block containing "SELECT, INSERT, UPDATE, DELETE, CREATE, DROP, INDEX,
ALTER, CREATE TEMPORARY TABLES, LOCK TABLES, EXECUTE, CREATE VIEW, SHOW VIEW,
CREATE ROUTINE, ALTER ROUTINE").
---
Nitpick comments:
In `@deployer/requirements/config/set.php`:
- Line 20: The new toggle set('requirements_check_database_grants_enabled',
true) is separated from the other _enabled toggles; move this set call into the
existing block where the other `_enabled` settings are declared (the grouped
toggle block containing the other set(..., true/false) entries) so all
`_enabled` flags are visually adjacent and consistent.
In `@deployer/requirements/task/check_database_grants.php`:
- Around line 106-118: The loop over $pool uses array_filter($config,
'is_string') which removes keys with non-string values and causes all non-string
entries to be reported as "missing"; update the validation in the foreach that
builds $missingKeys to explicitly check each required key for existence and type
(e.g., use isset/array_key_exists and is_string) so you can distinguish missing
keys from invalid-typed keys, and then call addRequirementRow with either
"Missing config: ..." or "Invalid type for: ..." as appropriate; refer to the
$pool foreach, $requiredKeys, $missingKeys, and addRequirementRow locations to
implement this change.
Also fix inconsistent "Prerequirements" headings in DATABASE.md and add sql language identifier to REQUIREMENTS.md code block.
Contributor
Author
|
Regarding the two nitpick comments from the review: config/set.php toggle grouping: The toggle is already adjacent to the other
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deployer/requirements/functions.php (1)
513-515:explode(', ', ...)is brittle if MySQL omits the space after commas.Standard MySQL/MariaDB output always uses
', 'so in practice this works, but a more defensive split handles any whitespace variation:♻️ Proposed refactor
- foreach (explode(', ', $grantsStr) as $grant) { - $globalGrants[] = trim($grant); - } + foreach (array_map('trim', explode(',', $grantsStr)) as $grant) { + if ($grant !== '') { + $globalGrants[] = $grant; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployer/requirements/functions.php` around lines 513 - 515, The loop that splits $grantsStr uses explode(', ', $grantsStr) which fails if MySQL omits the space; change the split to be whitespace-tolerant (e.g. use preg_split('/\s*,\s*/', $grantsStr) or explode(',', $grantsStr) combined with array_map('trim', ...) ) and keep pushing trimmed entries into $globalGrants (the foreach that currently iterates over explode(', ', $grantsStr) and calls trim($grant) should be updated accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployer/requirements/functions.php`:
- Around line 423-476: The credential guard (if ($user === '' || $password ===
'')) prevents reading shared env vars so host/port resolution for typ o3/symfony
never runs when credentials are pre-resolved; change the logic so host/port are
resolved independently: call getSharedEnvVars() when $host === '127.0.0.1' or
$port === 3306 (or when $host/$port are defaults) and apply the TYPO3
(TYPO3_CONF_VARS__DB__Connections__Default__host) and Symfony (DATABASE_URL
parse_url PHP_URL_HOST / PHP_URL_PORT) resolution outside or in addition to the
existing ($user/$password) block, reusing the same $envVars and keeping the
existing keys (env_key_database_passwort, DATABASE_URL) and parse_url usage.
---
Nitpick comments:
In `@deployer/requirements/functions.php`:
- Around line 513-515: The loop that splits $grantsStr uses explode(', ',
$grantsStr) which fails if MySQL omits the space; change the split to be
whitespace-tolerant (e.g. use preg_split('/\s*,\s*/', $grantsStr) or
explode(',', $grantsStr) combined with array_map('trim', ...) ) and keep pushing
trimmed entries into $globalGrants (the foreach that currently iterates over
explode(', ', $grantsStr) and calls trim($grant) should be updated accordingly).
Comment on lines
+423
to
+476
| if ($user === '' || $password === '') { | ||
| $envVars = getSharedEnvVars(); | ||
|
|
||
| if (has('app_type') && get('app_type') === 'typo3') { | ||
| if ($user === '') { | ||
| $user = $envVars['TYPO3_CONF_VARS__DB__Connections__Default__user'] ?? ''; | ||
| } | ||
|
|
||
| if ($password === '') { | ||
| $key = has('env_key_database_passwort') | ||
| ? get('env_key_database_passwort') | ||
| : 'TYPO3_CONF_VARS__DB__Connections__Default__password'; | ||
| $password = $envVars[$key] ?? ''; | ||
| } | ||
|
|
||
| if ($host === '127.0.0.1') { | ||
| $envHost = $envVars['TYPO3_CONF_VARS__DB__Connections__Default__host'] ?? ''; | ||
|
|
||
| if ($envHost !== '') { | ||
| $host = $envHost; | ||
| } | ||
| } | ||
| } elseif (has('app_type') && get('app_type') === 'symfony') { | ||
| $databaseUrl = $envVars['DATABASE_URL'] ?? ''; | ||
|
|
||
| if ($databaseUrl !== '') { | ||
| if ($user === '') { | ||
| $parsed = parse_url($databaseUrl, PHP_URL_USER); | ||
| $user = is_string($parsed) ? urldecode($parsed) : ''; | ||
| } | ||
|
|
||
| if ($password === '') { | ||
| $parsed = parse_url($databaseUrl, PHP_URL_PASS); | ||
| $password = is_string($parsed) ? urldecode($parsed) : ''; | ||
| } | ||
|
|
||
| if ($host === '127.0.0.1') { | ||
| $parsed = parse_url($databaseUrl, PHP_URL_HOST); | ||
|
|
||
| if (is_string($parsed) && $parsed !== '') { | ||
| $host = $parsed; | ||
| } | ||
| } | ||
|
|
||
| if ($port === 3306) { | ||
| $parsed = parse_url($databaseUrl, PHP_URL_PORT); | ||
|
|
||
| if (is_int($parsed)) { | ||
| $port = $parsed; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
$host stays 127.0.0.1 when both credentials are pre-resolved before reaching the .env block.
The guard at line 423 ($user === '' || $password === '') prevents .env from being read when both credentials are already satisfied — for example, database_user set in the Deployer config and database_password supplied via DEPLOYER_CONFIG_DATABASE_PASSWORD. In that case getSharedEnvVars() is never called, so the TYPO3/Symfony host resolution on lines 438–464 is never reached and $host remains 127.0.0.1. For projects where the DB host is only in the shared .env (remote RDS/MariaDB endpoint), the grants check will silently attempt to connect to the wrong host.
The TYPO3/Symfony host-from-env logic added inside the block already handles the case where at least one credential is missing (addressing the earlier review). The remaining gap is when credentials are fully pre-resolved but database_host is not explicitly configured.
🛠️ Proposed fix
Decouple host/port resolution from the credential guard by checking whether defaults are still in place independently:
+ $needsHostFromEnv = !has('database_host') || $host === '127.0.0.1';
+ $needsPortFromEnv = !has('database_port') || $port === 3306;
+
- if ($user === '' || $password === '') {
+ if ($user === '' || $password === '' || $needsHostFromEnv || $needsPortFromEnv) {
$envVars = getSharedEnvVars();
if (has('app_type') && get('app_type') === 'typo3') {
if ($user === '') {
$user = $envVars['TYPO3_CONF_VARS__DB__Connections__Default__user'] ?? '';
}
if ($password === '') {
$key = has('env_key_database_passwort')
? get('env_key_database_passwort')
: 'TYPO3_CONF_VARS__DB__Connections__Default__password';
$password = $envVars[$key] ?? '';
}
- if ($host === '127.0.0.1') {
+ if ($needsHostFromEnv) {
$envHost = $envVars['TYPO3_CONF_VARS__DB__Connections__Default__host'] ?? '';
if ($envHost !== '') {
$host = $envHost;
}
}
} elseif (has('app_type') && get('app_type') === 'symfony') {
$databaseUrl = $envVars['DATABASE_URL'] ?? '';
if ($databaseUrl !== '') {
if ($user === '') {
$parsed = parse_url($databaseUrl, PHP_URL_USER);
$user = is_string($parsed) ? urldecode($parsed) : '';
}
if ($password === '') {
$parsed = parse_url($databaseUrl, PHP_URL_PASS);
$password = is_string($parsed) ? urldecode($parsed) : '';
}
- if ($host === '127.0.0.1') {
+ if ($needsHostFromEnv) {
$parsed = parse_url($databaseUrl, PHP_URL_HOST);
if (is_string($parsed) && $parsed !== '') {
$host = $parsed;
}
}
- if ($port === 3306) {
+ if ($needsPortFromEnv) {
$parsed = parse_url($databaseUrl, PHP_URL_PORT);
if (is_int($parsed)) {
$port = $parsed;
}
}
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deployer/requirements/functions.php` around lines 423 - 476, The credential
guard (if ($user === '' || $password === '')) prevents reading shared env vars
so host/port resolution for typ o3/symfony never runs when credentials are
pre-resolved; change the logic so host/port are resolved independently: call
getSharedEnvVars() when $host === '127.0.0.1' or $port === 3306 (or when
$host/$port are defaults) and apply the TYPO3
(TYPO3_CONF_VARS__DB__Connections__Default__host) and Symfony (DATABASE_URL
parse_url PHP_URL_HOST / PHP_URL_PORT) resolution outside or in addition to the
existing ($user/$password) block, reusing the same $envVars and keeping the
existing keys (env_key_database_passwort, DATABASE_URL) and parse_url usage.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
requirements:check:database_grantstask that validates database user permissions based ondatabase_manager_typeSHOW GRANTS, and verifies all required grants exist on*.*.envfile)ALL PRIVILEGESshortcut supportChanges
autoload.php— Register new task filedeployer/requirements/config/set.php— Addrequirements_check_database_grants_enabledtoggledeployer/requirements/functions.php— AddREQUIREMENT_DATABASE_GRANTSconstant,resolveDatabaseCredentials(),parseGlobalGrants()deployer/requirements/task/check_database_grants.php— New task with Root and Simple mode checksdeployer/requirements/task/list.php— Display database grants info inrequirements:listdeployer/requirements/task/requirements.php— Register task in check pipelinedocs/DATABASE.md— Improve Root mode docs with SQL grant exampledocs/REQUIREMENTS.md— Add database grants check documentationSummary by CodeRabbit
New Features
Documentation