feat: rise phpstan level 8 and resolve errors #2165

shakaran · 2025-12-13T00:26:43Z

This pull request introduces several improvements and bug fixes across the codebase, focusing on stricter static analysis, improved null handling, and better type safety. The main changes include raising the PHPStan analysis level, adding type and null checks to prevent runtime errors, and improving documentation for generics-related issues.

Static analysis and type safety improvements:

Increased PHPStan static analysis level from 7 to 8 in phpstan.neon.dist for stricter code quality checks.
Added @phpstan-ignore missingType.generics annotations throughout src/DependencyInjection/Configuration.php to suppress generics-related warnings and clarify intent.
Improved PHPDoc for the $params argument in provideIncompatibleDriverOptions to specify expected structure, aiding static analysis tools.

Null handling and bug fixes:

Added null checks when accessing bundle metadata in DoctrineExtension.php and when importing DOM nodes in XMLSchemaTest.php to prevent potential errors.
Added a null check for the profile object in ProfilerController.php to return a 404 response if not found, avoiding errors on missing profiles.
Ensured the container is not null before shutdown logic in DoctrineBundle.php.
Improved constructor in DoctrineDataCollector.php to always provide a valid DebugDataHolder, preventing null-related issues.

Other improvements:

Added a null check for middleware class in MiddlewaresPass.php to avoid calling is_subclass_of on null.
Cast the result of preg_replace_callback to string in DoctrineExtension.php for stricter type safety.

greg0ire · 2025-12-13T09:04:12Z

Analysis of Commit Practice:

The commit titled "fix: run phpbcf in changes" represents a suboptimal development pattern that should be avoided in your workflow.

Root Cause Identification:

This type of commit indicates that code formatting corrections (phpbcf) were applied in a subsequent commit rather than being integrated into the original code changes. This approach creates interdependencies between commits that compromise version control integrity.

Impact Assessment:

Atomic Commit Principle Violation: Each commit should represent a complete, functional unit of work
Revert Capability Degradation: Individual commits cannot be safely reverted without breaking the build
History Navigation Complexity: Tools like git bisect become unreliable when commits have external dependencies

Recommended Resolution Strategy:

Implement a pre-commit validation workflow where all automated checks (formatting, linting, type checking) are executed and resolved prior to commit creation. Remedial formatting changes should be squashed or amended into their associated feature commits to maintain a clean, independently-revertable commit history.

Expected Outcome:

A linear commit history where each individual commit maintains build integrity and can be cherry-picked, reverted, or bisected without dependency conflicts.

greg0ire · 2025-12-13T09:16:32Z

src/DataCollector/DoctrineDataCollector.php

    ) {
+        if ($debugDataHolder === null) {
+            $debugDataHolder = new DebugDataHolder();
+        }


Contextual Analysis Gap:
The rationale behind commit 0c50581 introducing null-permissive behavior for the $debugDataHolder parameter remains unclear from the available context. Without understanding the original design intent and use case requirements that necessitated this nullable implementation, it is challenging to conduct a proper risk assessment regarding whether the proposed modification represents an appropriate solution or potentially introduces regression scenarios.

Request for Additional Context:
To make an informed evaluation of this change, please provide:

The business logic or technical requirements that motivated the original null-allowance implementation

Existing usage patterns where $debugDataHolder is intentionally passed as null

Any documented edge cases or conditional flows that rely on this nullable behavior

Current Assessment Status:
Evaluation pending until sufficient historical context and architectural understanding can be established.

I see that the logic seems that it is used for enviroments as production where profiling is not required, so it is feasible get as null since could or not exist with nullOnInvalid().

But the parent constructor (DoctrineDataCollector in symfony/doctrine-bridge)

parent::__construct($registry, $debugDataHolder);

Doesn't wait a nullable $debugDataHolder, which was the error warning showed by PHPStan when I run the phpstan at level 8.

At vendor/symfony/doctrine-bridge/DataCollector/DoctrineDataCollector.php

In this case, it seems a mandatory param in parent constructor. So unless that the modification for allow nullable would be made in symfony/doctrine-bridge, I dont know how to proceed here, but I am open to suggestions

Historical Context Investigation - Findings:
Utilizing advanced Git history analysis techniques (specifically git log -S pickaxe functionality or git log -L line history tracking) reveals the relevant implementation history for this parameter.

Source Reference:
The nullable parameter design originates from: symfony/symfony#45491

Historical Rationale Analysis:
Based on the pull request context, the nullable parameter was introduced as a backward compatibility bridge during the migration phase between two distinct logging architectures:

DBAL 2 Architecture: Legacy logging implementation

DBAL 3 Architecture: Modern middleware-based logging approach
The nullable type annotation served as a transitional mechanism, allowing codebases to gradually migrate between these paradigms without immediate breaking changes.

Current State Assessment:
Given that the bundle has officially deprecated and removed DBAL 2 support, the backward compatibility constraint that necessitated the nullable parameter no longer applies. Therefore, removing the nullable annotation from DebugDataHolder represents a logical architectural evolution that:

Simplifies the type contract

Eliminates unnecessary null-state handling

Aligns with the current supported DBAL version requirements

Clarification Request:
You mentioned that this nullable parameter relates to production environment considerations. Could you provide additional context or documentation supporting this assertion? The historical evidence suggests the nullable design was motivated by version migration compatibility rather than environment-specific behavior. Understanding the basis for the production environment connection would help ensure we're not overlooking an important use case or deployment scenario.

In production environments, profiling and debugging features are typically disabled for performance optimizations, so debug services are not registered (aka services related to DebugDataHolder may not be registered in the dependency container in production).

This nullability which is introduced for backward compatibility during DBAL 2→3.

But I dont know how do you want to avoid the nullality in DBAL 3

Since in dbal.php the codebase has:

service('doctrine.debug_data_holder')->nullOnInvalid()

It always allows inject the null state. Would make sense avoid nullOnInvalid() since is intentionally designed to handle the scenario where the debug data holder service is not registered in the container?

If I remove that it will not allows disable profiling for performance in production

So removing

if ($debugDataHolder === null) { $debugDataHolder = new DebugDataHolder(); }

it still will inject the null via nullOnInvalid() even using DBAL 3 only, and in consecuence we have the phpstan error, unless that you want remove or ignore the phpstan error.

Updated in eed58af

Architectural Inconsistency Question: 🤔
Your assertion states:

"In production environments, profiling and debugging features are typically disabled for performance optimizations, so debug services are not registered (aka services related to DebugDataHolder may not be registered in the dependency container in production)."

Critical Logic Gap Identified: 🔍
If the rationale for making DebugDataHolder nullable is based on production environments not registering debug-related services, this raises an important question:
Why is DoctrineDataCollector being instantiated in production environments at all? 🚨
Analysis: 💡
Data collectors are inherently debugging/profiling components that serve no functional purpose in production deployments. The typical Symfony architecture pattern dictates:

Development/Debug Mode: Data collectors are registered, collect profiling information, and populate the Web Debug Toolbar/Profiler 🛠️

Production Mode: Data collectors are not registered in the service container at all, eliminating overhead entirely ⚡
Logical Contradiction: ⚠️
If DoctrineDataCollector exists and is executing in production (necessitating nullable DebugDataHolder handling), this suggests one of the following scenarios:

Service Registration Misconfiguration: The data collector is incorrectly registered in production environments when it should be conditionally excluded

Misunderstood Architecture: The assumption about production behavior may be incorrect, and debug services ARE actually registered

Legacy Compatibility Issue: There may be deployment configurations that intentionally enable collectors in production (non-standard but possible)

Request for Clarification: 🎯
Could you provide evidence or configuration examples demonstrating scenarios where DoctrineDataCollector is instantiated in production while DebugDataHolder remains unregistered? Understanding this edge case is essential to determining whether null-handling is architecturally sound or symptomatic of a service registration configuration issue that should be addressed differently.

src/DependencyInjection/Compiler/MiddlewaresPass.php

src/DependencyInjection/DoctrineExtension.php

src/Twig/DoctrineExtension.php

src/DoctrineBundle.php

tests/DependencyInjection/XMLSchemaTest.php

Add explicit null check in XMLSchemaTest incorporating PHPBCF changes. This feature enhances test robustness and meets PHPStan level 8 requirements.

Correct the type hint for connection parameters in DropDatabaseDoctrineTest. This test update ensures accurate type checking and complies with PHPStan level 8.

Add explicit null check in XMLSchemaTest and apply code formatting. This feature improves test reliability and code quality for PHPStan level 8.

Specify phpstan-symfony 2.0.9 and phpstan 2.1.13 to support class.NotFound errors. This fix ensures proper error resolution and compatibility with PHPStan level 8.

shakaran · 2025-12-13T14:01:37Z

Analysis of Commit Practice:

The commit titled "fix: run phpbcf in changes" represents a suboptimal development pattern that should be avoided in your workflow.

Root Cause Identification:

This type of commit indicates that code formatting corrections (phpbcf) were applied in a subsequent commit rather than being integrated into the original code changes. This approach creates interdependencies between commits that compromise version control integrity.

Impact Assessment:

Atomic Commit Principle Violation: Each commit should represent a complete, functional unit of work

Revert Capability Degradation: Individual commits cannot be safely reverted without breaking the build

History Navigation Complexity: Tools like git bisect become unreliable when commits have external dependencies

Recommended Resolution Strategy:

Implement a pre-commit validation workflow where all automated checks (formatting, linting, type checking) are executed and resolved prior to commit creation. Remedial formatting changes should be squashed or amended into their associated feature commits to maintain a clean, independently-revertable commit history.

Expected Outcome:

A linear commit history where each individual commit maintains build integrity and can be cherry-picked, reverted, or bisected without dependency conflicts.

ok, squashed and removed from history push

greg0ire · 2025-12-13T15:08:13Z

Continuous Integration Failure Notification:
The current pull request has triggered failures in one or more CI pipeline jobs. These failures must be resolved before the changes can be merged into the main branch.

Resolution Guidance:
To address the failing checks, please consult the official contribution documentation provided by the Doctrine project:
Resource: Doctrine Project - Dealing with Checks and Tools

This guide provides comprehensive instructions on:

Interpreting CI Failure Output: Understanding which specific checks have failed and why
Local Reproduction: Running the same validation tools locally to diagnose issues before pushing
Toolchain Usage: Proper invocation of linters, formatters, static analyzers, and test suites
Remediation Procedures: Step-by-step workflows for resolving common CI failure scenarios

Next Steps:

Review the CI job logs to identify which specific checks are failing
Consult the linked documentation for tool-specific remediation guidance
Apply necessary fixes locally and validate using the same toolchain
Push corrected commits that satisfy all CI requirements

Objective:
Ensure all automated quality gates pass successfully, maintaining the project's code quality standards and enabling seamless integration of your contributions.

greg0ire · 2025-12-13T18:41:59Z

Commit Message Convention Violation: 🚫
Please remove semantic prefixes (e.g., feat:, fix:, chore:) from your commit messages. This project does not adhere to the Conventional Commits specification, and such prefixes are inconsistent with the established commit history style. 📝

Additional Issue - Incorrect Prefix Semantics: ⚠️
Beyond the stylistic inconsistency, the applied prefix demonstrates a misunderstanding of semantic commit conventions. Specifically:

Observed: A commit affecting only test files was prefixed with feat: 🔍
Problem: The feat: prefix is reserved for commits that introduce new functionality visible to end users ❌
Correct Classification: Test-only changes should be categorized as test: (if using semantic prefixes) or simply described without prefixes, as they represent internal quality improvements rather than user-facing features ✅

Recommended Action: 🎯

Review the project's existing commit history to understand the preferred message format and style 📚
Rewrite commit messages to align with project conventions (typically imperative mood without categorical prefixes) ✏️
Ensure commit messages accurately describe the purpose of changes rather than mechanically categorizing them 💡

Reference Documentation: 📖
For comprehensive guidance on crafting commit messages that align with project standards, please consult:
Doctrine Project - Crafting Meaningful Commit Messages

Expected Outcome: 🏆
Commit messages that integrate seamlessly with the project's historical style and accurately communicate the nature and intent of changes to future contributors reviewing the Git history. ✨

Add explicit checks for preg_replace_callback return value and PCRE errors. This change enhances error handling and satisfies PHPStan level 8 type safety.

Replace silent null container check with LogicException. This ensures explicit error handling and addresses PHPStan level 8 requirements.

Replace defensive null checks with explicit LogicException throws. This refactoring improves code clarity and meets PHPStan level 8 standards for error handling.

Replace conditional null check with LogicException when bundle metadata is missing. This change enforces proper error handling and complies with PHPStan level 8.

Replace silent null continuation with an explicit exception when a middleware service lacks a class definition. This improves error reporting and satisfies PHPStan level 8 type requirements.

src/Twig/DoctrineExtension.php

greg0ire · 2025-12-14T10:32:21Z

Continuous Integration Pipeline Failure - Recurrence: 🚨
The CI pipeline is experiencing failures again. To prevent repeated push-revert cycles and maintain code quality standards, please implement a pre-push Git hook as recommended in the project's contribution guidelines.
Root Cause: 🔍
Code is being pushed to the remote repository without local validation, resulting in CI failures that could have been detected and resolved before the push operation. This creates unnecessary pipeline load and delays the review process.
Recommended Solution - Pre-Push Hook Implementation: 🛠️
Configure a Git pre-push hook that automatically executes the same validation checks used by the CI pipeline before allowing code to be pushed to the remote repository. This ensures:

Early Error Detection: Issues are caught locally before reaching CI infrastructure ⚡
Faster Feedback Loop: Immediate validation results rather than waiting for remote pipeline execution 🔄
Reduced CI Load: Only validated code reaches the remote repository, preserving CI resources 💪
Improved Developer Experience: Fix issues locally with full IDE/debugging support available 🎯
Implementation Guidance: 📖
Detailed instructions for setting up pre-push hooks and other automated quality checks are available at:
Doctrine Project - Dealing with Checks and Tools
Expected Outcome: ✅
All commits pushed to the remote repository pass validation checks on the first attempt, eliminating the need for follow-up fix commits and maintaining a clean, functional commit history. 🏆

greg0ire · 2025-12-14T10:39:10Z

Contributing Guidelines Compliance Issue: 📋
It appears the project's contributing guidelines have not been fully reviewed or followed. Specifically, the commit message format deviates from the standards outlined in the official documentation.

Reference Documentation: 📖
Doctrine Project - Crafting Meaningful Commit Messages

Identified Violations: ❌

Missing Commit Message Body: All commits lack a body section that provides detailed context, rationale, and impact description
Subject Line Length Exceeded: Multiple commit subjects exceed the 50-character recommended limit, reducing readability in Git history views and tooling
Expected Commit Message Structure: ✅
According to the contributing guidelines, properly formatted commit messages should include:

Subject Line (≤50 characters): Concise imperative-mood summary of the change
Blank Line: Separator between subject and body
Body (wrapped at 72 characters): Detailed explanation including:
- Why the change was necessary (motivation and context) 💡
- What problem it solves or functionality it adds 🎯
- How it impacts the codebase or end users (if applicable) 📊
- References to related issues, PRs, or documentation 🔗

Recommended Action: 🛠️

Review the complete contributing guidelines at the provided URL 📚
Rewrite existing commit messages using git rebase -i to comply with the format specification ✏️
For future commits, ensure both subject length constraints and body content requirements are satisfied before pushing 🚀

Expected Outcome: 🏆
Commit messages that provide comprehensive historical context for future maintainers, integrate seamlessly with project tooling, and demonstrate adherence to established contribution standards. Well-crafted commit messages are essential for long-term project maintainability and collaborative development efficiency. ✨

Restore the DoctrineDataCollector to allow null DebugDataHolder. This change fixes type handling issues and ensures compatibility with PHPStan level 8.

Remove ignore directives since phpstan/symfony 2.0.9 resolves class.NotFound errors. This update cleans up the code and aligns with the latest PHPStan capabilities.

Handle the case where a parameter can be null due to Symfony container's nullOnInvalid() behavior. This change adds proper null handling to prevent errors and meet PHPStan level 8 standards.

@var

Add @var type annotation for stdClass ReflectionClass to satisfy PHPStan level 8. This annotation provides explicit type information, improving static type checking and code quality.

This commit reorders the null check in DoctrineExtension.php to improve error handling. By performing the null check earlier in the code flow, it enhances robustness and complies with PHPStan level 8 requirements for type safety.

shakaran · 2025-12-14T11:17:41Z

Contributing Guidelines Compliance Issue: 📋 It appears the project's contributing guidelines have not been fully reviewed or followed. Specifically, the commit message format deviates from the standards outlined in the official documentation.

Reference Documentation: 📖 Doctrine Project - Crafting Meaningful Commit Messages

Identified Violations: ❌

Missing Commit Message Body: All commits lack a body section that provides detailed context, rationale, and impact description

Subject Line Length Exceeded: Multiple commit subjects exceed the 50-character recommended limit, reducing readability in Git history views and tooling
Expected Commit Message Structure: ✅
According to the contributing guidelines, properly formatted commit messages should include:

Subject Line (≤50 characters): Concise imperative-mood summary of the change

Blank Line: Separator between subject and body

Body (wrapped at 72 characters): Detailed explanation including:

Why the change was necessary (motivation and context) 💡

What problem it solves or functionality it adds 🎯

How it impacts the codebase or end users (if applicable) 📊

References to related issues, PRs, or documentation 🔗

Recommended Action: 🛠️

Review the complete contributing guidelines at the provided URL 📚

Rewrite existing commit messages using git rebase -i to comply with the format specification ✏️

For future commits, ensure both subject length constraints and body content requirements are satisfied before pushing 🚀

Expected Outcome: 🏆 Commit messages that provide comprehensive historical context for future maintainers, integrate seamlessly with project tooling, and demonstrate adherence to established contribution standards. Well-crafted commit messages are essential for long-term project maintainability and collaborative development efficiency. ✨

Fixed

shakaran · 2025-12-14T11:18:09Z

Contributing Guidelines Compliance Issue: 📋 It appears the project's contributing guidelines have not been fully reviewed or followed. Specifically, the commit message format deviates from the standards outlined in the official documentation.

Reference Documentation: 📖 Doctrine Project - Crafting Meaningful Commit Messages

Identified Violations: ❌

Missing Commit Message Body: All commits lack a body section that provides detailed context, rationale, and impact description

Subject Line Length Exceeded: Multiple commit subjects exceed the 50-character recommended limit, reducing readability in Git history views and tooling
Expected Commit Message Structure: ✅
According to the contributing guidelines, properly formatted commit messages should include:

Subject Line (≤50 characters): Concise imperative-mood summary of the change

Blank Line: Separator between subject and body

Body (wrapped at 72 characters): Detailed explanation including:

Why the change was necessary (motivation and context) 💡

What problem it solves or functionality it adds 🎯

How it impacts the codebase or end users (if applicable) 📊

References to related issues, PRs, or documentation 🔗

Recommended Action: 🛠️

Review the complete contributing guidelines at the provided URL 📚

Rewrite existing commit messages using git rebase -i to comply with the format specification ✏️

For future commits, ensure both subject length constraints and body content requirements are satisfied before pushing 🚀

Expected Outcome: 🏆 Commit messages that provide comprehensive historical context for future maintainers, integrate seamlessly with project tooling, and demonstrate adherence to established contribution standards. Well-crafted commit messages are essential for long-term project maintainability and collaborative development efficiency. ✨

Fixed

shakaran · 2025-12-14T11:37:26Z

Suggestion: Could you mark as "resolved" in items or conversations that are solved to you? I can do by myself, but I dont know if it ok for you. I have a lot threads in this issue and it is being difficult to follow every reply opened. Thanks

greg0ire · 2025-12-14T12:17:04Z

Suggestion: Could you mark as "resolved" in items or conversations that are solved to you? I can do by myself, but I dont know if it ok for you. I have a lot threads in this issue and it is being difficult to follow every reply opened. Thanks

Sure! Although, are you sure the issue is only the number of threads, and not also maybe how verbose and lengthy they are? Would you agree to say that it might be better if everyone dialed a bit back from delegating so much to AI?

shakaran · 2025-12-14T12:58:18Z

Suggestion: Could you mark as "resolved" in items or conversations that are solved to you? I can do by myself, but I dont know if it ok for you. I have a lot threads in this issue and it is being difficult to follow every reply opened. Thanks

Sure! Although, are you sure the issue is only the number of threads, and not also maybe how verbose and lengthy they are? Would you agree to say that it might be better if everyone dialed a bit back from delegating so much to AI?

Actually at some points in the review, your verbose in some replies helps me to understand better the problem because is more detailed and deep. For new contributors as me or people just wacthing and learning, maybe it is even useful

In my side, I just use "github summary" button in the description at the moment of open the PR thinking that it was more clear to show the changes, which it seems not very good for the maintaners and generate a kind of conflict, then you want continue with the lesson overreacting with more verbose and lenghty replies to teach me probably about that. And I have patient about that.

I see your point since in your side you are having everyday a lot PR to review in a lot projects and probably it is a lot boilerplate to you and extra consuming time, so I will not use the github summary button in generate descriptions for PR for this project and I will try to be more direct. At the end is only "show me the code" who speaks better than descriptions.

greg0ire · 2025-12-14T13:47:08Z

A little description doesn't hurt, but if you find yourself (or your LLM) mentioning filenames, it's probably too much detail.

I've seen you've updated your commit messages, but as per the guide, please wrap the commit message bodies at 72 chars, so that it looks good when running git log.

-Add @var type annotation for stdClass ReflectionClass to satisfy PHPStan level 8. This annotation provides explicit type information, improving static type checking and code quality.
+Add @var type annotation for stdClass ReflectionClass to satisfy PHPStan
+level 8. This annotation provides explicit type information, improving static type
+checking and code quality.

greg0ire · 2025-12-14T13:48:58Z

Some of your commit need to be squashed together: nobody needs to know you ordered the preg_last_error() and null checks wrong during your first attempt.

ostrolucky · 2025-12-15T21:58:33Z

Thank you for going through review loops here @greg0ire, I didn't have energy so soon again. Don't worry about commits though, we squash merge PRs in these cases