feat: improve shopware exception ignoring#28
Conversation
WalkthroughA new event-driven mechanism for filtering exceptions before Sentry reporting has been introduced. This includes a new event class, an event subscriber for filtering based on log level, configuration loading enhancements, and updates to service registration. Exception ignore logic is now handled via event dispatching and configuration rather than hardcoded arrays. Changes
Sequence Diagram(s)sequenceDiagram
participant Sentry as Sentry Event Processor
participant UseIgnores as UseShopwareExceptionIgnores
participant Dispatcher as EventDispatcher
participant FilterEvent as FilterExceptionEvent
participant Subscriber as FilterShopwareExceptionIgnoresSubscriber
Sentry->>UseIgnores: Process event with exceptions
UseIgnores->>Dispatcher: Dispatch FilterExceptionEvent(exceptions)
Dispatcher->>Subscriber: filterSentryExceptions(event)
Subscriber->>FilterEvent: Filter exceptions by config (log level)
Subscriber-->>Dispatcher: Updated exceptions in event
Dispatcher-->>UseIgnores: FilterExceptionEvent (filtered)
UseIgnores-->>Sentry: Return event with filtered exceptions
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/Integration/FilterExceptionEvent.php (2)
10-10: Remove unnecessary blank line.-class FilterExceptionEvent extends Event -{ - +class FilterExceptionEvent extends Event +{
30-32: Remove unnecessary blank lines.- } - - - -} + } +}src/Integration/UseShopwareExceptionIgnores.php (1)
22-22: Simplify return type declaration.The anonymous function always returns an Event object, never null.
- Scope::addGlobalEventProcessor(static function (Event $event) use ($eventDispatcher): ?Event { + Scope::addGlobalEventProcessor(static function (Event $event) use ($eventDispatcher): Event {🧰 Tools
🪛 GitHub Check: phpstan
[failure] 22-22:
Anonymous function never returns null so it can be removed from the return type.🪛 PHPStan (2.1.15)
22-22: Anonymous function never returns null so it can be removed from the return type.
(return.unusedType)
src/ShopwareSentryBundle.php (1)
75-88: Improve error handling in configuration loading.The configuration loading method looks good but could benefit from better error handling to prevent silent failures during configuration loading.
Consider adding error handling to the configuration loading:
private function loadConfiguration(ContainerBuilder $container): void { $locator = new FileLocator('Resources/config'); $resolver = new LoaderResolver([ new YamlFileLoader($container, $locator), new GlobFileLoader($container, $locator), new DirectoryLoader($container, $locator), ]); $configLoader = new DelegatingLoader($resolver); $confDir = \rtrim($this->getPath(), '/') . '/Resources/config'; - $configLoader->load($confDir . '/{packages}/*.yaml', 'glob'); + + $configPattern = $confDir . '/{packages}/*.yaml'; + if (!\is_dir(\dirname($configPattern))) { + return; // No config directory, skip loading + } + + $configLoader->load($configPattern, 'glob'); }This adds a safety check to ensure the configuration directory exists before attempting to load files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Integration/FilterExceptionEvent.php(1 hunks)src/Integration/UseShopwareExceptionIgnores.php(1 hunks)src/Resources/config/packages/sentry.yaml(1 hunks)src/ShopwareSentryBundle.php(3 hunks)src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/Integration/FilterExceptionEvent.php (2)
src/Integration/UseShopwareExceptionIgnores.php (1)
__construct(12-16)src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php (1)
__construct(11-15)
src/Integration/UseShopwareExceptionIgnores.php (1)
src/Integration/FilterExceptionEvent.php (4)
__construct(14-18)getExceptions(20-23)FilterExceptionEvent(8-32)setExceptions(25-28)
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php (2)
src/Integration/FilterExceptionEvent.php (4)
FilterExceptionEvent(8-32)__construct(14-18)getExceptions(20-23)setExceptions(25-28)src/Integration/UseShopwareExceptionIgnores.php (1)
__construct(12-16)
🪛 GitHub Check: phpstan
src/Integration/FilterExceptionEvent.php
[failure] 25-25:
Method Frosh\SentryBundle\Integration\FilterExceptionEvent::setExceptions() has parameter $exceptions with no value type specified in iterable type array.
[failure] 20-20:
Method Frosh\SentryBundle\Integration\FilterExceptionEvent::getExceptions() return type has no value type specified in iterable type array.
src/Integration/UseShopwareExceptionIgnores.php
[failure] 22-22:
Anonymous function never returns null so it can be removed from the return type.
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php
[failure] 25-25:
Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::filterSentryExceptions() has no return type specified.
[failure] 11-11:
Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
🪛 PHPStan (2.1.15)
src/Integration/FilterExceptionEvent.php
20-20: Method Frosh\SentryBundle\Integration\FilterExceptionEvent::getExceptions() return type has no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
25-25: Method Frosh\SentryBundle\Integration\FilterExceptionEvent::setExceptions() has parameter $exceptions with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
27-27: Property Frosh\SentryBundle\Integration\FilterExceptionEvent::$exceptions (array<Sentry\ExceptionDataBag>) does not accept array.
(assign.propertyType)
src/Integration/UseShopwareExceptionIgnores.php
22-22: Anonymous function never returns null so it can be removed from the return type.
(return.unusedType)
28-28: Parameter #1 $exceptions of method Sentry\Event::setExceptions() expects array<Sentry\ExceptionDataBag>, array given.
(argument.type)
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php
11-11: Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
25-25: Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::filterSentryExceptions() has no return type specified.
(missingType.return)
30-30: Cannot access offset 'log_level' on mixed.
(offsetAccess.nonOffsetAccessible)
30-30: Cannot call method getType() on mixed.
(method.nonObject)
🪛 GitHub Actions: PHP
src/Integration/FilterExceptionEvent.php
[error] 7-21: PHP CS Fixer formatting issue: constructor formatting should be compacted to one line with trailing comma.
src/Integration/UseShopwareExceptionIgnores.php
[error] 11-19: PHP CS Fixer formatting issue: constructor formatting should be compacted to one line with trailing comma.
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php
[error] 7-23: PHP CS Fixer formatting issue: constructor formatting should be compacted to one line with trailing comma; missing comma in array return.
src/ShopwareSentryBundle.php
[error] 26-26: PHP CS Fixer formatting issue: extraneous blank line detected inside class declaration.
🔇 Additional comments (4)
src/Resources/config/packages/sentry.yaml (1)
1-8: LGTM!The configuration structure is well-formed and the exception classes listed are appropriate for Sentry exclusion.
src/ShopwareSentryBundle.php (3)
32-32: Good integration of configuration loading.The configuration loading is properly called at the beginning of the build method, ensuring that configuration parameters are available when services are registered.
50-50:Details
✅ Verification successful
Verify event_dispatcher service dependency.
The service registration has been updated to use the event_dispatcher service instead of the previous exceptions parameter. This aligns with the new event-driven architecture.
Verify that the event_dispatcher service is available in the Shopware context:
🏁 Script executed:
#!/bin/bash # Description: Verify that event_dispatcher service is commonly available in Symfony/Shopware applications # Expected: Find references to event_dispatcher service registration or usage # Search for event_dispatcher service references rg -A 3 -B 3 "event_dispatcher" --type phpLength of output: 461
No changes needed:
event_dispatcheris provided by Symfony/Shopware by default
Theevent_dispatcherservice is registered out of the box in Symfony’s DI container (and therefore in Shopware). No additional configuration is required.
58-62:Details
✅ Verification successful
Verify configuration parameter availability.
The new subscriber service is registered with the
%frosh_sentry.exclude_exceptions%parameter. Ensure this parameter is properly defined in the loaded configuration.Verify that the configuration parameter is properly defined:
🏁 Script executed:
#!/bin/bash # Description: Verify that frosh_sentry.exclude_exceptions parameter is defined in configuration files # Expected: Find the parameter definition in YAML configuration files # Search for the parameter definition rg -A 5 -B 5 "frosh_sentry\.exclude_exceptions|exclude_exceptions" --type yamlLength of output: 80
🏁 Script executed:
#!/bin/bash # Comprehensive search for the exclude_exceptions parameter definition or usage rg -n 'exclude_exceptions|excludeExceptions'Length of output: 285
Configuration parameter is defined via the compiler pass
The
frosh_sentry.exclude_exceptionsparameter is set insrc/CompilerPass/ExceptionConfigCompilerPass.php:17, so it will be available when registering the subscriber inShopwareSentryBundle. No further changes are needed.
| $exceptions = $eventDispatcher->dispatch(new FilterExceptionEvent($exceptions))->getExceptions(); | ||
| $event->setExceptions($exceptions); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure type safety when setting exceptions.
The static analysis indicates a type mismatch. While the code will work at runtime, it's better to ensure proper typing.
The getExceptions() method returns an array that should contain ExceptionDataBag objects. Let me verify the return type declaration:
🏁 Script executed:
#!/bin/bash
# Description: Check the return type of FilterExceptionEvent::getExceptions()
# Search for the getExceptions method definition
ast-grep --pattern $'class FilterExceptionEvent {
$$$
public function getExceptions(): $_
$$$
}'Length of output: 103
Let’s locate the actual FilterExceptionEvent class and its getExceptions() signature:
🏁 Script executed:
#!/bin/bash
# 1. Find where FilterExceptionEvent is defined
rg "class FilterExceptionEvent" -n .
# 2. Find all occurrences of getExceptions to see its return type
rg "function getExceptions" -n .Length of output: 248
Annotate exception array types for static analysis
Static analysis warns that getExceptions() returns a plain array, which makes downstream assignments to ExceptionDataBag[] untyped. Add PHPDoc annotations so tools like PHPStan/Psalm know this array holds ExceptionDataBag instances.
• src/Integration/FilterExceptionEvent.php
– Line 20: add a PHPDoc @return ExceptionDataBag[] above getExceptions(): array.
– Line 24: add a PHPDoc @param ExceptionDataBag[] $exceptions above setExceptions(array $exceptions): void.
• src/Integration/UseShopwareExceptionIgnores.php
– Line 27 (just before assignment): annotate the local variable as /** @var ExceptionDataBag[] $exceptions */.
Example diff:
--- a/src/Integration/FilterExceptionEvent.php
+++ b/src/Integration/FilterExceptionEvent.php
@@
- public function getExceptions(): array
+ /**
+ * @return ExceptionDataBag[]
+ */
+ public function getExceptions(): array
@@
- public function setExceptions(array $exceptions): void
+ /**
+ * @param ExceptionDataBag[] $exceptions
+ */
+ public function setExceptions(array $exceptions): void--- a/src/Integration/UseShopwareExceptionIgnores.php
+++ b/src/Integration/UseShopwareExceptionIgnores.php
@@ -25,6 +25,7 @@
// …
- $exceptions = $eventDispatcher->dispatch(new FilterExceptionEvent($exceptions))->getExceptions();
+ /** @var ExceptionDataBag[] $exceptions */
+ $exceptions = $eventDispatcher->dispatch(new FilterExceptionEvent($exceptions))->getExceptions();
$event->setExceptions($exceptions);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $exceptions = $eventDispatcher->dispatch(new FilterExceptionEvent($exceptions))->getExceptions(); | |
| $event->setExceptions($exceptions); | |
| // … | |
| /** @var ExceptionDataBag[] $exceptions */ | |
| $exceptions = $eventDispatcher->dispatch(new FilterExceptionEvent($exceptions))->getExceptions(); | |
| $event->setExceptions($exceptions); |
🧰 Tools
🪛 PHPStan (2.1.15)
28-28: Parameter #1 $exceptions of method Sentry\Event::setExceptions() expects array<Sentry\ExceptionDataBag>, array given.
(argument.type)
🤖 Prompt for AI Agents
In src/Integration/UseShopwareExceptionIgnores.php around lines 27 to 28, the
variable $exceptions assigned from getExceptions() lacks explicit type
annotation, causing static analysis warnings about type safety. To fix this, add
a PHPDoc comment just before the assignment to annotate $exceptions as an array
of ExceptionDataBag objects (/** @var ExceptionDataBag[] $exceptions */). This
will ensure proper typing for static analysis tools and maintain type safety.
| public function filterSentryExceptions(FilterExceptionEvent $event) | ||
| { | ||
| $filteredExceptions = []; | ||
| foreach ($event->getExceptions() as $exception) { | ||
|
|
||
| $exceptionLogLevel = $this->shopwareExceptionConfig[$exception->getType()]['log_level'] ?? null; | ||
|
|
||
| if ($exceptionLogLevel === 'notice') { | ||
| continue; | ||
| } | ||
|
|
||
| $filteredExceptions[] = $exception; | ||
| } | ||
|
|
||
| $event->setExceptions($filteredExceptions); | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
Add return type and improve type safety in filterSentryExceptions method.
The method has several issues that need to be addressed:
- Missing return type specification
- Type safety issues when accessing array offsets and calling methods on mixed types
- Potential null pointer exceptions
Apply this diff to fix the return type and improve type safety:
- public function filterSentryExceptions(FilterExceptionEvent $event)
+ public function filterSentryExceptions(FilterExceptionEvent $event): void
{
$filteredExceptions = [];
foreach ($event->getExceptions() as $exception) {
+ $exceptionType = $exception->getType();
+ if (!isset($this->shopwareExceptionConfig[$exceptionType])) {
+ $filteredExceptions[] = $exception;
+ continue;
+ }
- $exceptionLogLevel = $this->shopwareExceptionConfig[$exception->getType()]['log_level'] ?? null;
+ $exceptionLogLevel = $this->shopwareExceptionConfig[$exceptionType]['log_level'] ?? null;
if ($exceptionLogLevel === 'notice') {
continue;
}
$filteredExceptions[] = $exception;
}
$event->setExceptions($filteredExceptions);
-
-
}This change:
- Adds the missing
voidreturn type - Improves type safety by checking if the exception type exists in config
- Removes trailing whitespace
- Caches the exception type to avoid multiple method calls
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function filterSentryExceptions(FilterExceptionEvent $event) | |
| { | |
| $filteredExceptions = []; | |
| foreach ($event->getExceptions() as $exception) { | |
| $exceptionLogLevel = $this->shopwareExceptionConfig[$exception->getType()]['log_level'] ?? null; | |
| if ($exceptionLogLevel === 'notice') { | |
| continue; | |
| } | |
| $filteredExceptions[] = $exception; | |
| } | |
| $event->setExceptions($filteredExceptions); | |
| } | |
| public function filterSentryExceptions(FilterExceptionEvent $event): void | |
| { | |
| $filteredExceptions = []; | |
| foreach ($event->getExceptions() as $exception) { | |
| $exceptionType = $exception->getType(); | |
| if (!isset($this->shopwareExceptionConfig[$exceptionType])) { | |
| $filteredExceptions[] = $exception; | |
| continue; | |
| } | |
| $exceptionLogLevel = $this->shopwareExceptionConfig[$exceptionType]['log_level'] ?? null; | |
| if ($exceptionLogLevel === 'notice') { | |
| continue; | |
| } | |
| $filteredExceptions[] = $exception; | |
| } | |
| $event->setExceptions($filteredExceptions); | |
| } |
🧰 Tools
🪛 GitHub Check: phpstan
[failure] 25-25:
Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::filterSentryExceptions() has no return type specified.
🪛 PHPStan (2.1.15)
25-25: Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::filterSentryExceptions() has no return type specified.
(missingType.return)
30-30: Cannot access offset 'log_level' on mixed.
(offsetAccess.nonOffsetAccessible)
30-30: Cannot call method getType() on mixed.
(method.nonObject)
🤖 Prompt for AI Agents
In src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php around lines 25
to 42, the filterSentryExceptions method lacks a return type and has type safety
issues when accessing the shopwareExceptionConfig array and calling methods on
exceptions. Fix this by adding a void return type to the method signature,
caching the exception type in a variable before using it to avoid multiple
method calls, and checking if the exception type key exists in the config array
before accessing its 'log_level' to prevent null pointer exceptions. Also,
remove any trailing whitespace in the method.
| public function __construct( | ||
| private readonly array $shopwareExceptionConfig | ||
| ) | ||
| { | ||
| } |
There was a problem hiding this comment.
Fix constructor parameter type annotation and formatting.
The constructor has multiple issues that need to be addressed:
- Missing proper type annotation for the array parameter
- Formatting doesn't comply with PHP CS Fixer standards
Apply this diff to fix the type annotation and formatting:
- public function __construct(
- private readonly array $shopwareExceptionConfig
- )
- {
- }
+ public function __construct(private readonly array $shopwareExceptionConfig)
+ {
+ }For better type safety, consider adding a more specific type annotation:
- public function __construct(private readonly array $shopwareExceptionConfig)
+ /**
+ * @param array<string, array{log_level?: string}> $shopwareExceptionConfig
+ */
+ public function __construct(private readonly array $shopwareExceptionConfig)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function __construct( | |
| private readonly array $shopwareExceptionConfig | |
| ) | |
| { | |
| } | |
| /** | |
| * @param array<string, array{log_level?: string}> $shopwareExceptionConfig | |
| */ | |
| public function __construct(private readonly array $shopwareExceptionConfig) | |
| { | |
| } |
🧰 Tools
🪛 GitHub Check: phpstan
[failure] 11-11:
Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
🪛 PHPStan (2.1.15)
11-11: Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
🤖 Prompt for AI Agents
In src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php around lines 11
to 15, the constructor parameter lacks a proper type annotation and the
formatting does not comply with PHP CS Fixer standards. Fix this by explicitly
typing the parameter as an array with a more specific type if possible (e.g.,
array of strings or a defined type), and adjust the formatting to match PSR-12
standards, ensuring correct indentation, spacing, and line breaks in the
constructor signature and body.
schneider-felix
force-pushed
the
feat/improve-exception-ignore
branch
from
f32699f to
a01065e
Compare
| sentry: | ||
| options: | ||
| ignore_exceptions: | ||
| - 'Shopware\Storefront\Framework\Routing\Exception\SalesChannelMappingException' | ||
| - 'Shopware\Core\Framework\Api\Controller\Exception\AuthThrottledException' | ||
| - 'Shopware\Core\Framework\RateLimiter\Exception\RateLimitExceededException' | ||
| - 'Symfony\Component\Console\Exception\CommandNotFoundException' | ||
| - 'League\OAuth2\Server\Exception\OAuthServerException' |
There was a problem hiding this comment.
Not sure... feels weird to configure a third party bundle here. What do you think?
There was a problem hiding this comment.
Yeah but probably there is no other possibility as we cannot really influence League\OAuth2 ? Sounds legit to me.
schneider-felix
force-pushed
the
feat/improve-exception-ignore
branch
from
a01065e to
90812ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php (3)
11-13: Fix constructor formatting to comply with PHP CS Fixer standards.The constructor should be formatted on a single line according to the pipeline failure.
Apply this diff to fix the formatting:
- public function __construct(private readonly array $shopwareExceptionConfig) - { - } + public function __construct(private readonly array $shopwareExceptionConfig) + { + }🧰 Tools
🪛 GitHub Check: phpstan
[failure] 11-11:
Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.🪛 PHPStan (2.1.15)
11-11: Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type(missingType.iterableValue)
11-11: 🛠️ Refactor suggestionAdd type annotation for array parameter to improve type safety.
The static analysis indicates that the array parameter lacks a specific value type specification. This affects type safety and static analysis accuracy.
Add a PHPDoc annotation to specify the array structure:
+ /** + * @param array<string, array{log_level?: string}> $shopwareExceptionConfig + */ public function __construct(private readonly array $shopwareExceptionConfig)🧰 Tools
🪛 GitHub Check: phpstan
[failure] 11-11:
Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.🪛 PHPStan (2.1.15)
11-11: Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type(missingType.iterableValue)
26-28: 🛠️ Refactor suggestionImprove type safety when accessing exception configuration.
The current code has potential type safety issues when accessing array offsets and calling methods on mixed types. This could lead to runtime errors if the exception type doesn't exist in the configuration.
Apply this diff to improve type safety:
foreach ($event->getExceptions() as $exception) { + $exceptionType = $exception->getType(); + if (!isset($this->shopwareExceptionConfig[$exceptionType])) { + $filteredExceptions[] = $exception; + continue; + } - $exceptionLogLevel = $this->shopwareExceptionConfig[$exception->getType()]['log_level'] ?? null; + $exceptionLogLevel = $this->shopwareExceptionConfig[$exceptionType]['log_level'] ?? null;🧰 Tools
🪛 PHPStan (2.1.15)
28-28: Cannot access offset 'log_level' on mixed.
(offsetAccess.nonOffsetAccessible)
src/Integration/UseShopwareExceptionIgnores.php (1)
12-14: Fix constructor formatting and remove trailing comma.The constructor formatting doesn't comply with PHP CS Fixer standards and has an unnecessary trailing comma.
Apply this diff to fix the formatting:
- public function __construct(private readonly EventDispatcherInterface $eventDispatcher,) - { - } + public function __construct(private readonly EventDispatcherInterface $eventDispatcher) + { + }
🧹 Nitpick comments (2)
src/Integration/UseShopwareExceptionIgnores.php (2)
20-20: Remove unused null return type from anonymous function.The static analysis correctly identifies that the anonymous function never returns null, so the nullable return type is unnecessary.
Apply this diff to clean up the return type:
- Scope::addGlobalEventProcessor(static function (Event $event) use ($eventDispatcher): ?Event { + Scope::addGlobalEventProcessor(static function (Event $event) use ($eventDispatcher): Event {🧰 Tools
🪛 GitHub Check: phpstan
[failure] 20-20:
Anonymous function never returns null so it can be removed from the return type.🪛 PHPStan (2.1.15)
20-20: Anonymous function never returns null so it can be removed from the return type.
(return.unusedType)
25-26: Consider adding type annotation for better static analysis.While the code works correctly, adding a type annotation would help static analysis tools understand the exception array type.
Consider adding a PHPDoc annotation before the assignment:
+ /** @var ExceptionDataBag[] $exceptions */ $exceptions = $eventDispatcher->dispatch(new FilterExceptionEvent($exceptions))->getExceptions();This helps static analysis tools understand that
$exceptionscontainsExceptionDataBagobjects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Integration/FilterExceptionEvent.php(1 hunks)src/Integration/UseShopwareExceptionIgnores.php(1 hunks)src/Resources/config/packages/sentry.yaml(1 hunks)src/ShopwareSentryBundle.php(4 hunks)src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Resources/config/packages/sentry.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Integration/FilterExceptionEvent.php
- src/ShopwareSentryBundle.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Integration/UseShopwareExceptionIgnores.php (1)
src/Integration/FilterExceptionEvent.php (4)
__construct(14-16)getExceptions(21-24)FilterExceptionEvent(8-36)setExceptions(29-32)
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php (2)
src/Integration/FilterExceptionEvent.php (4)
FilterExceptionEvent(8-36)__construct(14-16)getExceptions(21-24)setExceptions(29-32)src/Integration/UseShopwareExceptionIgnores.php (1)
__construct(12-14)
🪛 GitHub Check: phpstan
src/Integration/UseShopwareExceptionIgnores.php
[failure] 20-20:
Anonymous function never returns null so it can be removed from the return type.
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php
[failure] 11-11:
Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
🪛 PHPStan (2.1.15)
src/Integration/UseShopwareExceptionIgnores.php
20-20: Anonymous function never returns null so it can be removed from the return type.
(return.unusedType)
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php
11-11: Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
28-28: Cannot access offset 'log_level' on mixed.
(offsetAccess.nonOffsetAccessible)
🪛 GitHub Actions: PHP
src/Integration/UseShopwareExceptionIgnores.php
[error] 9-17: PHP CS Fixer formatting issue: constructor should be formatted as 'public function __construct(private readonly EventDispatcherInterface $eventDispatcher) {}'.
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php
[error] 7-17: PHP CS Fixer formatting issue: constructor should be formatted as 'public function __construct(private readonly array $shopwareExceptionConfig) {}'.
🔇 Additional comments (2)
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php (1)
23-40: Logic implementation looks solid.The event filtering logic correctly:
- Iterates through all exceptions from the event
- Checks the configured log level for each exception type
- Excludes exceptions with 'notice' log level from Sentry reporting
- Updates the event with the filtered exception list
This provides a clean separation of concerns for exception filtering.
🧰 Tools
🪛 PHPStan (2.1.15)
28-28: Cannot access offset 'log_level' on mixed.
(offsetAccess.nonOffsetAccessible)
src/Integration/UseShopwareExceptionIgnores.php (1)
16-30: Excellent architectural improvement to event-driven exception filtering.The refactoring successfully:
- Replaces hardcoded exception filtering with flexible event dispatching
- Enables external subscribers to modify exception filtering logic
- Maintains clean separation of concerns
- Integrates well with Symfony's event dispatcher pattern
This approach makes the exception filtering more maintainable and extensible.
🧰 Tools
🪛 GitHub Check: phpstan
[failure] 20-20:
Anonymous function never returns null so it can be removed from the return type.🪛 PHPStan (2.1.15)
20-20: Anonymous function never returns null so it can be removed from the return type.
(return.unusedType)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php (2)
11-13: Fix constructor formatting and add proper type annotation.The constructor has formatting and type annotation issues that need to be addressed based on pipeline failures and static analysis.
Based on past review comments and current pipeline failures, apply this diff to fix both issues:
- public function __construct(private readonly array $shopwareExceptionConfig) - { - } + /** + * @param array<string, array{log_level?: string}> $shopwareExceptionConfig + */ + public function __construct(private readonly array $shopwareExceptionConfig) + { + }🧰 Tools
🪛 GitHub Check: phpstan
[failure] 11-11:
Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.🪛 PHPStan (2.1.15)
11-11: Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type(missingType.iterableValue)
26-28: Improve type safety when accessing exception configuration.The current implementation has type safety issues when accessing the configuration array and calling methods on potentially mixed types.
Apply this diff to improve type safety:
foreach ($event->getExceptions() as $exception) { + $exceptionType = $exception->getType(); + if (!isset($this->shopwareExceptionConfig[$exceptionType])) { + $filteredExceptions[] = $exception; + continue; + } - $exceptionLogLevel = $this->shopwareExceptionConfig[$exception->getType()]['log_level'] ?? null; + $exceptionLogLevel = $this->shopwareExceptionConfig[$exceptionType]['log_level'] ?? null;This change:
- Caches the exception type to avoid multiple method calls
- Checks if the exception type exists in config before accessing it
- Prevents potential offset access errors on mixed types
🧰 Tools
🪛 PHPStan (2.1.15)
28-28: Cannot access offset 'log_level' on mixed.
(offsetAccess.nonOffsetAccessible)
src/Integration/UseShopwareExceptionIgnores.php (1)
12-14: Fix constructor formatting to comply with PHP CS Fixer.The constructor formatting doesn't comply with PHP CS Fixer standards based on pipeline failures.
Apply this diff to fix the formatting:
- public function __construct(private readonly EventDispatcherInterface $eventDispatcher) - { - } + public function __construct(private readonly EventDispatcherInterface $eventDispatcher) + { + }
🧹 Nitpick comments (1)
src/Integration/UseShopwareExceptionIgnores.php (1)
20-20: Remove unnecessary null from return type annotation.The anonymous function never returns null, so the nullable return type is unnecessary.
Apply this diff to clean up the return type:
- Scope::addGlobalEventProcessor(static function (Event $event) use ($eventDispatcher): ?Event { + Scope::addGlobalEventProcessor(static function (Event $event) use ($eventDispatcher): Event {🧰 Tools
🪛 GitHub Check: phpstan
[failure] 20-20:
Anonymous function never returns null so it can be removed from the return type.🪛 PHPStan (2.1.15)
20-20: Anonymous function never returns null so it can be removed from the return type.
(return.unusedType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Integration/FilterExceptionEvent.php(1 hunks)src/Integration/UseShopwareExceptionIgnores.php(1 hunks)src/Resources/config/packages/sentry.yaml(1 hunks)src/ShopwareSentryBundle.php(4 hunks)src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Resources/config/packages/sentry.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ShopwareSentryBundle.php
- src/Integration/FilterExceptionEvent.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php (1)
src/Integration/FilterExceptionEvent.php (4)
FilterExceptionEvent(8-36)__construct(14-16)getExceptions(21-24)setExceptions(29-32)
🪛 GitHub Check: phpstan
src/Integration/UseShopwareExceptionIgnores.php
[failure] 20-20:
Anonymous function never returns null so it can be removed from the return type.
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php
[failure] 11-11:
Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
🪛 PHPStan (2.1.15)
src/Integration/UseShopwareExceptionIgnores.php
20-20: Anonymous function never returns null so it can be removed from the return type.
(return.unusedType)
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php
11-11: Method Frosh\SentryBundle\Subscriber\FilterShopwareExceptionIgnoresSubscriber::__construct() has parameter $shopwareExceptionConfig with no value type specified in iterable type array.
See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
(missingType.iterableValue)
28-28: Cannot access offset 'log_level' on mixed.
(offsetAccess.nonOffsetAccessible)
🪛 GitHub Actions: PHP
src/Integration/UseShopwareExceptionIgnores.php
[error] 9-17: PHP CS Fixer formatting issue: Constructor should be in one line (public function __construct(private readonly EventDispatcherInterface $eventDispatcher) {}).
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php
[error] 7-17: PHP CS Fixer formatting issue: Constructor should be in one line (public function __construct(private readonly array $shopwareExceptionConfig) {}).
🔇 Additional comments (2)
src/Subscriber/FilterShopwareExceptionIgnoresSubscriber.php (1)
23-40: LGTM! Event-driven filtering logic is well implemented.The filtering logic correctly:
- Iterates through all exceptions in the event
- Filters out exceptions with 'notice' log level as intended
- Updates the event with the filtered exceptions
- Uses proper void return type
This implementation successfully moves the exception filtering logic into an event subscriber pattern, improving modularity and configurability.
🧰 Tools
🪛 PHPStan (2.1.15)
28-28: Cannot access offset 'log_level' on mixed.
(offsetAccess.nonOffsetAccessible)
src/Integration/UseShopwareExceptionIgnores.php (1)
21-26: Excellent refactor to event-driven exception filtering!The refactoring successfully:
- Replaces hardcoded exception filtering with event dispatching
- Maintains the same filtering behavior while improving extensibility
- Properly integrates with the new
FilterExceptionEventand subscriber pattern- Allows for configuration-driven exception filtering
This change significantly improves the modularity and maintainability of the exception filtering system.
| sentry: | ||
| options: | ||
| ignore_exceptions: | ||
| - 'Shopware\Storefront\Framework\Routing\Exception\SalesChannelMappingException' |
There was a problem hiding this comment.
Could be configured in the core as a shopware pull request and would automatically picked up by https://github.com/FriendsOfShopware/shopware-sentry-bundle/blob/main/src/Integration/UseShopwareExceptionIgnores.php ?
vendor/shopware/core/Framework/Resources/config/packages/framework.yaml:85
2 participants
Summary by CodeRabbit
New Features
Improvements