Feat/4312 language permission enforcement - #4554
Conversation
📝 WalkthroughWalkthroughAdds language-scoped restrictions for users and groups. The change persists restrictions, exposes administration APIs and UI controls, enforces permissions in FAQ operations, updates migrations, and adds extensive PHPUnit and frontend coverage. ChangesLanguage restriction persistence and permission model
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Administrator
participant GroupOrUserUI
participant LanguageRestrictionAPI
participant LanguagePermissionRepository
participant Database
Administrator->>GroupOrUserUI: Select permissions and languages
GroupOrUserUI->>LanguageRestrictionAPI: Submit restriction payload with CSRF token
LanguageRestrictionAPI->>LanguagePermissionRepository: Validate and replace restrictions
LanguagePermissionRepository->>Database: Delete old rows and insert supported languages
Database-->>LanguagePermissionRepository: Return persistence result
LanguagePermissionRepository-->>LanguageRestrictionAPI: Return success or failure
LanguageRestrictionAPI-->>GroupOrUserUI: Return response
GroupOrUserUI-->>Administrator: Display notification
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 10
🧹 Nitpick comments (7)
phpmyfaq/admin/assets/src/group/groups.ts (1)
665-705: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove new visible fallback strings into translations.
The new language-restriction UI includes literal empty-state, permission-label, help, success, and failure text. This bypasses the translation system.
phpmyfaq/admin/assets/src/group/groups.ts#L665-L705: Read all language-restriction messages and fallback permission labels from translated template data.phpmyfaq/admin/assets/src/group/groups.ts#L750-L753: Read save-result messages from translated template data.phpmyfaq/admin/assets/src/user/users.ts#L519-L559: Read all language-restriction messages and fallback permission labels from translated template data.phpmyfaq/admin/assets/src/user/users.ts#L605-L608: Read save-result messages from translated template data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/admin/assets/src/group/groups.ts` around lines 665 - 705, Replace the literal language-restriction UI and result messages with values read from translated template data. In phpmyfaq/admin/assets/src/group/groups.ts:665-705, use translated empty-state, help, and fallback permission-label messages; at 750-753, use translated save success/failure messages. Apply the same changes in phpmyfaq/admin/assets/src/user/users.ts:519-559 and 605-608, preserving the existing language-restriction and save flows.Source: Coding guidelines
tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php (2)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the test file header with the other new test files.
tests/phpMyFAQ/Language/LanguageRestrictionFilterTest.phpin this pull request declaresstrict_types=1and marks the classfinal. This file does neither. The production classLanguagePermissionRepositoryalso declaresstrict_types=1. Other test classes in the repository, such astests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php, add#[CoversClass].♻️ Proposed header alignment
<?php +declare(strict_types=1); + namespace phpMyFAQ\Permission; use phpMyFAQ\Configuration; use phpMyFAQ\Database; use phpMyFAQ\Database\Sqlite3; +use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; use ReflectionClass; -class LanguagePermissionRepositoryTest extends TestCase +#[CoversClass(LanguagePermissionRepository::class)] +final class LanguagePermissionRepositoryTest extends TestCase {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php` around lines 1 - 11, Align the LanguagePermissionRepositoryTest header with the repository’s test conventions: declare strict_types=1, mark the test class final, and add the appropriate PHPUnit CoversClass attribute targeting LanguagePermissionRepository. Keep the existing imports and test behavior unchanged.
239-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
getAllUserLanguageRestrictions().
testGetAllLanguageRestrictions()covers the group variant. The user variantgetAllUserLanguageRestrictions()has no test. Both methods build a keyed result fromright_id, so a regression in the user path would go undetected.🧪 Proposed test for the user variant
+ public function testGetAllUserLanguageRestrictions(): void + { + $this->assertEmpty($this->repository->getAllUserLanguageRestrictions(0)); + + $this->repository->setUserLanguageRestrictions(1, 1, ['en', 'de']); + $this->repository->setUserLanguageRestrictions(1, 3, ['fr']); + + $all = $this->repository->getAllUserLanguageRestrictions(1); + $this->assertCount(2, $all); + $this->assertArrayHasKey(1, $all); + $this->assertArrayHasKey(3, $all); + $this->assertContains('en', $all[1]); + $this->assertContains('de', $all[1]); + $this->assertContains('fr', $all[3]); + } + public function testCheckUserGroupRightForLanguageWithNoRestrictions(): void🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php` around lines 239 - 253, Add a dedicated test for getAllUserLanguageRestrictions() alongside testGetAllLanguageRestrictions(), covering an empty user result, setting restrictions for multiple right IDs, and asserting the returned keyed entries and language values to detect regressions in the user path.phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php (3)
60-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the duplication between the user and group code paths.
getAllUserLanguageRestrictions()andgetAllLanguageRestrictions()have identical bodies except for the table name and the ID column.checkUserRightForLanguage()andcheckUserGroupRightForLanguage()share the sameNOT EXISTS OR EXISTSrestriction logic. Theset*anddelete*pairs already share helpers.Extract a private helper that takes the table name, the owner column, and the ID, in the same style as
replaceLanguageRows().Also applies to: 217-246, 312-355
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php` around lines 60 - 89, Extract the duplicated retrieval logic from getAllUserLanguageRestrictions() and getAllLanguageRestrictions() into a private helper accepting the table name, owner-column name, and owner ID, then have both methods delegate to it while preserving validation, ordering, and result grouping. Apply the same deduplication to checkUserRightForLanguage() and checkUserGroupRightForLanguage() by introducing a shared private helper for their NOT EXISTS/EXISTS restriction logic, following the existing replaceLanguageRows() helper style.
196-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the group method names symmetric with the user method names.
The user methods carry the entity in the name:
getUserLanguageRestrictions(),setUserLanguageRestrictions(),deleteUserLanguageRestrictions(). The group methods drop it:getLanguageRestrictions(),setLanguageRestrictions(),deleteLanguageRestrictions(). OnlydeleteAllForGroup()names the entity. A caller cannot tell the scope ofsetLanguageRestrictions()from the name alone.Rename the group methods to
getGroupLanguageRestrictions(),setGroupLanguageRestrictions(), anddeleteGroupLanguageRestrictions(). This class is new, so the rename has no external consumers outside this pull request. Note thatMediumPermissionexposessetLanguageRestrictions()for groups, astests/phpMyFAQ/Permission/MediumPermissionTest.phpLines 780-796 show. Keep the public permission API unchanged if it is already documented.Also applies to: 254-267, 272-286
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php` around lines 196 - 210, Rename the group-scoped methods in LanguagePermissionRepository from getLanguageRestrictions(), setLanguageRestrictions(), and deleteLanguageRestrictions() to getGroupLanguageRestrictions(), setGroupLanguageRestrictions(), and deleteGroupLanguageRestrictions(), updating all internal call sites accordingly. Preserve MediumPermission’s existing public setLanguageRestrictions() API and its behavior; only the repository method names should change.
26-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegister
LanguagePermissionRepositoryas a service.
LanguagePermissionRepositorytakesConfigurationin its constructor, andBasicPermissioninstantiates it directly. Add it tophpmyfaq/src/services.phpwithservice('phpmyfaq.configuration')so it follows the manual dependency injection pattern already used for this permission class and the project’s service-guideline requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php` around lines 26 - 31, Register LanguagePermissionRepository in services.php using the existing service-definition pattern, injecting service('phpmyfaq.configuration') into its constructor. Keep BasicPermission’s dependency resolution aligned with this registered service.Source: Coding guidelines
tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php (1)
1195-1231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the empty
languagesarray.This test covers the guard for a language the acting user does not hold. It does not cover
languages: [], which clears every restriction and grants unrestricted language access. That payload currently passes the guard; see the comment onApi/UserController.phpLines 699-711.Add a sibling test that posts
'languages' => []with the same restricted acting user and asserts HTTP 403.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php` around lines 1195 - 1231, Add a sibling test next to testSaveUserLanguageRestrictionsRejectsLanguageNotHeldByNonSuperAdmin using the same restricted non-SuperAdmin setup, but submit an empty languages array. Call saveUserLanguageRestrictions and assert the response status is HTTP 403, preserving the existing permission and CSRF setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phpmyfaq/admin/assets/src/group/groups.ts`:
- Around line 681-690: Associate each dynamically created language label with
its select in groups.ts (681-690) and users.ts (535-544): generate a unique ID
for each select, assign it to the select’s id, and set labelElement.htmlFor to
the same ID.
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php`:
- Around line 129-131: Update the CSV import flow around Import::import() so
each row’s languageCode and category are validated with
userHasPermissionForLanguage(PermissionType::FAQ_ADD, ...) before
$faq->create($faqEntity) persists it. Reuse the create endpoint’s existing
language/category restriction behavior and reject unauthorized rows rather than
allowing records in unpermitted languages.
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php`:
- Around line 259-311: The language-restriction write paths must prevent
restricted non-SuperAdmins from granting unrestricted access. In
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php:259-311,
before setLanguageRestrictions, obtain the acting user’s allowed languages with
getAllowedLanguagesForRight and return HTTP 403 when that set is restricted and
$languages is empty or contains values outside it. In
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php:699-711,
reject an empty $languages inside the existing $allowedLanguages !== null branch
before foreach. In
tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php:1195-1231,
add HTTP 403 coverage for empty languages with a restricted non-SuperAdmin for
both user and group endpoints.
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php`:
- Around line 713-714: Check the boolean result of User::getUserById($userId) in
the restriction-writing flow before evaluating isSuperAdmin(), getStatus(), or
calling setUserLanguageRestrictions(). If the lookup fails, return the same
ad_user_error_noId response used by editUser, and only write restrictions for an
existing user.
In `@phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.php`:
- Around line 319-328: Update BasicPermission::hasPermissionForLanguage to
return true immediately for SuperAdmin users before calling
languageRepository->checkUserRightForLanguage, while preserving the existing
permission and language-restriction checks for other users. Add a regression
test covering a SuperAdmin with no direct faquser_right grant row.
- Around line 281-283: Update the reset operation containing
languageRepository->deleteAllForUser() and repository->refuseAllUserRights() to
execute both changes within one database transaction, committing only when both
succeed and rolling back on failure so the language restriction cannot be lost;
if transactions are unavailable, restore or preserve the restriction when right
revocation fails. Add a test covering the failure path.
In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php`:
- Around line 405-424: Update replaceLanguageRows to validate the input
languages before deleting existing rows: preserve mixed-list behavior by
ignoring unsupported codes when at least one supported code remains, but return
false for a non-empty input containing no supported languages. Ensure this
validation occurs before the DELETE/mutation so an all-unsupported request
cannot create an unrestricted permission set.
- Around line 397-426: Update the transaction handling in
LanguagePermissionRepository and GroupCategoryPermissionRepository to use SQL
Server-compatible transaction-start SQL instead of bare BEGIN, while preserving
the existing DELETE/INSERT rollback flow. Check and handle failures from
transaction start, rollback, and commit through the available
DatabaseDriver::query() API, returning failure consistently when transaction
operations fail.
In `@phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php`:
- Around line 673-675: Update refuseAllGroupRights() to delete the group’s
language-restriction rows from the storage used by setLanguageRestrictions()
when all rights are revoked, ensuring a later grant starts unrestricted. Add a
regression test covering revoke-all followed by regranting the same right and
verifying the old language restriction is absent.
In `@tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php`:
- Around line 51-66: Update tearDown() in LanguagePermissionRepositoryTest to
reset both Database::$databaseDriver and Database::$dbType after closing the
handle, matching the cleanup performed by UserControllerTest. Preserve the
existing Configuration restoration and temporary database deletion.
---
Nitpick comments:
In `@phpmyfaq/admin/assets/src/group/groups.ts`:
- Around line 665-705: Replace the literal language-restriction UI and result
messages with values read from translated template data. In
phpmyfaq/admin/assets/src/group/groups.ts:665-705, use translated empty-state,
help, and fallback permission-label messages; at 750-753, use translated save
success/failure messages. Apply the same changes in
phpmyfaq/admin/assets/src/user/users.ts:519-559 and 605-608, preserving the
existing language-restriction and save flows.
In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php`:
- Around line 60-89: Extract the duplicated retrieval logic from
getAllUserLanguageRestrictions() and getAllLanguageRestrictions() into a private
helper accepting the table name, owner-column name, and owner ID, then have both
methods delegate to it while preserving validation, ordering, and result
grouping. Apply the same deduplication to checkUserRightForLanguage() and
checkUserGroupRightForLanguage() by introducing a shared private helper for
their NOT EXISTS/EXISTS restriction logic, following the existing
replaceLanguageRows() helper style.
- Around line 196-210: Rename the group-scoped methods in
LanguagePermissionRepository from getLanguageRestrictions(),
setLanguageRestrictions(), and deleteLanguageRestrictions() to
getGroupLanguageRestrictions(), setGroupLanguageRestrictions(), and
deleteGroupLanguageRestrictions(), updating all internal call sites accordingly.
Preserve MediumPermission’s existing public setLanguageRestrictions() API and
its behavior; only the repository method names should change.
- Around line 26-31: Register LanguagePermissionRepository in services.php using
the existing service-definition pattern, injecting
service('phpmyfaq.configuration') into its constructor. Keep BasicPermission’s
dependency resolution aligned with this registered service.
In `@tests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.php`:
- Around line 1195-1231: Add a sibling test next to
testSaveUserLanguageRestrictionsRejectsLanguageNotHeldByNonSuperAdmin using the
same restricted non-SuperAdmin setup, but submit an empty languages array. Call
saveUserLanguageRestrictions and assert the response status is HTTP 403,
preserving the existing permission and CSRF setup.
In `@tests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.php`:
- Around line 1-11: Align the LanguagePermissionRepositoryTest header with the
repository’s test conventions: declare strict_types=1, mark the test class
final, and add the appropriate PHPUnit CoversClass attribute targeting
LanguagePermissionRepository. Keep the existing imports and test behavior
unchanged.
- Around line 239-253: Add a dedicated test for getAllUserLanguageRestrictions()
alongside testGetAllLanguageRestrictions(), covering an empty user result,
setting restrictions for multiple right IDs, and asserting the returned keyed
entries and language values to detect regressions in the user path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4151114d-0b02-4027-aec9-f61544e086e8
📒 Files selected for processing (42)
docs/administration.mdphpmyfaq/admin/assets/src/api/group.test.tsphpmyfaq/admin/assets/src/api/group.tsphpmyfaq/admin/assets/src/api/user.test.tsphpmyfaq/admin/assets/src/api/user.tsphpmyfaq/admin/assets/src/group/groups.test.tsphpmyfaq/admin/assets/src/group/groups.tsphpmyfaq/admin/assets/src/interfaces/Group.tsphpmyfaq/admin/assets/src/user/users.test.tsphpmyfaq/admin/assets/src/user/users.tsphpmyfaq/assets/templates/admin/user/group.twigphpmyfaq/assets/templates/admin/user/user.twigphpmyfaq/src/phpMyFAQ/Controller/AbstractController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/FaqController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/GroupController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/UserController.phpphpmyfaq/src/phpMyFAQ/Helper/LanguageHelper.phpphpmyfaq/src/phpMyFAQ/Language/LanguageRestrictionFilter.phpphpmyfaq/src/phpMyFAQ/Permission/BasicPermission.phpphpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.phpphpmyfaq/src/phpMyFAQ/Permission/MediumPermission.phpphpmyfaq/src/phpMyFAQ/Permission/PermissionInterface.phpphpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/MigrationRegistry.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha2.phpphpmyfaq/translations/language_en.phptests/phpMyFAQ/Administration/AdminMenuBuilderTest.phptests/phpMyFAQ/Attachment/AttachmentServiceTest.phptests/phpMyFAQ/Controller/Administration/Api/FaqControllerTest.phptests/phpMyFAQ/Controller/Administration/Api/GroupControllerTest.phptests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.phptests/phpMyFAQ/Controller/Administration/FaqControllerTest.phptests/phpMyFAQ/Language/LanguageRestrictionFilterTest.phptests/phpMyFAQ/Permission/BasicPermissionTest.phptests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.phptests/phpMyFAQ/Permission/MediumPermissionTest.phptests/phpMyFAQ/Setup/Installation/DatabaseSchemaTest.phptests/phpMyFAQ/Setup/Installation/SchemaInstallerTest.phptests/phpMyFAQ/Setup/Migration/MigrationRegistryTest.php
| $this->languageRepository->deleteAllForUser($userId); | ||
|
|
||
| return $this->repository->refuseAllUserRights($userId); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Make the reset operation atomic.
If deleteAllForUser() succeeds and refuseAllUserRights() fails, the user keeps the direct right but loses its language restriction. The remaining grant becomes unrestricted.
Perform both changes in one database transaction. If a transaction is not available, preserve the restriction when revoking rights fails. Add a failure-path test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.php` around lines 281 - 283,
Update the reset operation containing languageRepository->deleteAllForUser() and
repository->refuseAllUserRights() to execute both changes within one database
transaction, committing only when both succeed and rolling back on failure so
the language restriction cannot be lost; if transactions are unavailable,
restore or preserve the restriction when right revocation fails. Add a test
covering the failure path.
| $db->query('BEGIN'); | ||
|
|
||
| $delete = sprintf('DELETE FROM %s%s WHERE %s', Database::getTablePrefix(), $table, $whereClause); | ||
| if (!$db->query($delete)) { | ||
| $db->query('ROLLBACK'); | ||
| return false; | ||
| } | ||
|
|
||
| foreach ($languages as $language) { | ||
| if (!Language::isASupportedLanguage($language)) { | ||
| continue; | ||
| } | ||
|
|
||
| $escapedLanguage = sprintf("'%s'", $db->escape($language)); | ||
|
|
||
| $insert = sprintf( | ||
| 'INSERT INTO %s%s (%s) VALUES %s', | ||
| Database::getTablePrefix(), | ||
| $table, | ||
| $columns, | ||
| $rowBuilder($escapedLanguage), | ||
| ); | ||
|
|
||
| if (!$db->query($insert)) { | ||
| $db->query('ROLLBACK'); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| $db->query('COMMIT'); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the database driver interface for transaction support.
fd -t f 'DatabaseDriver.php' phpmyfaq/src | xargs -r ast-grep outline --items all
# Find existing transaction handling patterns in the codebase.
rg -nP --type=php -C3 "query\(\s*'(BEGIN|COMMIT|ROLLBACK)" phpmyfaq/src
rg -nP --type=php -C3 '\b(beginTransaction|startTransaction|commit|rollback)\s*\(' phpmyfaq/srcRepository: thorsten/phpMyFAQ
Length of output: 5023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate database driver implementations and SQL Server dialect =="
git ls-files | rg 'phpmyfaq/src/phpMyFAQ/Database/.*(Driver|Pg|MySQL|SQLite|SqlServer|Dialect)\.php$|Migration420Alpha2\.php$' || true
echo "== DatabaseDriver interface =="
cat -n phpmyfaq/src/phpMyFAQ/Database/DatabaseDriver.php | sed -n '1,160p'
echo "== SqlServerDialect outlines =="
fd -t f 'SqlServerDialect.php' phpmyfaq/src | xargs -r ast-grep outline --items all || true
for f in $(fd -t f 'SqlServerDialect.php' phpmyfaq/src); do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo "== migration reference =="
fd -t f 'Migration420Alpha2.php' java phpmyfaq src . | head -20
for f in $(fd -t f 'Migration420Alpha2.php' .); do
echo "--- $f"
sed -n '1,180p' "$f"
done
echo "== repo usages of BEGIN/COMMIT/ROLLBACK and transaction-like methods =="
rg -n --type=php -C2 "query\(\s*'(BEGIN TRANSACTION|BEGIN\s*$|BEGIN\s*;|ROLLBACK|COMMIT)" . || true
rg -n --type=php -C2 '<\?php[[:space:]]*use PDO|beginTransaction\(|commit\(|rollback\(|startTransaction\(' phpmyfaq/src || true
echo "== inspect LanguagePermissionRepository relevant method =="
sed -n '360,435p' phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.phpRepository: thorsten/phpMyFAQ
Length of output: 21714
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find all PHP files referencing Database/Driver and sqlsrv/sql_server =="
rg -n --type=php -C2 "namespace phpMyFAQ\\\\Database|class .*Driver|extends .*Driver|sqlsrv|sql_server|SqlServer|Sql Server|mssql|odbc" phpmyfaq/src/phpMyFAQ/Database phpmyfaq/src/phpMyFAQ || true
echo "== find Doctrine/Database classes and services =="
git ls-files 'phpmyfaq/src/**/*Database*.php' 'phpmyfaq/src/**/*Db*.php' 'phpmyfaq/src/services.php' | sed -n '1,200p'
rg -n --type=php -C2 "\\\\Doctrine|Doctrine\\\\DBAL|DatabaseDriver|DatabaseService|getDb\\(|database|sqlsrv" phpmyfaq/src/phpMyFAQ src phpmyfaq/src/services.php || true
echo "== inspect services.php database-related entries =="
rg -n -C3 "db|Database|configuration|service" phpmyfaq/src/services.php || true
echo "== inspect config database keys/docs =="
rg -n -C2 "sqlsrv|sql_server|database_name|database_host|database_user|database_password" phpmyfaq/src phpmyfaq src README.md 2>/dev/null || trueRepository: thorsten/phpMyFAQ
Length of output: 50374
Replace manual transaction SQL with database-level transaction handling.
The DatabaseDriver interface exposes query(), but no transaction API exists. SQL Server uses sqlsrv/pdo_sqlsrv in the migration path, and SQL Server requires BEGIN TRANSACTION; SQL Server does not support BEGIN by itself. This leaves DELETE and INSERT statements running without a transaction scope. Use SQL Server-safe beginning SQL where BEGIN is used, handle explicit transaction errors, and duplicate the same fix in GroupCategoryPermissionRepository.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 399-399: Prevent SQL queries built from unsanitized input
Context: $db->query($delete)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-php)
[error] 400-400: Prevent SQL queries built from unsanitized input
Context: $db->query('ROLLBACK')
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-php)
[error] 419-419: Prevent SQL queries built from unsanitized input
Context: $db->query($insert)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-php)
[error] 420-420: Prevent SQL queries built from unsanitized input
Context: $db->query('ROLLBACK')
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-php)
[error] 425-425: Prevent SQL queries built from unsanitized input
Context: $db->query('COMMIT')
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-php)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php` around
lines 397 - 426, Update the transaction handling in LanguagePermissionRepository
and GroupCategoryPermissionRepository to use SQL Server-compatible
transaction-start SQL instead of bare BEGIN, while preserving the existing
DELETE/INSERT rollback flow. Check and handle failures from transaction start,
rollback, and commit through the available DatabaseDriver::query() API,
returning failure consistently when transaction operations fail.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/phpMyFAQ/Permission/MediumPermissionTest.php (2)
44-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the new inline SQL from the fixture changes.
Lines [44-45], [483-484], and [500-502] add SQL strings directly to this test. The values are constants, so this is not an SQL injection finding. The code still couples the test to table names and bypasses the permission abstraction. Use the existing repository or fixture helper for these state changes.
As per coding guidelines, “Do not add inline SQL; use the existing database abstraction layer.”
Also applies to: 483-484, 500-502
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/phpMyFAQ/Permission/MediumPermissionTest.php` around lines 44 - 45, Replace the inline DELETE queries in MediumPermissionTest with the existing repository or fixture helper that clears the corresponding permission state. Update all affected locations, including the setup and later cleanup blocks, while preserving the test’s current behavior and avoiding direct table-name SQL.Source: Coding guidelines
995-995: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the union assertion independent of result order.
getAllowedLanguagesForRight()returns a union, but Line [995] usesassertSame(['de', 'fr'], ...). A valid result in another order would fail. UseassertEqualsCanonicalizing()or sort both arrays before asserting.Proposed test fix
- $this->assertSame(['de', 'fr'], $this->mediumPermission->getAllowedLanguagesForRight(1, 1)); + $this->assertEqualsCanonicalizing( + ['de', 'fr'], + $this->mediumPermission->getAllowedLanguagesForRight(1, 1), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/phpMyFAQ/Permission/MediumPermissionTest.php` at line 995, Update the assertion for mediumPermission->getAllowedLanguagesForRight(1, 1) to compare the language union without depending on array order, using assertEqualsCanonicalizing() or sorting both expected and actual arrays before comparison.
🧹 Nitpick comments (2)
phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php (1)
447-449: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a shared backed enum for database drivers.
Convert
Database::getType()to the enum before matching. Use the enum’s SQL Server cases in bothbeginTransactionStatement()helpers atLanguagePermissionRepository.php:447-449andGroupCategoryPermissionRepository.php:270-272. Include all supported driver values in the shared enum.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php` around lines 447 - 449, The beginTransactionStatement() helper in phpMyFAQ/src/phpMyFAQ/Permission/LanguagePermissionRepository.php:447-449 must convert Database::getType() to the shared backed database-driver enum and match its SQL Server cases; apply the same change to beginTransactionStatement() in phpMyFAQ/src/phpMyFAQ/Permission/GroupCategoryPermissionRepository.php:270-272, ensuring the shared enum defines every supported driver value.Source: Coding guidelines
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php (1)
259-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute new language-restriction error messages through the translation system. All three sites return new, raw English strings for user-facing errors instead of using
Translation::get(), which violates the guideline that forbids hard-coded user-facing strings for PHP/Twig/TS/TSX/HTML files.
GroupController.php#L259-L325: replace the literal'No supported language code provided.'at line 305 with a translated string viaTranslation::get()and a corresponding translation key.UserController.php#L662-L727: replace the identical literal'No supported language code provided.'at line 696 with the same translated string used above.FaqController.php#L907-L951: replace thesprintf('Row %d: no "%s" permission for category %d.', ...)andsprintf('Row %d: no "%s" permission for language "%s".', ...)messages infindImportRowsOutOfScope()with translated equivalents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php` around lines 259 - 325, The three affected sites contain hard-coded user-facing error messages that must use translations. In phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php lines 259-325 and phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php lines 662-727, update the language validation responses in saveLanguageRestrictions to use the same Translation::get() key for “No supported language code provided.”; add the corresponding translation entry. In phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php lines 907-951, update findImportRowsOutOfScope() to replace both sprintf permission messages with translated equivalents while preserving their row, permission, category, and language values.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php`:
- Around line 445-453: Update the group-right removal flow in the surrounding
MediumPermission method to check the boolean result of
languageRepository->deleteAllForGroup($groupId). Return false when cleanup
fails, and return true only after both refuseAllGroupRights() and
language-restriction deletion succeed.
---
Outside diff comments:
In `@tests/phpMyFAQ/Permission/MediumPermissionTest.php`:
- Around line 44-45: Replace the inline DELETE queries in MediumPermissionTest
with the existing repository or fixture helper that clears the corresponding
permission state. Update all affected locations, including the setup and later
cleanup blocks, while preserving the test’s current behavior and avoiding direct
table-name SQL.
- Line 995: Update the assertion for
mediumPermission->getAllowedLanguagesForRight(1, 1) to compare the language
union without depending on array order, using assertEqualsCanonicalizing() or
sorting both expected and actual arrays before comparison.
---
Nitpick comments:
In `@phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php`:
- Around line 259-325: The three affected sites contain hard-coded user-facing
error messages that must use translations. In
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.php lines
259-325 and
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.php lines
662-727, update the language validation responses in saveLanguageRestrictions to
use the same Translation::get() key for “No supported language code provided.”;
add the corresponding translation entry. In
phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.php lines
907-951, update findImportRowsOutOfScope() to replace both sprintf permission
messages with translated equivalents while preserving their row, permission,
category, and language values.
In `@phpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.php`:
- Around line 447-449: The beginTransactionStatement() helper in
phpMyFAQ/src/phpMyFAQ/Permission/LanguagePermissionRepository.php:447-449 must
convert Database::getType() to the shared backed database-driver enum and match
its SQL Server cases; apply the same change to beginTransactionStatement() in
phpMyFAQ/src/phpMyFAQ/Permission/GroupCategoryPermissionRepository.php:270-272,
ensuring the shared enum defines every supported driver value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f33349a-093e-44a7-8977-361894cb4cf3
📒 Files selected for processing (22)
docs/administration.mdphpmyfaq/admin/assets/src/group/groups.test.tsphpmyfaq/admin/assets/src/group/groups.tsphpmyfaq/admin/assets/src/user/users.test.tsphpmyfaq/admin/assets/src/user/users.tsphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/AbstractAdministrationApiController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/FaqController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/GroupController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/UserController.phpphpmyfaq/src/phpMyFAQ/Permission/BasicPermission.phpphpmyfaq/src/phpMyFAQ/Permission/GroupCategoryPermissionRepository.phpphpmyfaq/src/phpMyFAQ/Permission/LanguagePermissionRepository.phpphpmyfaq/src/phpMyFAQ/Permission/MediumPermission.phptests/phpMyFAQ/Controller/Administration/Api/FaqControllerTest.phptests/phpMyFAQ/Controller/Administration/Api/GroupControllerTest.phptests/phpMyFAQ/Controller/Administration/Api/UserControllerTest.phptests/phpMyFAQ/Export/PdfTest.phptests/phpMyFAQ/NotificationTest.phptests/phpMyFAQ/Permission/BasicPermissionTest.phptests/phpMyFAQ/Permission/LanguagePermissionRepositoryTest.phptests/phpMyFAQ/Permission/MediumPermissionTest.phptests/phpMyFAQ/Setup/Migration/Operations/UserCreateOperationTest.php
🚧 Files skipped from review as they are similar to previous changes (6)
- phpmyfaq/admin/assets/src/group/groups.ts
- docs/administration.md
- phpmyfaq/admin/assets/src/user/users.ts
- phpmyfaq/src/phpMyFAQ/Permission/BasicPermission.php
- phpmyfaq/admin/assets/src/group/groups.test.ts
- phpmyfaq/admin/assets/src/user/users.test.ts
| if (!$this->mediumRepository->refuseAllGroupRights($groupId)) { | ||
| return false; | ||
| } | ||
|
|
||
| // The language restrictions are scoped to a group-right pair. Leaving them behind | ||
| // would silently re-apply the old scope if the same right is granted again later. | ||
| $this->languageRepository->deleteAllForGroup($groupId); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate language-restriction cleanup failure.
Line 451 ignores the result from deleteAllForGroup(). If the delete fails, this method returns true after removing the group rights. The stale rows then restrict a later re-grant of the same right, while the API reports success.
Proposed fix
- $this->languageRepository->deleteAllForGroup($groupId);
+ if (!$this->languageRepository->deleteAllForGroup($groupId)) {
+ return false;
+ }
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!$this->mediumRepository->refuseAllGroupRights($groupId)) { | |
| return false; | |
| } | |
| // The language restrictions are scoped to a group-right pair. Leaving them behind | |
| // would silently re-apply the old scope if the same right is granted again later. | |
| $this->languageRepository->deleteAllForGroup($groupId); | |
| return true; | |
| if (!$this->mediumRepository->refuseAllGroupRights($groupId)) { | |
| return false; | |
| } | |
| // The language restrictions are scoped to a group-right pair. Leaving them behind | |
| // would silently re-apply the old scope if the same right is granted again later. | |
| if (!$this->languageRepository->deleteAllForGroup($groupId)) { | |
| return false; | |
| } | |
| return true; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@phpmyfaq/src/phpMyFAQ/Permission/MediumPermission.php` around lines 445 -
453, Update the group-right removal flow in the surrounding MediumPermission
method to check the boolean result of
languageRepository->deleteAllForGroup($groupId). Return false when cleanup
fails, and return true only after both refuseAllGroupRights() and
language-restriction deletion succeed.
1 participant
Summary by CodeRabbit