Skip to content

Issue/587 Fix the Rector findings and run rector:check in CI - #588

Merged
armanist merged 2 commits into
quantum-php:masterfrom
armanist:issue/587-rector-check
Oct 19, 2026
Merged

armanist merged 2 commits into
quantum-php:masterfrom
armanist:issue/587-rector-check

Conversation

@armanist

@armanist armanist commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #587

What changed

  • Applied the 22 pending Rector findings in src/:
    • if (!isset($x)) { $x = … } becomes $x ??= … (17 files: the factories, Config, DebuggerStore, DiContainer, SleekDbal, Validator).
    • Truthiness checks on nullable objects become explicit instanceof checks (Config, FileAdapter, AssetManager).
    • Native return types on env(), CurlAdapter::getId() and CurlAdapter::getResponseCookies(), with their docblocks aligned to the native types.
  • Added a Run Rector step (composer rector:check) to .github/workflows/php.yml, right after Run PHPStan.
  • Added a Changed entry to the 3.1.0 changelog.

Why

composer rector:check failed on master for 22 files, and CI did not run it, so new findinthe fixes applied andthe step added, rector.php is enforced the same way PHP-CS-Fixer and PHPStan are.

Breaking changes

None expected. The refactors keep behavior; the only change visible to callers is the three new return types. CurlAdapter has no subclass in the framework and its interface has no native return types, so the narrowed types are compatible.

Testing

  • composer rector:check exits 0 (22 files before).

Notes

  • The instanceof checks in Config and FileAdapter guard a lazily initialized ?Data property. Replacing them with a non-nullable container is a larger change and is out of scope here.

Summary by CodeRabbit

  • Chores
    • Added an automated code-quality check to the CI workflow.
  • Refactor
    • Updated internal initialization and type checks while preserving existing behavior.
    • Added explicit return types to several public methods.
  • Documentation
    • Updated the 3.1.0 changelog with these changes.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-19T11:44:07.223428Z c8fd0e5 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9bf7d2f5-7ff7-4858-bf25-35fe5fefe457
📥 Commits

Reviewing files that changed from the base of the PR and between c8fd0e5 and cdb6a9d.

📒 Files selected for processing: 12
  • src/App/Factories/AppFactory.php
  • src/Archive/Factories/ArchiveFactory.php
  • src/Auth/Factories/AuthFactory.php
  • src/Cache/Factories/CacheFactory.php
  • src/Captcha/Factories/CaptchaFactory.php
  • src/Encryption/Factories/CryptorFactory.php
  • src/Logger/Factories/LoggerFactory.php
  • src/Mailer/Factories/MailerFactory.php
  • src/RateLimit/Factories/RateLimiterFactory.php
  • src/Renderer/Factories/RendererFactory.php
  • src/Session/Factories/SessionFactory.php
  • src/Storage/Factories/FileSystemFactory.php

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The changes apply Rector refactors across initialization, object checks, and return declarations. The PHP workflow now runs composer rector:check after PHPStan and before the test suite.

Changes: Rector findings and CI enforcement

Layer / File(s) Summary
Cached initialization and state
src/*/Factories/*, src/Archive/Adapters/ZipAdapter.php, src/Database/Adapters/Sleekdb/SleekDbal.php, src/Debugger/DebuggerStore.php, src/Di/DiContainer.php, src/Validation/Validator.php
Null-coalescing assignment replaces explicit initialization checks for cached instances and per-field arrays.
Object type checks
src/Asset/AssetManager.php, src/Config/Config.php, src/Lang/Adapters/FileAdapter.php
Checks for Asset and Data instances replace truthiness checks in the affected lookups and deletion.
Return types and Rector CI check
src/Environment/Helpers/env.php, src/HttpClient/Adapters/CurlAdapter.php, .github/workflows/php.yml, CHANGELOG.md
env() declares a mixed return type. CurlAdapter declares int and array return types. The PHP workflow runs composer rector:check, and the changelog records the updates.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Refactor

Suggested reviewers: andrey-smaelov

Merge Risk: ⚪ Minimal · up to cdb6a

The reviewed refactors preserve the observed behavior, and CI is configured to run the Rector check after PHPStan on PHP 8.0, 8.1, and 8.2. No actionable merge-blocking regression is evident; merge after normal CI checks pass.

Pre-merge checks: Passed 4 · Inconclusive 1

Failed Failed checks: 1 inconclusive

Check name Status Explanation Resolution
Linked Issues check Inconclusive Issue #587 requires the 22 Rector findings to be fixed and composer rector:check to run after PHPStan. The reviewed changes implement those requirements. The workflow matrix targets PHP 8.0, 8.1, an… Provide CI results for PHP 8.0, 8.1, and 8.2 that show the workflow is green, including successful composer test, composer cs:check, and composer phpstan runs.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies the two main changes: fixing Rector findings and adding rector:check to CI.
Out of Scope Changes check Passed The changed source files apply the Rector findings from issue #587. The workflow adds the requested Rector check after PHPStan. The changelog entry documents these changes. No unrelated project change…
Docstring Coverage Passed Docstring coverage is 93.10% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 22 files.
Full details: Out of Scope Changes check

Explanation

The changed source files apply the Rector findings from issue #587. The workflow adds the requested Rector check after PHPStan. The changelog entry documents these changes. No unrelated project change, new Rector set, or quantum-php/project workflow change is present in the reviewed scope.

Full details: Linked Issues check

Explanation

Issue #587 requires the 22 Rector findings to be fixed and composer rector:check to run after PHPStan. The reviewed changes implement those requirements. The workflow matrix targets PHP 8.0, 8.1, and 8.2. The PR reports only the Rector result. It does not provide results for composer test, composer cs:check, or composer phpstan, and it does not establish green CI for all three PHP versions.

Resolution

Provide CI results for PHP 8.0, 8.1, and 8.2 that show the workflow is green, including successful composer test, composer cs:check, and composer phpstan runs.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@andrey-smaelov andrey-smaelov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small style nit/suggestion for consistency:

In several factory methods you assign then return, e.g.

$this->instances[$adapter] ??= $this->createInstance($adapterClass, $adapter);
return $this->instances[$adapter];

You can simplify this to the equivalent one-liner:

return $this->instances[$adapter] ??= $this->createInstance($adapterClass, $adapter);

@armanist
armanist merged commit 59199a3 into quantum-php:master Oct 19, 2026
6 checks passed
@armanist
armanist deleted the issue/587-rector-check branch October 19, 2026 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix Rector findings and run rector:check in CI

3 participants