Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Allow to pass a logger instance to the Router#24826
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| * @param LoggerInterface|null $logger | ||
| */ | ||
| publicfunction__construct(ContainerInterface$container,$resource,array$options =array(),RequestContext$context =null) | ||
| publicfunction__construct(ContainerInterface$container,$resource,array$options =array(),RequestContext$context =null,LoggerInterface$logger =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
how about using a nullable typehint ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We reverted it last time we tried adding one 😄
See discussion starting from#22743 (comment)
| $this->resource =$resource; | ||
| $this->context =$context ?:newRequestContext(); | ||
| $this->logger =$logger; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just wondering: Why don't we call the parent class' constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The$loader argument is mandatory in parent constructor, but in this implementation we're lazy-loading it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks, I missed that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should probably change this in 4.1 to have a lazy-loading implementation of the LoaderInterface (using composition over the actual loader which would get loaded when needed), to avoid having a special constructor.
fabpot commentedDec 1, 2017
Thank you@ogizanagi. |
…he Router (ogizanagi)This PR was merged into the 4.1-dev branch.Discussion----------[FrameworkBundle] Allow to pass a logger instance to the Router| Q | A| ------------- | ---| Branch? | 4.1 <!-- see comment below -->| 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 |#24739 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AAs explained in#24739, this will allow the `UrlGenerator` to log invalid calls when `router.strict_requirements` is `false` (so instead of throwing):<img width="1064" alt="screenshot 2017-10-29 a 09 57 31" src="https://user-images.githubusercontent.com/2211145/32142080-482bc64e-bc90-11e7-8382-b78b507bae48.PNG">~~This PR must re-introduce the `logger` argument in the definition along with the `monolog.logger` tag removed for cleaning in#24739, once it's merged up into master.~~ DoneCommits-------78f4f88 [FrameworkBundle] Allow to pass a logger instance to the Router
Uh oh!
There was an error while loading.Please reload this page.
As explained in#24739, this will allow the
UrlGeneratorto log invalid calls whenrouter.strict_requirementsisfalse(so instead of throwing):This PR must re-introduce theDoneloggerargument in the definition along with themonolog.loggertag removed for cleaning in#24739, once it's merged up into master.