Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log unsuccessful deliveries to the system log #337

Merged
merged 3 commits into from
May 24, 2024

Conversation

fritzmg
Copy link
Collaborator

@fritzmg fritzmg commented May 23, 2024

This implements #335

image

@fritzmg fritzmg requested a review from Toflar May 23, 2024 21:37
@fritzmg
Copy link
Collaborator Author

fritzmg commented May 23, 2024

Hm, the dependency checker complains about

Found shadow dependencies!
(those are used, but not listed as dependency in composer.json)

  • psr/log
      e.g. Psr\Log\LoggerInterface in src\EventListener\LogUnsuccessfulDeliveries.php:14

However this is technically not a hard dependency and thus does not necessarily need to be added as a requirement to the composer.json of this package. wdyt?

@Toflar
Copy link
Member

Toflar commented May 24, 2024

However this is technically not a hard dependency and thus does not necessarily need to be added as a requirement to the composer.json of this package. wdyt?

I think this specific error we can ignore by adding a comment that logging is optional in https://github.com/terminal42/contao-notification_center/blob/main/depcheck.php

@Toflar
Copy link
Member

Toflar commented May 24, 2024

PR looks correct otherwise, I guess you also tested it? :)

@fritzmg
Copy link
Collaborator Author

fritzmg commented May 24, 2024

PR looks correct otherwise, I guess you also tested it? :)

Yes, screenshots from above are from the test 👍

@Toflar Toflar merged commit 2ba97e7 into terminal42:main May 24, 2024
4 checks passed
@Toflar
Copy link
Member

Toflar commented May 24, 2024

Thanks @fritzmg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants