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

[Lock] Log already-locked errors as "notice" instead of "warning" #26020

Merged

Conversation

Simperfit
Copy link
Contributor

@Simperfit Simperfit commented Feb 2, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25887
License MIT
Doc PR todo

We add the ability to specify the log level we want in the lock component. That could be useful when parsing the logs.

img_3186 heic

@Simperfit
Copy link
Contributor Author

Travis failure are not related and seems to be already adressed in another PR.

@chalasr chalasr added this to the 4.1 milestone Feb 2, 2018
@jderusse
Copy link
Member

jderusse commented Feb 3, 2018

What's about injecting a logger with a dedicated channel instead of adding another argument to the Lock constructor?

@nicolas-grekas nicolas-grekas changed the title feautre: add the ability to modify the logger level in lock component [Lock] Add the ability to modify the logger level in lock component Feb 4, 2018
@nicolas-grekas
Copy link
Member

@jderusse but log channel != log level, isn't it?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 4, 2018

Hum, actually, shouldn't we just change these "warning" to "notice"? A lock failure is normal situation for a lock system. "notice" better fits the description of the log level IMHO.
On the other ide, making the log level configurable doesn't make really sense to me, at the semantical level.

@jderusse
Copy link
Member

jderusse commented Feb 4, 2018

Yep, it's not the same thing. But I wonder if the purpose of this PR wasn't just to reduce noise. That's why I suggested to use channel and move logs to another stream (this could be configured by the user without modifications on SF).

I'm also fine with reducing log level (I don't remember why I chose warning at first), Is it considered as BC break?

Anyway I agreed that configuring log level is not the right way to do.

@nicolas-grekas
Copy link
Member

ping @omnilight can you describe your use case? IMHO we should just turn that "warning" into "notice" in 3.4, as bug fix.

@omnilight
Copy link

omnilight commented Feb 4, 2018 via email

@nicolas-grekas
Copy link
Member

ok, so @Simperfit let's turn "warning" into "notice" on 3.4 and done IMHO

@omnilight
Copy link

omnilight commented Feb 5, 2018 via email

@Simperfit Simperfit force-pushed the feature/add-log-level-to-lock-component branch from 1c64a3d to d930cd8 Compare February 5, 2018 09:27
@Simperfit Simperfit changed the base branch from master to 3.4 February 5, 2018 09:27
@Simperfit Simperfit force-pushed the feature/add-log-level-to-lock-component branch 2 times, most recently from 01520a7 to cee77a9 Compare February 5, 2018 09:31
@Simperfit
Copy link
Contributor Author

done @nicolas-grekas

@Simperfit Simperfit force-pushed the feature/add-log-level-to-lock-component branch from cee77a9 to 52222b4 Compare February 9, 2018 16:08
@nicolas-grekas nicolas-grekas changed the title [Lock] Add the ability to modify the logger level in lock component [Lock] Log already-locked errors as "notice" instead of "warning" Feb 11, 2018
@nicolas-grekas nicolas-grekas force-pushed the feature/add-log-level-to-lock-component branch from 52222b4 to 2a74edb Compare February 11, 2018 11:29
@nicolas-grekas
Copy link
Member

Thank you @Simperfit.

@nicolas-grekas nicolas-grekas merged commit 2a74edb into symfony:3.4 Feb 11, 2018
nicolas-grekas added a commit that referenced this pull request Feb 11, 2018
…arning" (Simperfit)

This PR was merged into the 3.4 branch.

Discussion
----------

[Lock] Log already-locked errors as "notice" instead of "warning"

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #25887 <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | todo

We add the ability to specify the log level we want in the lock component. That could be useful when parsing the logs.

![img_3186 heic](https://user-images.githubusercontent.com/3451634/35733664-8ed991dc-081e-11e8-89dd-9efff253e492.jpeg)

Commits
-------

2a74edb [Lock] Log already-locked errors as "notice" instead of "warning"
This was referenced Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants