Repository navigation
setup workflows - #6
Conversation
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds developer tooling and CI (GitHub Actions, PHPUnit, PHPStan, PHPCS, PHP-CS-Fixer), updates Composer for PHP ^8.2 and dev deps, extracts test stubs to dedicated files, and applies minor typing and reference-style edits in the Settings library. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 7
🧹 Nitpick comments (2)
tests/Stubs/SettingModelStub.php (2)
17-21:where()/first()assume a hard-coded'key'columnThe stub records only the value in
where($col, $val)and later filters rows by$row['key']infirst(), ignoring the$colargument entirely. This is fine if tests only ever callwhere('key', $value), but it will behave unexpectedly for other columns (e.g.,where('id', 1)will still compare against'key').If you want a more faithful stub, consider storing both column and value:
- protected ?string $whereKey = null; + protected ?string $whereColumn = null; + protected mixed $whereValue = null; public function where($col, $val) { - $this->whereKey = $val; + $this->whereColumn = $col; + $this->whereValue = $val; return $this; } public function first() { - if ($this->whereKey === null) { + if ($this->whereColumn === null) { return null; } foreach ($this->rows as $row) { - if (($row['key'] ?? null) === $this->whereKey) { + if (($row[$this->whereColumn] ?? null) === $this->whereValue) { return $row; } } return null; }If the tests only care about
'key', this can be deferred, but documenting the assumption in a comment would help future readers.Also applies to: 28-39
17-18: Tidy unused parameters or suppress them to satisfy static analysisSeveral parameters are intentionally unused in this stub (
$col,$cols,$limit,$offset,$returnID,$purge), which PHPMD has flagged in the static analysis hints. To keep tools quiet without changing behavior, you could:
- Prefix with
_to signal “intentionally unused”, e.g.$_col,$_limit.- Or add lightweight
unset()calls:public function where($col, $val) { unset($col); $this->whereKey = $val; return $this; } public function findAll(?int $limit = null, int $offset = 0) { unset($limit, $offset); return $this->rows; }Alternatively, exclude this stub file from PHPMD rules. This is low priority but will reduce noise in your reports.
Based on static analysis hints.
Also applies to: 23-24, 41-42, 46-47, 66-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/php-ci.yml(1 hunks).gitignore(1 hunks).php-cs-fixer.dist.php(1 hunks)composer.json(1 hunks)phpcs.xml(1 hunks)phpstan.neon(1 hunks)phpunit.xml(1 hunks)src/Libraries/Settings.php(6 hunks)src/Models/SettingModel.php(0 hunks)tests/SettingsTest.php(3 hunks)tests/Stubs/SettingConfigStub.php(1 hunks)tests/Stubs/SettingModelStub.php(1 hunks)
💤 Files with no reviewable changes (1)
- src/Models/SettingModel.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/SettingsTest.php (4)
src/Libraries/Settings.php (4)
Settings(8-401)set(39-63)get(29-34)delete(68-107)tests/Stubs/SettingModelStub.php (3)
SettingModelStub(7-87)delete(66-75)getRows(83-86)tests/Stubs/SettingConfigStub.php (1)
SettingConfigStub(7-15)src/Helpers/settings_helper.php (1)
settings(15-28)
tests/Stubs/SettingConfigStub.php (2)
src/Libraries/Settings.php (2)
Settings(8-401)__construct(20-24)tests/Stubs/SettingModelStub.php (1)
__construct(12-15)
🪛 PHPMD (2.15.0)
tests/Stubs/SettingModelStub.php
17-17: Avoid unused parameters such as '$col'. (undefined)
(UnusedFormalParameter)
23-23: Avoid unused parameters such as '$cols'. (undefined)
(UnusedFormalParameter)
41-41: Avoid unused parameters such as '$limit'. (undefined)
(UnusedFormalParameter)
41-41: Avoid unused parameters such as '$offset'. (undefined)
(UnusedFormalParameter)
46-46: Avoid unused parameters such as '$returnID'. (undefined)
(UnusedFormalParameter)
66-66: Avoid unused parameters such as '$purge'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (17)
.gitignore (1)
2-10: LGTM!The ignore patterns are appropriate for a PHP project with CI/CD infrastructure, covering vendor dependencies, test artifacts, build outputs, and IDE-specific files.
phpcs.xml (1)
26-34: LGTM!The PSR-12 base standard with the line length relaxation is a practical choice for real-world PHP code, especially when dealing with configuration arrays or SQL queries.
.php-cs-fixer.dist.php (1)
9-14: LGTM!The configuration is sensible with
@autorules and parallel execution. Disallowing risky fixes is a safe default for CI environments.src/Libraries/Settings.php (4)
29-29: LGTM!The addition of
mixedtype hints improves type safety and is appropriate for a method that can return various types based on the stored settings.
302-302: LGTM!The addition of explicit type hints (
array &$array,string $path,voidreturn type) improves type safety and makes the method signatures clearer.Also applies to: 340-340
319-319: LGTM!The consistent spacing in reference assignments (
= &instead of=&) improves readability and aligns with common PHP style guidelines.Also applies to: 336-336, 355-355, 362-362
309-309: No action required. The project's minimum PHP version is set to 8.2 incomposer.json, which exceeds the PHP 8.0 requirement forstr_contains(). The code is compatible.tests/SettingsTest.php (1)
3-8: LGTM!The namespace update and use of external stubs improves test organization and reusability. This is a cleaner approach than inline stub definitions.
phpstan.neon (3)
5-11: LGTM!Level 5 provides a good balance of type safety without being overly strict. Including both
srcandtestsin the analysis ensures comprehensive coverage.
19-26: LGTM!The strict checks (
checkMissingIterableValueType,checkGenericClassInNonGenericObjectType, etc.) are valuable for catching type-related issues early. SettingtreatPhpDocTypesAsCertain: falseis a pragmatic choice that prevents PHPStan from making overly strong assumptions from documentation.
14-17: No action needed. Thecodeigniter4/frameworkdependency is already declared incomposer.jsonwith version constraint^4.0, making the reference tovendor/codeigniter4/framework/system/Test/bootstrap.phpvalid.phpunit.xml (2)
2-12: LGTM!The strict configuration (
failOnRisky="true",failOnWarning="true",beStrictAboutChangesToGlobalState="true") ensures high-quality tests and catches potential issues early.
30-46: LGTM!The PHP and environment configuration is well-suited for testing:
- Full error reporting ensures all issues are visible
- CI4 testing environment flags are properly set
- The commented SQLite configuration provides a helpful template for database testing
tests/Stubs/SettingConfigStub.php (1)
9-14: Skipping parent constructor is intentional and safe.The parent
Settingsclass has no constructor—it only defines properties with defaults ($useCache = true,$cacheTTL = 600,$cacheKey = 'ci4_settings'). The stub directly sets its own property values without callingparent::__construct(), which is the established pattern in this codebase (seeSettingModelStub) to avoid CI dependencies in tests. No required initialization logic is skipped.composer.json (2)
11-15: Test autoload namespace looks goodThe
autoload-devPSR-4 mapping ("Jide\\Settings\\Tests\\": "tests/") is consistent with the test namespaces described in the PR and should work well with PHPUnit/autoloading.
42-46: Composer config and stability flags look reasonable
"sort-packages": true,"minimum-stability": "stable", and"prefer-stable": trueare sensible defaults for a library and should keep your dependency graph predictable.tests/Stubs/SettingModelStub.php (1)
54-75: Update/delete semantics look consistent for a test stubThe
update()anddelete()implementations correctly search by'id', merge new data while preserving the ID on update, and remove matching rows on delete. For a simple in-memory stub these semantics are clear and adequate.
| name: Github Action for CI4 Settings and Feature Flag Library | ||
|
|
||
| on: | ||
| push: | ||
| branches: [ "main", "develop" ] | ||
| pull_request: | ||
| branches: [ "main", "develop" ] | ||
| # Manual runs from the Actions tab | ||
| workflow_dispatch: | ||
|
|
There was a problem hiding this comment.
Workflow file location will prevent it from running
This workflow is defined as .github/php-ci.yml in the PR, but GitHub Actions only loads workflows from .github/workflows/*.yml. Unless there is a planned move/rename, this file will not be picked up by GitHub, and the CI will never run. Consider moving it to .github/workflows/php-ci.yml (or similar).
🤖 Prompt for AI Agents
.github/php-ci.yml lines 1-10: the workflow file is placed at .github/php-ci.yml
which GitHub Actions does not load since workflows must live in
.github/workflows/*.yml; move/rename this file to .github/workflows/php-ci.yml
(or another name under .github/workflows/) so GitHub will detect and run the
workflow on push, pull_request, and workflow_dispatch events.
| - name: Run PHP-CS-Fixer (dry-run, if configured) | ||
| if: ${{ hashFiles('.php-cs-fixer.dist.php', '.php-cs-fixer.php') != '' }} | ||
| run: | | ||
| if [ -f vendor/bin/phpcbf ]; then | ||
| vendor/bin/phpcbf fix --dry-run --diff |
There was a problem hiding this comment.
PHP-CS-Fixer step is wired to the wrong binary/command
This step is labeled “PHP-CS-Fixer” and gated on .php-cs-fixer.* configs, but it checks for vendor/bin/phpcbf and runs phpcbf fix --dry-run --diff. phpcbf is PHP_CodeSniffer’s fixer, not PHP-CS-Fixer, and it doesn’t take a fix subcommand. If the intention is to use PHP-CS-Fixer, this should instead be:
- - name: Run PHP-CS-Fixer (dry-run, if configured)
+ - name: Run PHP-CS-Fixer (dry-run, if configured)
if: ${{ hashFiles('.php-cs-fixer.dist.php', '.php-cs-fixer.php') != '' }}
run: |
- if [ -f vendor/bin/phpcbf ]; then
- vendor/bin/phpcbf fix --dry-run --diff
+ if [ -f vendor/bin/php-cs-fixer ]; then
+ vendor/bin/php-cs-fixer fix --dry-run --diff
fiOr, if you actually want to use PHPCBF here, drop the PHP-CS-Fixer configs from the condition and rename the step accordingly.
🤖 Prompt for AI Agents
.github/php-ci.yml lines 109-113: the step is labeled and gated as
“PHP-CS-Fixer” but is checking and running PHPCBF (vendor/bin/phpcbf) with an
invalid `fix` subcommand; either switch to the PHP-CS-Fixer binary and command
or change the step to PHPCBF and its gating. Fix by replacing vendor/bin/phpcbf
with the php-cs-fixer binary (e.g., vendor/bin/php-cs-fixer) and run the proper
php-cs-fixer invocation for a dry-run with diffs, or if you intend to use PHPCBF
instead, update the if condition to check for PHPCBF-related config/files and
rename the step to “PHPCBF” and use the correct PHPCBF command/flags.
| "require": { | ||
| "php": "^8.0", | ||
| "codeigniter4/framework": "^4.0" | ||
| } | ||
| "php": "^8.2" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CodeIgniter 4 official PHP version support requirements latest release
💡 Result:
Short answer
- CodeIgniter 4 (latest stable v4.6.3) requires PHP 8.1 or newer. [1][2]
Compatibility notes
- PHP 8.1: minimum for CI4. [1]
- PHP 8.2 requires CI 4.2.11 or later. [1]
- PHP 8.3 requires CI 4.4.4 or later. [1]
- PHP 8.4 requires CI 4.6.0 or later. [1]
Sources
- CodeIgniter 4 — Server requirements (official User Guide). [1]
- CodeIgniter download page (shows latest v4.6.3 and PHP 8.1+). [2]
Confirm intentional drop of PHP 8.1 support
Raising the requirement to "php": "^8.2" drops support for PHP 8.1, which is the minimum version officially supported by CodeIgniter 4. Your CI matrix (8.2 and 8.3) aligns with CodeIgniter 4's supported versions—note that CodeIgniter 4 v4.4.4+ supports PHP 8.3. Consider whether you intentionally want to require PHP 8.2+ rather than CodeIgniter 4's minimum of 8.1.
🤖 Prompt for AI Agents
In composer.json around lines 16-18 the PHP requirement was raised to "php":
"^8.2", which removes PHP 8.1 support; if that was not intentional, change the
constraint back to "php": "^8.1" to maintain CodeIgniter 4 compatibility, update
the CI matrix and README to reflect supported PHP versions, or if you do intend
to drop 8.1, add a short justification in the PR description and project docs
stating the intentional minimum of PHP 8.2 and the reason (e.g., using 8.2-only
features).
| "require-dev": { | ||
| "codeigniter4/framework": "^4.0", | ||
| "phpunit/phpunit": "^10.0", | ||
| "phpstan/phpstan": "^1.11", | ||
| "phpstan/phpstan-phpunit": "^1.4", | ||
| "squizlabs/php_codesniffer": "^3.9", | ||
| "friendsofphp/php-cs-fixer": "^3.0", | ||
| "systemsdk/phpcpd": "^7.0.1" | ||
| }, | ||
| "scripts": { | ||
| "test": "phpunit --testdox", | ||
| "analyse": "phpstan analyse src tests", | ||
| "deduplicate": "phpcpd src/ tests/", | ||
| "cs": "phpcs", | ||
| "cs-fix": "phpcbf", | ||
| "style": "@cs-fix", | ||
| "ci": [ | ||
| "@cs", | ||
| "@deduplicate", | ||
| "@analyse", | ||
| "@test" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
squizlabs php_codesniffer command-line executables phpcs phpcbf
💡 Result:
Summary — phpcs and phpcbf (squizlabs/php_codesniffer)
-
What they are:
- phpcs — PHP_CodeSniffer command-line tool that tokenizes files and reports coding-standard violations. [1]
- phpcbf — "PHP Code Beautifier and Fixer" that attempts to automatically fix violations reported by phpcs. [1][2]
-
How to run them:
- PHAR downloads:
- curl/wget phpcs.phar and phpcbf.phar, then run with PHP: php phpcs.phar and php phpcbf.phar . [1][2]
- Composer (recommended):
- Project local: composer require --dev squizlabs/php_codesniffer → run ./vendor/bin/phpcs and ./vendor/bin/phpcbf. [1][2]
- Global: composer global require squizlabs/php_codesniffer → add Composer global bin dir (e.g. ~/.composer/vendor/bin or composer global config bin-dir --absolute) to PATH so phpcs/phpcbf are available system-wide (Windows installs include .bat wrappers). [1][2][3]
- Git clone / source: php bin/phpcs and php bin/phpcbf from the cloned repo. [1][2]
- PHAR downloads:
-
Typical executable locations:
- ./vendor/bin/phpcs, ./vendor/bin/phpcbf (project). [1][2]
- ~/.composer/vendor/bin/ (or Composer bin-dir) for global installs; Windows may use .bat files in that bin dir. [1][2][7]
-
Quick examples:
- Check code: phpcs --standard=PSR12 path/to/code. [1]
- Auto-fix: phpcbf --standard=PSR12 path/to/code. [1][6]
Sources:
[1] squizlabs/PHP_CodeSniffer (README/installation).
[2] PHPCSStandards/PHP_CodeSniffer (GitHub docs & PHAR instructions).
[3] VS Code extension docs / community guides on locating phpcs/phpcbf.
[4] Additional install/usage docs (examples for Composer/Phive/PEAR).
🌐 Web query:
friendsofphp php-cs-fixer command-line executable php-cs-fixer
💡 Result:
Short answer — php-cs-fixer is a CLI PHAR/composer package. Common ways to get the php-cs-fixer executable and how to run it:
-
Per-project (recommended):
- Install: composer require --dev friendsofphp/php-cs-fixer
- Run: ./vendor/bin/php-cs-fixer fix . [1][3]
-
Global Composer install:
- composer global require friendsofphp/php-cs-fixer
- Make sure Composer's global vendor bin (e.g. $HOME/.composer/vendor/bin or $HOME/.config/composer/vendor/bin) is in your PATH, then run php-cs-fixer. [1][8]
-
PHAR / manual executable:
- Download PHAR (example): curl -L https://cs.symfony.com/download/php-cs-fixer-v3.phar -o php-cs-fixer
- Make executable and move to a bin dir: chmod a+x php-cs-fixer && sudo mv php-cs-fixer /usr/local/bin/php-cs-fixer
- Run: php-cs-fixer fix (or just php-cs-fixer if moved into PATH). [1][3][5]
-
Other options: Homebrew, Docker, or phive; project docs list available installation methods. [1][3]
Docs/references: [1] Symfony docs (php-cs-fixer installation). [2] PHP-CS-Fixer GitHub. [3] Older install/PHAR instructions and examples.
🏁 Script executed:
# Check the current state of composer.json to verify the claims
cat -n composer.json | head -50Repository: olajideolamide/ci4-settings
Length of output: 1443
🏁 Script executed:
# Look for the CI file mentioned (.github/php-ci.yml)
find . -name "*ci*.yml" -o -name "*ci*.yaml" | head -20Repository: olajideolamide/ci4-settings
Length of output: 91
🏁 Script executed:
# Check the CI file contents
cat -n ./.github/php-ci.ymlRepository: olajideolamide/ci4-settings
Length of output: 4700
🌐 Web query:
phpcbf command-line options syntax dry-run diff squizlabs php_codesniffer
💡 Result:
Short answer: phpcbf itself does not provide a standard --dry-run/--diff switch to preview changes; use phpcs' diff report or safe phpcbf modes (suffix, STDIN, VCS) to preview changes before applying.
Key points + examples
-
Basic phpcbf usage:
- phpcbf [options] <file|dir> ... . Options include --standard, --extensions, --tab-width, --encoding, --processes, --suffix, --basepath, filters, severity, etc. [1][2]
-
Preview changes (recommended)
- Generate a unified diff with phpcs and review it (no files changed):
- phpcs --report=diff /path/to/code > changes.diff
- Review changes.diff or apply with patch if desired. (phpcs prints diffs that show what phpcbf would change). [1]
- Use phpcbf but avoid overwriting originals:
- phpcbf --suffix=".orig" /path/to/code # writes modified files with the given suffix so originals remain. (phpcbf supports --suffix). [1][3]
- Use version control (git): run phpcbf and then inspect git diff / stage/undo as needed — safest in CI or local dev workflow.
- Generate a unified diff with phpcs and review it (no files changed):
-
Why there’s no simple --dry-run flag on phpcbf
- The docs/wiki describe generating diffs via phpcs' --report=diff and note phpcbf will automatically apply fixes; phpcbf’s CLI options focus on how fixes are applied (suffix, processes, severity, filters), not a dedicated dry-run flag. Use phpcs --report=diff to preview. [1][3]
References
- PHPCS "Fixing Errors Automatically" / phpcbf usage (wiki). [squizlabs / PHP_CodeSniffer]. [1]
- Official repository / README and usage (squizlabs / PHPCS and successor PHPCSStandards). [2]
- phpcbf manpage / Debian packaging options (lists --suffix, filter, processes, etc.). [3]
If you want, I can give exact commands for your repo (git-aware preview, a one-liner producing diffs, or a small script that runs phpcs -> phpcbf safely).
Align code style tools between require-dev, composer scripts, and CI
You require both squizlabs/php_codesniffer and friendsofphp/php-cs-fixer, but they serve overlapping purposes:
scripts.csrunsphpcs(CodeSniffer checker)scripts.cs-fixrunsphpcbf(CodeSniffer's fixer).github/php-ci.ymlstep 10 is labeled "Run PHP-CS-Fixer (dry-run, if configured)" but callsvendor/bin/phpcbf fix --dry-run --diff— which is invalid syntax for phpcbf (it has nofixsubcommand or--dry-runflag)
This makes friendsofphp/php-cs-fixer effectively unused and creates a broken CI step. Consider one of:
-
Use PHP-CS-Fixer as the primary style tool
Update composer scripts to call
php-cs-fixerand fix the CI step:"scripts": {
- "cs": "phpcs",
- "cs-fix": "phpcbf",
- "cs": "php-cs-fixer fix --dry-run --diff",
- "cs-fix": "php-cs-fixer fix",
"style": "@cs-fix",
}
Update CI step 10 to correctly call `vendor/bin/php-cs-fixer fix --dry-run --diff`, and remove `squizlabs/php_codesniffer`.
2. **Use PHPCS/PHPCBF only**
- Remove `friendsofphp/php-cs-fixer` and any `.php-cs-fixer.*` config files.
- Fix CI step 10 to use `vendor/bin/phpcs` for checking (similar to step 9) or remove the step entirely.
Picking one approach will simplify maintenance and make the CI step functional.
> Committable suggestion skipped: line range outside the PR's diff.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In composer.json at lines 19-41, the repo mixes PHPCS (phpcs/phpcbf) and
PHP-CS-Fixer and the CI step 10 is calling an invalid phpcbf command; fix by
choosing one tool: either (A) make PHP-CS-Fixer the canonical style tool —
remove squizlabs/php_codesniffer from require-dev, update scripts.cs and
scripts.cs-fix to invoke vendor/bin/php-cs-fixer (e.g., "php-cs-fixer" and a fix
command), and change .github/php-ci.yml step 10 to call vendor/bin/php-cs-fixer
fix --dry-run --diff; or (B) keep PHPCS/PHPCBF only — remove
friendsofphp/php-cs-fixer and any php-cs-fixer config, and update CI step 10 to
use vendor/bin/phpcs (or remove that step) so no invalid phpcbf flags are used;
apply the chosen change consistently across composer scripts and CI.
</details>
<!-- fingerprinting:phantom:poseidon:olive -->
<!-- This is an auto-generated comment by CodeRabbit -->
| <description> | ||
| Coding standard for the CI4 Settings / Feature Flag library. | ||
| Applies PSR-12 to src/ and tests/ while ignoring vendor and build artifacts. | ||
| </description> |
There was a problem hiding this comment.
Clarify the library description.
The description mentions "CI4 Settings / Feature Flag library", but the PR title and context suggest this is primarily a settings library. If feature flags are a core part of this library, this is fine; otherwise, consider simplifying to "CI4 Settings Library".
🤖 Prompt for AI Agents
phpcs.xml around lines 3 to 6: the <description> currently reads "Coding
standard for the CI4 Settings / Feature Flag library" which may misrepresent
scope; update the description to accurately reflect the repository (e.g.,
"Coding standard for the CI4 Settings Library") or, if feature flags are indeed
core, make that explicit (e.g., "CI4 Settings and Feature Flags Library");
change only the text inside the <description> element to this clarified wording
and ensure it matches the PR title/context.
| public function insert($data = null, bool $returnID = true) | ||
| { | ||
| $id = $this->rows ? (max(array_column($this->rows, 'id')) + 1) : 1; | ||
| $data['id'] = $id; | ||
| $this->rows[] = $data; | ||
| return $id; | ||
| } |
There was a problem hiding this comment.
Harden insert() against missing IDs and non-array $data
insert() assumes:
- Existing
$this->rowsall have an'id'key (otherwisemax(array_column(...))on an empty array will throw in PHP 8+). $datais always a non-null array ($data['id']will fatal if$dataisnull).
Given this is a reusable test stub, it’s safer to guard these cases:
public function insert($data = null, bool $returnID = true)
{
- $id = $this->rows ? (max(array_column($this->rows, 'id')) + 1) : 1;
- $data['id'] = $id;
- $this->rows[] = $data;
- return $id;
+ $ids = array_filter(
+ array_column($this->rows, 'id'),
+ static fn ($v) => is_int($v) || ctype_digit((string) $v)
+ );
+ $id = $ids ? (max($ids) + 1) : 1;
+
+ $row = (array) $data;
+ $row['id'] = $id;
+ $this->rows[] = $row;
+
+ return $id;
}This keeps behavior the same for well-formed rows while avoiding hard failures in edge cases.
🧰 Tools
🪛 PHPMD (2.15.0)
46-46: Avoid unused parameters such as '$returnID'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In tests/Stubs/SettingModelStub.php around lines 46 to 52, the insert()
implementation assumes $this->rows all have an 'id' key and that $data is a
non-null array; to harden it, ensure $data is cast to an array (e.g. $data =
(array) $data) before using it, compute the new id by collecting existing ids
only from rows that have an 'id' key (e.g. filter array_column or loop to avoid
passing an empty array to max), default to 1 when no existing ids found, assign
$data['id'] = $id, append $data to $this->rows and return $id (respecting
$returnID as before).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/action.yml (1)
67-73: Minor: Unconventional fallback file naming in .env preparation.Line 71 checks for a file named
env(without a dot prefix). While this may work, CodeIgniter conventionally uses.env(with a leading dot) or.env.example. Consider aligning with CI4 conventions by checking for.env.exampleinstead if that's the intended fallback, or clarify whyenv(without dot) is expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/action.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/action.yml (1)
1-14: File location and job structure are now correct. ✓The workflow is properly placed at
.github/workflows/action.yml(fixing the previous issue), and the job name correctly incorporates thedepsmatrix variable into thenamefield instead of using an invalid top-leveldepskey. Both prior structural concerns have been resolved.
| - name: Run PHP-CBF (dry-run, if configured) | ||
| if: ${{ hashFiles('.php-cs-fixer.dist.php', '.php-cs-fixer.php') != '' }} | ||
| run: | | ||
| if [ -f vendor/bin/phpcbf ]; then | ||
| vendor/bin/phpcbf fix --dry-run --diff | ||
| fi |
There was a problem hiding this comment.
Mismatch between tool command and config files.
The step checks for .php-cs-fixer.dist.php (php-cs-fixer configuration), but line 112 executes vendor/bin/phpcbf, which is the executable for squizlabs/php_codesniffer—a different tool with incompatible configuration formats. The php-cs-fixer tool uses the command vendor/bin/php-cs-fixer fix --dry-run --diff. This mismatch will cause the step to fail or behave unexpectedly.
Apply this diff to use the correct command:
- name: Run PHP-CBF (dry-run, if configured)
if: ${{ hashFiles('.php-cs-fixer.dist.php', '.php-cs-fixer.php') != '' }}
run: |
if [ -f vendor/bin/php-cs-fixer ]; then
- vendor/bin/phpcbf fix --dry-run --diff
+ vendor/bin/php-cs-fixer fix --dry-run --diff
fi📝 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.
| - name: Run PHP-CBF (dry-run, if configured) | |
| if: ${{ hashFiles('.php-cs-fixer.dist.php', '.php-cs-fixer.php') != '' }} | |
| run: | | |
| if [ -f vendor/bin/phpcbf ]; then | |
| vendor/bin/phpcbf fix --dry-run --diff | |
| fi | |
| - name: Run PHP-CBF (dry-run, if configured) | |
| if: ${{ hashFiles('.php-cs-fixer.dist.php', '.php-cs-fixer.php') != '' }} | |
| run: | | |
| if [ -f vendor/bin/php-cs-fixer ]; then | |
| vendor/bin/php-cs-fixer fix --dry-run --diff | |
| fi |
🤖 Prompt for AI Agents
.github/workflows/action.yml around lines 108 to 113: the step guarding on
php-cs-fixer config files mistakenly runs vendor/bin/phpcbf (phpcs-fixer vs
phpcbf mismatch); change the executed binary to vendor/bin/php-cs-fixer and keep
the same flags (fix --dry-run --diff) so the command matches the checked config
files and the intended tool.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
phpunit.xml (1)
29-44: Minor: Clean up whitespace and clarify strict global state setting.The environment configuration is sensible, but there are two minor improvements:
- Remove the empty lines 31–32 for consistency.
- Consider adding a brief comment explaining the
beStrictAboutChangesToGlobalStatestrictness (line 9) to clarify that tests must avoid modifying global state.The commented-out SQLite configuration is helpful for future reference.
<php> - - <!-- common CI4 testing env flags --> <env name="APP_ENV" value="testing"/> <env name="CI_ENVIRONMENT" value="testing"/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/action.yml(1 hunks)phpunit.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/action.yml
🔇 Additional comments (2)
phpunit.xml (2)
13-27: Test suite and coverage configuration look solid.The test discovery pattern and source filtering are well-structured. Ensure that:
- All test files in the
testsdirectory follow the*Test.phpnaming convention.- The
beStrictAboutChangesToGlobalStatesetting (line 9) is compatible with your test suite (i.e., no tests inappropriately modify global state).
1-12: The PHPUnit schema version 10.5 is compatible with thecomposer.jsonconstraint (^10.0), which allows any PHPUnit 10.x version. No action needed.
1 participant
No description provided.