Data collection for profiler #26
Conversation
WalkthroughAdds tag-collection and profiling support: a decorator that accumulates cache tags, a Symfony data collector with profiler UI, a compiler pass to disable collectors when profiler is absent, cache-type identifier additions, internal CacheTags storage changes, and related tests and service registrations. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application / Controller
participant Collector as CacheTagCollectionResponseTagger
participant Inner as Inner ResponseTagger
participant Profiler as Symfony Profiler / DataCollector
App->>Collector: tag(CacheTags)
Collector->>Collector: merge/accumulate tags
Collector->>Inner: tag(CacheTags) -- delegate
Profiler->>Collector: lateCollect()
Collector->>Profiler: return collected tags
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
3 times, most recently
from
daa447c to
2531c6c
Compare
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
4 times, most recently
from
2623c3b to
71d2669
Compare
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
from
ec62f9d to
2dd244f
Compare
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
2 times, most recently
from
64d93e5 to
53e0f01
Compare
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
3 times, most recently
from
2f6f1a9 to
1ed56bf
Compare
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
from
1ed56bf to
32e1c23
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (1)
1-2: Fix strict_types declaration formatting.The strict_types declaration should be on the same line as the PHP opening tag to match the project's preferred format.
-<?php -declare(strict_types=1); +<?php declare(strict_types=1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
config/services.php(3 hunks)src/Cache/CacheTags.php(3 hunks)src/Cache/CacheType.php(1 hunks)src/Cache/CacheType/CustomCacheType.php(1 hunks)src/Cache/CacheType/ElementCacheType.php(1 hunks)src/Cache/CacheType/EmptyCacheType.php(1 hunks)src/Cache/DataCollector/CacheTagDataCollector.php(1 hunks)src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php(1 hunks)src/DependencyInjection/CompilerPass/DisableCacheTagCollectionPass.php(1 hunks)src/NeustaPimcoreHttpCacheBundle.php(2 hunks)templates/cache_tags.html.twig(1 hunks)tests/Integration/Tagging/CollectTagsDataTest.php(1 hunks)tests/Integration/Tagging/TagObjectTest.php(1 hunks)tests/Unit/Cache/DataCollector/CacheTagDataCollectorTest.php(1 hunks)tests/Unit/Cache/ResponseTagger/CacheTagCollectionResponseTaggerTest.php(1 hunks)tests/Unit/DependencyInjection/CompilerPass/DisableCacheTagCollectionPassTest.php(1 hunks)tests/app/config/packages/web_profiler.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
src/NeustaPimcoreHttpCacheBundle.php (3)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#17
File: tests/Integration/Helpers/ArrangeCacheTest.php:1-2
Timestamp: 2025-04-16T14:39:31.681Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict types declaration is to have it on the same line as the PHP opening tag: `<?php declare(strict_types=1);` rather than on a separate line.
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (1)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
src/DependencyInjection/CompilerPass/DisableCacheTagCollectionPass.php (3)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#17
File: tests/Integration/Helpers/ArrangeCacheTest.php:1-2
Timestamp: 2025-04-16T14:39:31.681Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict types declaration is to have it on the same line as the PHP opening tag: `<?php declare(strict_types=1);` rather than on a separate line.
tests/Unit/Cache/ResponseTagger/CacheTagCollectionResponseTaggerTest.php (4)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#14
File: tests/Integration/Invalidation/CancelInvalidationTest.php:29-29
Timestamp: 2025-04-16T09:17:07.113Z
Learning: When writing PHP test classes, all properties (like $client or mock objects) must be explicitly declared with proper type hints at the class level. Dynamic properties (properties assigned without declaration) are deprecated in PHP 8.2+ and will cause warnings.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#17
File: tests/Integration/Helpers/ArrangeCacheTest.php:1-2
Timestamp: 2025-04-16T14:39:31.681Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict types declaration is to have it on the same line as the PHP opening tag: `<?php declare(strict_types=1);` rather than on a separate line.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
config/services.php (2)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
tests/Unit/Cache/DataCollector/CacheTagDataCollectorTest.php (3)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#14
File: tests/Integration/Invalidation/CancelInvalidationTest.php:29-29
Timestamp: 2025-04-16T09:17:07.113Z
Learning: When writing PHP test classes, all properties (like $client or mock objects) must be explicitly declared with proper type hints at the class level. Dynamic properties (properties assigned without declaration) are deprecated in PHP 8.2+ and will cause warnings.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
src/Cache/CacheTags.php (1)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
tests/Unit/DependencyInjection/CompilerPass/DisableCacheTagCollectionPassTest.php (3)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#14
File: tests/Integration/Invalidation/CancelInvalidationTest.php:29-29
Timestamp: 2025-04-16T09:17:07.113Z
Learning: When writing PHP test classes, all properties (like $client or mock objects) must be explicitly declared with proper type hints at the class level. Dynamic properties (properties assigned without declaration) are deprecated in PHP 8.2+ and will cause warnings.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jdreesen
PR: teamneusta/pimcore-http-cache-bundle#13
File: tests/app/bin/console:4-4
Timestamp: 2025-04-01T13:40:08.587Z
Learning: The project uses `teamneusta/pimcore-testing-framework` (version ^0.12.8) in the require-dev section of composer.json, which provides the `Neusta\Pimcore\TestingFramework\Pimcore\BootstrapPimcore` class used in the test environment.
src/Cache/DataCollector/CacheTagDataCollector.php (1)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
tests/Integration/Tagging/CollectTagsDataTest.php (3)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#14
File: tests/Integration/Invalidation/CancelInvalidationTest.php:29-29
Timestamp: 2025-04-16T09:17:07.113Z
Learning: When writing PHP test classes, all properties (like $client or mock objects) must be explicitly declared with proper type hints at the class level. Dynamic properties (properties assigned without declaration) are deprecated in PHP 8.2+ and will cause warnings.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
🧬 Code Graph Analysis (7)
src/Cache/CacheType.php (3)
src/Cache/CacheType/ElementCacheType.php (1)
identifier(41-44)src/Cache/CacheType/EmptyCacheType.php (1)
identifier(24-27)src/Cache/CacheType/CustomCacheType.php (1)
identifier(33-36)
src/Cache/CacheType/EmptyCacheType.php (3)
src/Cache/CacheType/ElementCacheType.php (1)
identifier(41-44)src/Cache/CacheType/CustomCacheType.php (1)
identifier(33-36)src/Cache/CacheType.php (1)
identifier(13-13)
src/Cache/CacheType/ElementCacheType.php (3)
src/Cache/CacheType/EmptyCacheType.php (1)
identifier(24-27)src/Cache/CacheType/CustomCacheType.php (1)
identifier(33-36)src/Cache/CacheType.php (1)
identifier(13-13)
src/Cache/CacheType/CustomCacheType.php (3)
src/Cache/CacheType/ElementCacheType.php (1)
identifier(41-44)src/Cache/CacheType/EmptyCacheType.php (1)
identifier(24-27)src/Cache/CacheType.php (1)
identifier(13-13)
src/NeustaPimcoreHttpCacheBundle.php (1)
src/DependencyInjection/CompilerPass/DisableCacheTagCollectionPass.php (1)
DisableCacheTagCollectionPass(8-22)
tests/Unit/Cache/DataCollector/CacheTagDataCollectorTest.php (6)
src/Cache/CacheTag.php (1)
CacheTag(9-42)src/Cache/CacheTags.php (2)
CacheTags(10-106)fromString(30-33)src/Cache/CacheTypeFactory.php (2)
CacheTypeFactory(11-31)createFromString(18-25)src/Cache/DataCollector/CacheTagDataCollector.php (4)
CacheTagDataCollector(11-49)lateCollect(23-30)getTags(35-38)reset(45-48)src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (2)
CacheTagCollectionResponseTagger(9-24)tag(19-23)tests/Unit/Cache/ResponseTagger/CacheTagCollectionResponseTaggerTest.php (1)
setUp(22-26)
src/Cache/DataCollector/CacheTagDataCollector.php (2)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (3)
CacheTagCollectionResponseTagger(9-24)__construct(13-17)tag(19-23)src/Cache/CacheTags.php (2)
__construct(20-28)toString(97-100)
🪛 PHPMD (2.15.0)
src/Cache/DataCollector/CacheTagDataCollector.php
18-18: Avoid unused parameters such as '$request'. (Unused Code Rules)
(UnusedFormalParameter)
18-18: Avoid unused parameters such as '$response'. (Unused Code Rules)
(UnusedFormalParameter)
18-18: Avoid unused parameters such as '$exception'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (24)
tests/app/config/packages/web_profiler.yaml (1)
1-2: LGTM! Standard profiler configuration.The configuration correctly enables the Symfony profiler with both
enabledandcollectset totrue, which is necessary for the cache tag data collection functionality.src/Cache/CacheType/EmptyCacheType.php (1)
24-27: LGTM! Consistent interface implementation.The
identifier()method correctly implements the new interface requirement, returning an appropriate string identifier for the empty cache type.src/Cache/CacheType.php (1)
13-13: LGTM! Well-designed interface addition.The
identifier()method declaration is clean and provides a consistent way for all cache types to expose their string identifiers, which is essential for the cache tag collection and profiling functionality.src/Cache/CacheType/CustomCacheType.php (1)
33-36: LGTM! Efficient implementation reusing existing property.The
identifier()method correctly implements the interface by returning the validated$typeproperty, which is consistent with the existingtoString()method behavior.src/Cache/CacheType/ElementCacheType.php (1)
41-44: LGTM! Appropriate implementation using full enum value.The
identifier()method correctly returns the full enum value ($this->type->value) rather than just the first character liketoString()does, which provides more descriptive identification for profiling purposes.src/NeustaPimcoreHttpCacheBundle.php (1)
27-31: LGTM! Proper compiler pass registration.The
buildmethod correctly calls the parent method first and registers the compiler pass to conditionally disable cache tag collection when profiler is not available.tests/Integration/Tagging/TagObjectTest.php (1)
161-161: LGTM! Method name now accurately reflects the test scenario.The method name correction properly indicates that the test verifies the behavior when an object class is disabled in the configuration.
tests/Unit/Cache/ResponseTagger/CacheTagCollectionResponseTaggerTest.php (2)
17-21: LGTM! Proper property declarations following PHP 8.2+ best practices.All properties are explicitly declared with appropriate type hints, avoiding deprecated dynamic properties.
48-58: Verify forwarding uses the original tags argument
The unit test asserts thatinnerTagger->tag($tags)is called with the exact$tagspassed intoCacheTagCollectionResponseTagger::tag(). Please confirm that the implementation inCacheTagCollectionResponseTagger::tag()indeed does:$this->inner->tag($tags);rather than using an accumulated property (
$this->collectedTags) or other value.Locations to review:
- src/…/CacheTagCollectionResponseTagger.php –
public function tag(CacheTags $tags)
• Ensure the call to$this->inner->tag()uses$tags.- tests/Unit/Cache/ResponseTagger/CacheTagCollectionResponseTaggerTest.php – Lines 48–58
• If the implementation is intended to forward accumulated tags instead, update the test to expect$this->collectedTags.src/DependencyInjection/CompilerPass/DisableCacheTagCollectionPass.php (1)
8-22: LGTM! Proper compiler pass implementation.The compiler pass correctly implements the conditional logic to disable cache tag collection decoration when the profiler service is not available, following Symfony DI container patterns.
tests/Unit/Cache/DataCollector/CacheTagDataCollectorTest.php (1)
1-62: LGTM! Well-structured test class.The test class follows proper PHPUnit conventions with:
- Correct property declarations (addressing PHP 8.2+ requirements from learnings)
- Appropriate use of Prophecy for mocking
- Clear test scenarios covering both data collection and reset functionality
- Proper setup and teardown patterns
src/Cache/CacheTags.php (4)
13-15: LGTM! Appropriate visibility change for cache tag collection.The change from private to public readonly allows external access to the tags collection while maintaining immutability, which is necessary for the data collector functionality.
22-27: LGTM! Smart optimization with associative array storage.The change to use associative array indexed by tag string prevents duplicate tags and enables efficient lookups, which is beneficial for the cache tag collection feature.
61-61: LGTM! Correct iterator implementation.Using
array_values()ensures the iterator returnsCacheTagobjects in the correct order without exposing the internal associative array keys.
88-90: LGTM! Consistent array handling.The
toArray()method correctly usesarray_values()to maintain the expected list structure for string conversion.templates/cache_tags.html.twig (1)
1-82: LGTM! Comprehensive profiler template implementation.The template provides excellent user experience with:
- Proper conditional rendering based on tag presence
- Consistent visual styling with status colors
- Clear data presentation in both toolbar and detailed panel
- Appropriate empty state handling
- Good integration with Symfony profiler conventions
config/services.php (3)
14-16: LGTM! Proper import additions.The new imports are correctly added for the cache tag collection functionality.
57-59: LGTM! Correct decorator service configuration.The decorator service is properly configured with:
- Appropriate priority (1) to wrap the response tagger
- Correct service dependency injection
- Proper decoration pattern
95-101: LGTM! Proper data collector service configuration.The data collector service is correctly configured with:
- Appropriate dependency injection
- Correct
data_collectortag with template and priority- Proper integration with Symfony profiler
src/Cache/DataCollector/CacheTagDataCollector.php (3)
18-21: LGTM! Correct implementation for late data collection.The empty
collect()method with explanatory comment is appropriate since cache tags are added after the response is created, requiring the use oflateCollect().The static analysis warnings about unused parameters are false positives - these parameters are required by the
DataCollectorinterface.
23-30: LGTM! Proper cache tag data collection.The
lateCollect()method correctly:
- Accesses the public
collectedTags->tagsproperty- Extracts tag string and type identifier
- Stores data in the expected format for template consumption
32-48: LGTM! Complete data collector interface implementation.The getter, name, and reset methods are properly implemented:
getTags()returns correctly typed array with null coalescinggetName()returns appropriate collector identifierreset()properly clears collected datatests/Unit/DependencyInjection/CompilerPass/DisableCacheTagCollectionPassTest.php (1)
1-53: LGTM! Well-structured unit test following established patterns.The test class properly follows all established conventions:
- Correct strict_types declaration format
- Explicit property declarations with type hints
- Proper use of Prophecy for mocking
- Comprehensive test coverage for both scenarios (profiler enabled/disabled)
- Clear test method names and assertions
The test logic correctly verifies that the compiler pass conditionally disables cache tag collection based on profiler availability.
tests/Integration/Tagging/CollectTagsDataTest.php (1)
29-236: Comprehensive test coverage for cache tag collection scenarios.The test methods provide excellent coverage of various cache tag collection scenarios:
- Different element types (documents, objects, assets)
- Custom cache types via event listeners
- Conditional collection based on configuration
- Disabled scenarios for type, caching, object subtypes, and classes
The use of attributes for configuration and proper assertions makes the tests clear and maintainable.
Docstrings generation was requested by @jan888adams. * #26 (comment) The following files were modified: * `src/Cache/CacheTags.php` * `src/Cache/CacheType.php` * `src/Cache/CacheType/CustomCacheType.php` * `src/Cache/CacheType/ElementCacheType.php` * `src/Cache/CacheType/EmptyCacheType.php` * `src/Cache/DataCollector/CacheTagDataCollector.php` * `src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php` * `src/DependencyInjection/CompilerPass/DisableCacheTagCollectionPass.php` * `src/NeustaPimcoreHttpCacheBundle.php`
|
Note Generated docstrings for this pull request at #35 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
templates/cache_tags.html.twig (2)
12-18: Improve accessibility for SVG icons.The SVG icons lack proper accessibility attributes, making them less accessible to screen readers and assistive technologies.
Add
role="img"andaria-labelattributes to the SVG elements:-<svg viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" stroke="#EEE"> +<svg viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" stroke="#EEE" role="img" aria-label="Cache tags icon">Also applies to: 36-42
61-76: Enhance table accessibility.The data table could benefit from better accessibility attributes for screen readers.
Add table caption and improve header association:
<table class="table"> + <caption class="sr-only">Cache tags collected during this request</caption> <thead> <tr> - <th>Tag</th> - <th>Type</th> + <th scope="col">Tag</th> + <th scope="col">Type</th> </tr> </thead>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/cache_tags.html.twig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: PHPUnit with PHP 8.2 highest
- GitHub Check: PHPUnit with PHP 8.1 lowest
- GitHub Check: PHPUnit with PHP 8.1 highest
- GitHub Check: Quality Checks
🔇 Additional comments (3)
templates/cache_tags.html.twig (3)
1-31: Well-structured profiler toolbar implementation.The toolbar block correctly implements the Symfony profiler toolbar pattern with conditional status colors and proper integration with the profiler URL.
77-81: Clean empty state implementation.The empty state message is clear and user-friendly, providing appropriate feedback when no cache tags are present.
69-74: No issues with tag structure in Twig – data collector always provides both fields.The
CacheTagDataCollector’slateCollect()method explicitly populates$this->data['tags'][] = [ 'tag' => $tag->toString(), 'type' => $tag->type->identifier(), ];and its docblock declares
@return array<array{tag: string, type: string}>.
SincegetTags()returns this structure (or an empty array), the template’stag.tagandtag.typeaccesses are safe.
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
from
e10e290 to
23f0871
Compare
tests/Unit/DependencyInjection/CompilerPass/DisableCacheTagCollectionPassTest.php
Show resolved
Hide resolved
tests/Unit/DependencyInjection/CompilerPass/DisableCacheTagCollectionPassTest.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
templates/profiler.html.twig (2)
3-31: Consider extracting the SVG icon for better maintainability.The toolbar logic is correct and follows Symfony profiler conventions. However, the inline SVG (lines 12-18) is quite verbose and duplicated in the menu block.
Consider extracting the SVG to a separate Twig macro or using a simpler icon:
+{% macro cache_tag_icon() %} + <svg viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" stroke="#EEE"> + <!-- SVG content here --> + </svg> +{% endmacro %} {% set icon %} <span class="sf-toolbar-icon"> - <svg viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" stroke="#EEE"> - <!-- long SVG content --> - </svg> + {{ _self.cache_tag_icon() }} <span class="sf-toolbar-value">{{ collector.tags|length }}</span> </span> {% endset %}
33-47: Code duplication: SVG icon repeated from toolbar block.The menu block logic is correct, but the SVG icon (lines 36-42) is duplicated from the toolbar block, violating DRY principles.
Extract the SVG to a reusable Twig macro as suggested in the toolbar block review to eliminate this duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
config/services.php(3 hunks)src/DataCollector.php(1 hunks)src/DependencyInjection/CompilerPass/DisableDataCollectorPass.php(1 hunks)src/DependencyInjection/NeustaPimcoreHttpCacheExtension.php(1 hunks)src/NeustaPimcoreHttpCacheBundle.php(2 hunks)templates/profiler.html.twig(1 hunks)tests/Integration/Configuration/CollectConfigurationDataTest.php(1 hunks)tests/Integration/Tagging/CollectTagsDataTest.php(1 hunks)tests/Unit/CacheTagDataCollectorTest.php(1 hunks)tests/Unit/DependencyInjection/CompilerPass/DisableDataCollectorPassTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- config/services.php
- src/NeustaPimcoreHttpCacheBundle.php
- tests/Integration/Tagging/CollectTagsDataTest.php
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
src/DependencyInjection/NeustaPimcoreHttpCacheExtension.php (3)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
Learnt from: jan888adams
PR: #17
File: tests/Integration/Helpers/ArrangeCacheTest.php:1-2
Timestamp: 2025-04-16T14:39:31.681Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict types declaration is to have it on the same line as the PHP opening tag: <?php declare(strict_types=1); rather than on a separate line.
src/DependencyInjection/CompilerPass/DisableDataCollectorPass.php (3)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
Learnt from: jan888adams
PR: #17
File: tests/Integration/Helpers/ArrangeCacheTest.php:1-2
Timestamp: 2025-04-16T14:39:31.681Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict types declaration is to have it on the same line as the PHP opening tag: <?php declare(strict_types=1); rather than on a separate line.
tests/Unit/DependencyInjection/CompilerPass/DisableDataCollectorPassTest.php (3)
Learnt from: jan888adams
PR: #14
File: tests/Integration/Invalidation/CancelInvalidationTest.php:29-29
Timestamp: 2025-04-16T09:17:07.113Z
Learning: When writing PHP test classes, all properties (like $client or mock objects) must be explicitly declared with proper type hints at the class level. Dynamic properties (properties assigned without declaration) are deprecated in PHP 8.2+ and will cause warnings.
Learnt from: jdreesen
PR: #13
File: tests/app/bin/console:4-4
Timestamp: 2025-04-01T13:40:08.587Z
Learning: The project uses teamneusta/pimcore-testing-framework (version ^0.12.8) in the require-dev section of composer.json, which provides the Neusta\Pimcore\TestingFramework\Pimcore\BootstrapPimcore class used in the test environment.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
tests/Unit/CacheTagDataCollectorTest.php (4)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jdreesen
PR: #13
File: tests/app/bin/console:4-4
Timestamp: 2025-04-01T13:40:08.587Z
Learning: The project uses teamneusta/pimcore-testing-framework (version ^0.12.8) in the require-dev section of composer.json, which provides the Neusta\Pimcore\TestingFramework\Pimcore\BootstrapPimcore class used in the test environment.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
Learnt from: jan888adams
PR: #14
File: tests/Integration/Invalidation/CancelInvalidationTest.php:29-29
Timestamp: 2025-04-16T09:17:07.113Z
Learning: When writing PHP test classes, all properties (like $client or mock objects) must be explicitly declared with proper type hints at the class level. Dynamic properties (properties assigned without declaration) are deprecated in PHP 8.2+ and will cause warnings.
src/DataCollector.php (3)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
Learnt from: jan888adams
PR: #17
File: tests/Integration/Helpers/ArrangeCacheTest.php:1-2
Timestamp: 2025-04-16T14:39:31.681Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict types declaration is to have it on the same line as the PHP opening tag: <?php declare(strict_types=1); rather than on a separate line.
tests/Integration/Configuration/CollectConfigurationDataTest.php (4)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jdreesen
PR: #14
File: tests/Integration/ConfigurableWebTestcase.php:17-18
Timestamp: 2025-04-08T16:57:20.877Z
Learning: In the ConfigurableWebTestcase class, the $kernelConfigurations property should be documented as @var list<KernelConfiguration> to accurately reflect it's a sequential list of KernelConfiguration objects.
Learnt from: jdreesen
PR: #13
File: tests/app/bin/console:4-4
Timestamp: 2025-04-01T13:40:08.587Z
Learning: The project uses teamneusta/pimcore-testing-framework (version ^0.12.8) in the require-dev section of composer.json, which provides the Neusta\Pimcore\TestingFramework\Pimcore\BootstrapPimcore class used in the test environment.
Learnt from: jdreesen
PR: #14
File: tests/Integration/ConfigurableWebTestcase.php:17-18
Timestamp: 2025-04-08T16:53:53.232Z
Learning: In the ConfigurableWebTestcase class, the $kernelConfigurations property is an array of KernelConfiguration objects, not a single KernelConfiguration instance.
🧬 Code Graph Analysis (1)
src/DataCollector.php (1)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (1)
CacheTagCollectionResponseTagger(9-24)
🪛 PHPMD (2.15.0)
src/DataCollector.php
20-20: Avoid unused parameters such as '$request'. (Unused Code Rules)
(UnusedFormalParameter)
20-20: Avoid unused parameters such as '$response'. (Unused Code Rules)
(UnusedFormalParameter)
20-20: Avoid unused parameters such as '$exception'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHPUnit with PHP 8.1 lowest
- GitHub Check: PHPUnit with PHP 8.1 highest
- GitHub Check: PHPUnit with PHP 8.2 highest
🔇 Additional comments (9)
src/DependencyInjection/NeustaPimcoreHttpCacheExtension.php (1)
73-74: LGTM! Clean configuration parameter registration.The addition of the container parameter follows standard dependency injection patterns, making the elements configuration available to the data collector service.
src/DependencyInjection/CompilerPass/DisableDataCollectorPass.php (1)
8-17: LGTM! Effective conditional service registration.The compiler pass correctly implements conditional service removal based on profiler availability, preventing unnecessary overhead when profiling is disabled.
tests/Unit/CacheTagDataCollectorTest.php (1)
16-80: LGTM! Comprehensive unit test coverage.The test class effectively covers the core functionality of the DataCollector with proper mocking, clear assertions, and good separation of concerns across the three test methods.
src/DataCollector.php (1)
11-60: LGTM! Solid DataCollector implementation following Symfony patterns.The implementation correctly follows Symfony's data collector architecture:
- Configuration stored in
collect()phase- Tag data processed in
lateCollect()phase after request completion- Proper integration with
CacheTagCollectionResponseTagger- Clean accessor methods and reset functionality
Note: The PHPMD warnings about unused parameters in the
collect()method are false positives - these parameters are required by theDataCollectorInterface.templates/profiler.html.twig (2)
1-2: LGTM!Correctly extends the Symfony WebProfiler layout following standard conventions.
49-99: LGTM! Well-structured profiler panel implementation.The panel block correctly implements:
- Conditional rendering based on data availability
- Proper use of Symfony profiler CSS classes
- Clear presentation of both summary and detailed information
- Configuration data display with appropriate JSON encoding
The structure follows Symfony profiler conventions and provides comprehensive debugging information.
tests/Integration/Configuration/CollectConfigurationDataTest.php (3)
1-30: LGTM! Proper test class structure following project conventions.The class correctly:
- Uses the standardized strict_types declaration format
- Implements appropriate test configuration via attributes
- Utilizes proper testing framework traits and setup
- Follows the project's integration testing patterns
32-55: LGTM! Comprehensive test for configuration data collection.The test properly:
- Configures profiler to enable data collection
- Sets up test data and makes realistic HTTP request
- Verifies correct collector type and configuration data
- Follows AAA testing pattern with clear assertions
57-75: LGTM! Proper negative test case for disabled profiler.The test correctly:
- Configures profiler as disabled to test the negative scenario
- Uses consistent test setup pattern
- Verifies that no profile data is collected when profiler is disabled
- Validates the compiler pass behavior for disabling data collection
tests/Unit/DependencyInjection/CompilerPass/DisableDataCollectorPassTest.php
Show resolved
Hide resolved
tests/Unit/DependencyInjection/CompilerPass/DisableDataCollectorPassTest.php
Show resolved
Hide resolved
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
from
44b560e to
015166e
Compare
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
from
015166e to
e287845
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/DataCollector.php (2)
21-24: Empty collect(): suppress unused-parameter warningsThe method must exist to satisfy the abstract contract, but the unused parameters trigger PHPMD noise. Prefix them with underscores or add an ignore/suppress attribute to make the intent explicit.
- public function collect(Request $request, Response $response, ?\Throwable $exception = null): void - { - } + /** @inheritDoc */ + public function collect(Request $_, Response $__ , ?\Throwable $___ = null): void + { + // intentionally left blank – collection happens in lateCollect() + }
18-19: Pre-initialise the tags storageSeeding
$this->data['tags']in the constructor avoids undefined-index checks later and makes intent clearer.) { $this->data['configuration'] = $this->configuration; + $this->data['tags'] = []; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
config/services.php(3 hunks)src/DataCollector.php(1 hunks)src/DependencyInjection/CompilerPass/DisableDataCollectorPass.php(1 hunks)src/DependencyInjection/NeustaPimcoreHttpCacheExtension.php(1 hunks)src/NeustaPimcoreHttpCacheBundle.php(2 hunks)templates/profiler.html.twig(1 hunks)tests/Integration/Configuration/CollectConfigurationDataTest.php(1 hunks)tests/Integration/Tagging/CollectTagsDataTest.php(1 hunks)tests/Unit/CacheTagDataCollectorTest.php(1 hunks)tests/Unit/DependencyInjection/CompilerPass/DisableDataCollectorPassTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/Integration/Configuration/CollectConfigurationDataTest.php
🚧 Files skipped from review as they are similar to previous changes (8)
- src/DependencyInjection/NeustaPimcoreHttpCacheExtension.php
- src/NeustaPimcoreHttpCacheBundle.php
- src/DependencyInjection/CompilerPass/DisableDataCollectorPass.php
- tests/Unit/CacheTagDataCollectorTest.php
- tests/Unit/DependencyInjection/CompilerPass/DisableDataCollectorPassTest.php
- config/services.php
- templates/profiler.html.twig
- tests/Integration/Tagging/CollectTagsDataTest.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
src/DataCollector.php (3)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
Learnt from: jan888adams
PR: #17
File: tests/Integration/Helpers/ArrangeCacheTest.php:1-2
Timestamp: 2025-04-16T14:39:31.681Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict types declaration is to have it on the same line as the PHP opening tag: <?php declare(strict_types=1); rather than on a separate line.
🧬 Code Graph Analysis (1)
src/DataCollector.php (1)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (1)
CacheTagCollectionResponseTagger(9-24)
🪛 PHPMD (2.15.0)
src/DataCollector.php
21-21: Avoid unused parameters such as '$request'. (Unused Code Rules)
(UnusedFormalParameter)
21-21: Avoid unused parameters such as '$response'. (Unused Code Rules)
(UnusedFormalParameter)
21-21: Avoid unused parameters such as '$exception'. (Unused Code Rules)
(UnusedFormalParameter)
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
2 times, most recently
from
6d599ad to
a3f09e4
Compare
tests/Unit/DependencyInjection/CompilerPass/DisableDataCollectorPassTest.php
Outdated
Show resolved
Hide resolved
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
from
a3f09e4 to
5de7513
Compare
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
from
5de7513 to
94125fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/DataCollector.php (1)
23-25: Emptycollect()→ silence PHPMD or store trivial data
collect()intentionally does nothing (late-collector pattern) but the unused parameters trigger PHPMD warnings. Either:
- Add a suppress-warning annotation
- Or explicitly unset the parameters.
+/** @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function collect(Request $request, Response $response, ?\Throwable $exception = null): void { -} + // Intentionally left blank – all work happens in lateCollect(). +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DataCollector.php(1 hunks)tests/Unit/CacheTagDataCollectorTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/CacheTagDataCollectorTest.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
src/DataCollector.php (2)
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
🧬 Code Graph Analysis (1)
src/DataCollector.php (1)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (1)
CacheTagCollectionResponseTagger(9-24)
🪛 PHPMD (2.15.0)
src/DataCollector.php
23-23: Avoid unused parameters such as '$request'. (Unused Code Rules)
(UnusedFormalParameter)
23-23: Avoid unused parameters such as '$response'. (Unused Code Rules)
(UnusedFormalParameter)
23-23: Avoid unused parameters such as '$exception'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHPUnit with PHP 8.1 highest
- GitHub Check: PHPUnit with PHP 8.2 highest
- GitHub Check: PHPUnit with PHP 8.1 lowest
Co-authored-by: Jacob Dreesen <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (1)
11-11: Consider encapsulation by making the property private.The
$collectedTagsproperty is public, which breaks encapsulation and allows external code to modify the internal state directly. Consider making it private and providing a getter method if external access is needed.- public CacheTags $collectedTags; + private CacheTags $collectedTags; + + public function getCollectedTags(): CacheTags + { + return $this->collectedTags; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php(1 hunks)src/DataCollector.php(1 hunks)tests/Unit/Cache/ResponseTagger/CacheTagCollectionResponseTaggerTest.php(1 hunks)tests/Unit/CacheTagDataCollectorTest.php(1 hunks)tests/Unit/DependencyInjection/CompilerPass/DisableDataCollectorPassTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Unit/DependencyInjection/CompilerPass/DisableDataCollectorPassTest.php
- tests/Unit/CacheTagDataCollectorTest.php
- tests/Unit/Cache/ResponseTagger/CacheTagCollectionResponseTaggerTest.php
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
📚 Learning: the pr #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all php files in the re...
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Applied to files:
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.phpsrc/DataCollector.php
📚 Learning: in the teamneusta/pimcore-http-cache-bundle project, a pr was created to ensure consistent formattin...
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
Applied to files:
src/DataCollector.php
📚 Learning: in the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict t...
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#17
File: tests/Integration/Helpers/ArrangeCacheTest.php:1-2
Timestamp: 2025-04-16T14:39:31.681Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict types declaration is to have it on the same line as the PHP opening tag: `<?php declare(strict_types=1);` rather than on a separate line.
Applied to files:
src/DataCollector.php
🧬 Code Graph Analysis (1)
src/DataCollector.php (1)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (4)
CacheTagCollectionResponseTagger(9-29)__construct(13-17)tag(19-23)reset(25-28)
🪛 PHPMD (2.15.0)
src/DataCollector.php
23-23: Avoid unused parameters such as '$request'. (Unused Code Rules)
(UnusedFormalParameter)
23-23: Avoid unused parameters such as '$response'. (Unused Code Rules)
(UnusedFormalParameter)
23-23: Avoid unused parameters such as '$exception'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHPUnit with PHP 8.1 lowest
- GitHub Check: PHPUnit with PHP 8.1 highest
- GitHub Check: PHPUnit with PHP 8.2 highest
🔇 Additional comments (6)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (2)
13-17: LGTM!The constructor properly uses dependency injection with readonly modifier and correctly initializes the collected tags to an empty state.
19-28: LGTM!The tag forwarding logic has been correctly implemented based on previous feedback - only new tags are forwarded to the inner tagger while still accumulating all tags for collection. The reset method properly clears the accumulated state.
src/DataCollector.php (4)
1-11: LGTM!The class structure follows Symfony data collector conventions properly, with correct strict_types formatting and appropriate use of
LateDataCollectorInterfacefor collecting data after response preparation.
13-21: LGTM!The constructor properly handles dependency injection and configuration storage. The optional configuration parameter with default empty array is well implemented.
23-35: LGTM!The collect method is correctly empty since tag collection is deferred to
lateCollect(). ThelateCollect()method properly extracts and stores tag data. The unused parameters incollect()are required by the interface signature and can be safely ignored.
37-62: LGTM!The getter methods are properly defensive with null coalescing. The
reset()method correctly clears both the data collector's internal state and calls reset on the tagger to prevent tag accumulation across requests, addressing previous architectural concerns.
jan888adams
changed the title
jan888adams
force-pushed
the
display-tags-in-symfony-profiler
branch
from
4666b5f to
a2b7d26
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Unit/DataCollectorTest.php (1)
47-62: Fix method name typoThe test logic is correct and properly verifies late collection of cache tags. However, there's a minor typo in the method name.
- public function lateCollect_collect_tag_data(): void + public function lateCollect_collects_tag_data(): void
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php(1 hunks)src/DataCollector.php(1 hunks)tests/Unit/DataCollectorTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#26
File: src/DataCollector.php:30-35
Timestamp: 2025-08-06T08:34:49.797Z
Learning: In the teamneusta/pimcore-http-cache-bundle, the CacheTags class uses an associative array indexed by tag string (array<string, CacheTag>) which automatically deduplicates cache tags when the same tag is added multiple times within the same collection.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
📚 Learning: in the teamneusta/pimcore-http-cache-bundle, the cachetags class uses an associative array indexed b...
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#26
File: src/DataCollector.php:30-35
Timestamp: 2025-08-06T08:34:49.797Z
Learning: In the teamneusta/pimcore-http-cache-bundle, the CacheTags class uses an associative array indexed by tag string (array<string, CacheTag>) which automatically deduplicates cache tags when the same tag is added multiple times within the same collection.
Applied to files:
tests/Unit/DataCollectorTest.phpsrc/DataCollector.php
📚 Learning: the pr #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all php files in the re...
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: The PR #18 in teamneusta/pimcore-http-cache-bundle successfully standardized all PHP files in the repository to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);), except for configuration files (.php-cs-fixer.php and config/services.php) which intentionally omit strict_types declarations.
Applied to files:
tests/Unit/DataCollectorTest.phpsrc/DataCollector.php
📚 Learning: the project uses `teamneusta/pimcore-testing-framework` (version ^0.12.8) in the require-dev section...
Learnt from: jdreesen
PR: teamneusta/pimcore-http-cache-bundle#13
File: tests/app/bin/console:4-4
Timestamp: 2025-04-01T13:40:08.587Z
Learning: The project uses `teamneusta/pimcore-testing-framework` (version ^0.12.8) in the require-dev section of composer.json, which provides the `Neusta\Pimcore\TestingFramework\Pimcore\BootstrapPimcore` class used in the test environment.
Applied to files:
tests/Unit/DataCollectorTest.php
📚 Learning: when writing php test classes, all properties (like $client or mock objects) must be explicitly decl...
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#14
File: tests/Integration/Invalidation/CancelInvalidationTest.php:29-29
Timestamp: 2025-04-16T09:17:07.113Z
Learning: When writing PHP test classes, all properties (like $client or mock objects) must be explicitly declared with proper type hints at the class level. Dynamic properties (properties assigned without declaration) are deprecated in PHP 8.2+ and will cause warnings.
Applied to files:
tests/Unit/DataCollectorTest.php
📚 Learning: in the teamneusta/pimcore-http-cache-bundle project, a pr was created to ensure consistent formattin...
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#0
File: :0-0
Timestamp: 2025-04-17T08:05:45.864Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, a PR was created to ensure consistent formatting of strict_types declarations across the codebase, with the preferred format being to have the strict_types declaration on the same line as the PHP opening tag (<?php declare(strict_types=1);).
Applied to files:
src/DataCollector.php
📚 Learning: in the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict t...
Learnt from: jan888adams
PR: teamneusta/pimcore-http-cache-bundle#17
File: tests/Integration/Helpers/ArrangeCacheTest.php:1-2
Timestamp: 2025-04-16T14:39:31.681Z
Learning: In the teamneusta/pimcore-http-cache-bundle project, the preferred formatting style for the strict types declaration is to have it on the same line as the PHP opening tag: `<?php declare(strict_types=1);` rather than on a separate line.
Applied to files:
src/DataCollector.php
🧬 Code Graph Analysis (2)
tests/Unit/DataCollectorTest.php (5)
src/Cache/CacheTag.php (1)
CacheTag(9-42)src/Cache/CacheTags.php (1)
CacheTags(10-106)src/Cache/CacheTypeFactory.php (2)
CacheTypeFactory(11-31)createFromString(18-25)src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (3)
CacheTagCollectionResponseTagger(10-30)tag(20-24)reset(26-29)src/DataCollector.php (6)
DataCollector(11-62)collect(23-26)getConfiguration(48-51)lateCollect(28-35)getTags(40-43)reset(58-61)
src/DataCollector.php (1)
src/Cache/ResponseTagger/CacheTagCollectionResponseTagger.php (4)
CacheTagCollectionResponseTagger(10-30)__construct(14-18)tag(20-24)reset(26-29)
🪛 PHPMD (2.15.0)
src/DataCollector.php
23-23: Avoid unused parameters such as '$request'. (Unused Code Rules)
(UnusedFormalParameter)
23-23: Avoid unused parameters such as '$response'. (Unused Code Rules)
(UnusedFormalParameter)
23-23: Avoid unused parameters such as '$exception'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHPUnit with PHP 8.2 highest
- GitHub Check: PHPUnit with PHP 8.1 lowest
- GitHub Check: PHPUnit with PHP 8.1 highest
🔇 Additional comments (9)
tests/Unit/DataCollectorTest.php (3)
16-32: LGTM - Well-structured test class with proper setupThe test class follows excellent practices with proper property declarations, appropriate use of ProphecyTrait for mocking, and a clean setUp method that correctly initializes the test dependencies.
34-45: LGTM - Configuration collection test is correctThe test properly verifies that configuration data is stored during construction and retrievable via
getConfiguration(). The test data structure aligns with the bundle's element configuration format.
64-78: LGTM - Reset functionality test is comprehensiveThe test properly verifies that the
reset()method clears collected tags while maintaining proper test isolation by setting up the initial state before testing the reset behavior.src/DataCollector.php (6)
13-21: LGTM - Constructor properly initializes static configurationThe constructor correctly stores the configuration immediately since it's static data that doesn't change between requests. The dependency injection and type hints are well-implemented.
23-26: LGTM - Intentionally empty collect method is correctThe empty implementation is appropriate since all data collection is deferred to
lateCollect(). The static analysis warnings about unused parameters are false positives - these parameters are required by the interface contract.
28-35: LGTM - Late collection correctly processes cache tagsThe implementation properly iterates through collected tags and stores them in the expected format. The cross-request accumulation concerns from previous reviews have been addressed since Symfony automatically calls
reset()after each request.
37-51: LGTM - Getter methods are well-implementedBoth getter methods properly use null coalescing for defensive programming and have accurate return type annotations that match their actual return values.
53-56: LGTM - Data collector name is appropriateThe name clearly identifies this as the Pimcore HTTP cache data collector and aligns with the service configuration.
58-61: Reset method is sufficient for current architectureThe current implementation correctly clears the DataCollector's internal tag storage. While previous reviews suggested also calling
reset()on theCacheTagCollectionResponseTagger, the service implementsResetInterfaceand Symfony's profiler system will handle callingreset()on all resettable services automatically.
Co-authored-by: Jacob Dreesen <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Chores