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

Use Monolog:SyslogHandler syslog default facility #17855

Merged
merged 15 commits into from Aug 24, 2021
Merged

Use Monolog:SyslogHandler syslog default facility #17855

merged 15 commits into from Aug 24, 2021

Conversation

mwithheld
Copy link
Contributor

Description:

This is a further fix to #17764, fixing the Monolog:SyslogHandler create() call to use its constructor-default facility rather than manually setting it to 'syslog' incorrectly.

@diosmosis
Copy link
Member

@mwithheld I think you'll need to merge these changes w/ 4.x-dev to remove the conflicts, something like:

git checkout 4.x-dev
git pull upstream 4.x-dev # upstream should be the matomo-org/matomo repo
git checkout patch-2
git merge 4.x-dev

@sgiehl sgiehl added the Needs Review PRs that need a code review label Aug 4, 2021
@@ -105,7 +105,8 @@
->method('setFormatter', DI\get('log.lineMessageFormatter.file')),

'\Monolog\Handler\SyslogHandler' => DI\create()
->constructor(DI\get('log.syslog.ident'), 'syslog', DI\get('log.level.syslog'))
->constructorParameter('ident' => DI\get('log.syslog.ident'))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
->constructorParameter('ident' => DI\get('log.syslog.ident'))
->constructorParameter('ident', DI\get('log.syslog.ident'))

@@ -105,7 +105,8 @@
->method('setFormatter', DI\get('log.lineMessageFormatter.file')),

'\Monolog\Handler\SyslogHandler' => DI\create()
->constructor(DI\get('log.syslog.ident'), 'syslog', DI\get('log.level.syslog'))
->constructorParameter('ident' => DI\get('log.syslog.ident'))
->constructorParameter('level' => DI\get('log.level.syslog'))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
->constructorParameter('level' => DI\get('log.level.syslog'))
->constructorParameter('level', DI\get('log.level.syslog'))

@diosmosis
Copy link
Member

@mwithheld left a couple suggestions, after those I should be able to merge it.

@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Aug 17, 2021
@diosmosis
Copy link
Member

Hi @mwithheld, sorry for the late reply, it seems like you added some commits for another PR into this one (the one dealing w/ NotSupportedBrowserException).

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Aug 18, 2021
@mwithheld
Copy link
Contributor Author

Good catch - I've reset the branch to before the incorrect ones, force-pushed the branch, and merged updates from the 4.x-dev branch. Hopefully that is clean now.

@diosmosis diosmosis added this to the 4.5.0 milestone Aug 24, 2021
@diosmosis diosmosis merged commit e817313 into matomo-org:4.x-dev Aug 24, 2021
@diosmosis
Copy link
Member

Thanks for fixing up the PR @mwithheld! It's now merged.

@mwithheld mwithheld deleted the patch-2 branch August 25, 2021 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants