[FrameworkBundle] [Routing] DelegatingLoader: remove unused logger #16117
Conversation
| * @param LoaderResolverInterface $resolver A LoaderResolverInterface instance | ||
| */ | ||
| public function __construct(ControllerNameParser $parser, LoggerInterface $logger = null, LoaderResolverInterface $resolver) | ||
| public function __construct(ControllerNameParser $parser, LoaderResolverInterface $resolver) |
There was a problem hiding this comment.
What about deprecating the $logger argument in Symfony 2.8 before?
There was a problem hiding this comment.
removing the argument in the middle of the signature is a BC break.
It is required to accept the new API in 2.8 already to ease the migration (and to throw a deprecation warning when using the old signature)
There was a problem hiding this comment.
So, what should be done in order to have both old and new api compatible in 2.8 ? Something like:
/**
* @param ControllerNameParser $parser A ControllerNameParser instance
* @param LoggerInterface|LoaderResolverInterface $logger A LoggerInterface instance
* @param LoaderResolverInterface|null $resolver A LoaderResolverInterface instance
*/
public function __construct(ControllerNameParser $parser, $logger = null, $resolver = null)
{
$this->parser = $parser;
$this->logger = $logger instanceof LoggerInterface ? $logger : null;
parent::__construct($logger instanceof LoaderResolverInterface ? $logger : $resolver);
}(the old api will still be used by the FrameworkBundle in 2.8 I guess) and raising a deprecation warning when subclasses access the $logger property by using a magic getter as @Tobion suggested ?
Pretty confusing.
There was a problem hiding this comment.
The deprecation warning should be triggered when a logger is passed as the second argument in Symfony 2.8. You can take a look at the FragmentHandler for an example.
There was a problem hiding this comment.
@xabbuh : Thanks for the reference. If I understand you right, this is what the changes would be like: 2.8...ogizanagi:2_8/delegating_loader_logger_deprecation
If it looks good to you, I'll create a second PR to merge before this one into 2.8.
There was a problem hiding this comment.
I will need some changes, but I think it's easier if you make the pull request and we can give you feedback there.
There was a problem hiding this comment.
Alright, opened: #16135
| @@ -28,20 +27,17 @@ | |||
| class DelegatingLoader extends BaseDelegatingLoader | |||
| { | |||
| protected $parser; | |||
| protected $logger; | |||
There was a problem hiding this comment.
we could also raise a deprecation warning when subclasses access this property by using a magic getter for it.
…logger argument (ogizanagi) This PR was merged into the 2.8 branch. Discussion ---------- [FrameworkBundle] [Routing] DelegatingLoader: deprecate logger argument | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Related to #16117 Commits ------- af0eba7 [FrameworkBundle] [Routing] DelegatingLoader: deprecate logger argument
There is no harm into keeping it, but I think 3.0 is the occasion to remove such things.
Also proposed in #11742