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

Symfony autowiring monolog channels #278 #315

Open
wants to merge 5 commits into
base: master
from

Conversation

@adrenalinkin
Copy link

commented Aug 10, 2019

resolved #278

  • Added possibility for auto-wire monolog channels according to variable type-hint and name.
    Variable will have appropriated name to camel cased monolog channel service name: monolog.logger.acme -> $acmeLogger.
  • Removed useless import DefinitionDecorator.
  • Renamed additional channel in the tests from manualchan to additional
ISSUE-278:
- Added possibility for auto-wire monolog channels according to variable type-hint and name.
  Variable will have appropriated name to camel cased monolog channel service name: `monolog.logger.acme -> $monologLoggerAcme`.
- Removed useless import `DefinitionDecorator`.
@lyrixx

lyrixx approved these changes Aug 13, 2019

Copy link
Member

left a comment

👍 LGTM.
Could you add a note in the CHANGELOG?
It will be part of 3.5.0
Thanks

@nicolas-grekas @stof Could you review this one, please?

adrenalinkin added some commits Aug 13, 2019

@adrenalinkin

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

@lyrixx Thanks. Now information has been add into CHANGELOG.
Should I add some documentation about new feature?

@tristanbes

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

I would be interested on how to use that in the README.MD since it adds great DX; (or in the Symfony documentation maybe in the monolog usage ?)

@Seldaek

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Yup more docs on this would be good, if I understand correctly you can only name the variable $monologAcmeLogger for monolog.logger.acme service/acme channel? Any chance of allowing $acmeLogger as well? I feel like the monolog part is kinda superfluous and overly verbose here.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

acmeLogger

Would make total sense to support only that.
Could be done by passing $channel.'Logger' as 3rd arg to createAliasForArgument.

A full example would be nice I agree because I'm not sure when this would be useful yet :)

@adrenalinkin

This comment has been minimized.

Copy link
Author

commented Aug 16, 2019

@nicolas-grekas @Seldaek Hello. Just my opinion to clarify this decision.

Current logic is pretty simple. A long time documentation teach us how can we find channel service in the DI container by specific logic monolog.logger.<channel_name>.

When auto-wire has been introduce, anyone who previously solves problem with monolog channel auto-wiring, know, how exactly channel services names generates: acme become to monolog.logger.acme.

So with new feature we are continue this logic - monolog.logger.acme become to Psr\Log\LoggerInterface $monologLoggerAcme.
Furthermore we get stronger protection from naming collision in the projects.

@Seldaek

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

IMO naming collision protection isn't needed here as you can only collide with things of class LoggerInterface.. so the monolog name doesn't add anything.

@adrenalinkin

This comment has been minimized.

Copy link
Author

commented Aug 16, 2019

@Seldaek agree, so we can simplify naming according to
monolog.logger.acme -> Psr\Log\LoggerInterface $loggerAcme.

Add the "like" to this comment if this solution acceptable. So I will update code.

About documentation - I am already investigates Symfony Documentation according to all places, where would be good to add information about this new feature.
I will create PR on this holidays.

@Seldaek

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

I still find $acmeLogger more readable than $loggerAcme, If you read like a sentence, you would speak of "the acme logger", or "the logger whose name is acme", but not "the logger acme".

The service name convention was different, but it doesn't mean we have to carry that over to the variable name.

@adrenalinkin

This comment has been minimized.

Copy link
Author

commented Aug 16, 2019

@Seldaek I am OK to change as you propose.
We just have different principle of the naming.
I was proposed "logger" first because it common for all variables.
$loggerAcme, $loggerDoctrine, $loggerSecurity etc.
Maybe this approach good only for buisness-code not for reusable.

@Seldaek

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

sorry but I don't get your point, I am only suggesting to swap these, i.e. $acmeLogger, $doctrineLogger, $securityLogger.

@adrenalinkin

This comment has been minimized.

Copy link
Author

commented Aug 16, 2019

@Seldaek no matter, I will change naming as you describes :)

ISSUE-278:
- Updated naming strategy. Channel 'test' become '$testLogger' and etc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.