Skip to content

[FrameworkBundle] [Routing] DelegatingLoader: deprecate logger argument #16135

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

Merged
merged 1 commit into from
Oct 6, 2015
Merged

[FrameworkBundle] [Routing] DelegatingLoader: deprecate logger argument #16135

merged 1 commit into from
Oct 6, 2015

Conversation

ogizanagi
Copy link
Contributor

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

*/
public function __construct(ControllerNameParser $parser, LoggerInterface $logger = null, LoaderResolverInterface $resolver)
public function __construct(ControllerNameParser $parser, $logger = null, $resolver = null)
Copy link
Member

Choose a reason for hiding this comment

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

The variable names and the docblock should reflect the behaviour in Symfony 3.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, I'd like to. But if naming the $logger argument $resolver is pretty obvious, what should be named the old $resolver one ? 😄

Copy link
Member

Choose a reason for hiding this comment

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

Dunno, maybe just $r (similar to what @nicolas-grekas did in #16129).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, updated. Thanks.

@xabbuh
Copy link
Member

xabbuh commented Oct 5, 2015

We should throw exceptions when the variables do not have the expected type after the evaluation (it's not enforced by type hints anymore).

Status: Needs work

@ogizanagi
Copy link
Contributor Author

We should throw exceptions when the variables do not have the expected type after the evaluation (it's not enforced by type hints anymore).

This is enforced by the parent constructor for the $resolver. But this check might be indeed good for the $logger.

use Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser;
use Symfony\Component\Config\Exception\FileLoaderLoadException;
use Symfony\Component\Config\Loader\DelegatingLoader as BaseDelegatingLoader;
use Symfony\Component\Config\Loader\LoaderResolverInterface;
use Psr\Log\LoggerInterface;
Copy link
Member

Choose a reason for hiding this comment

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

Should be moved back, we don't reorder use statements to avoid merging conflict on other branches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@stof
Copy link
Member

stof commented Oct 6, 2015

👍

Status: reviewed

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

Thank you @ogizanagi.

@fabpot fabpot merged commit af0eba7 into symfony:2.8 Oct 6, 2015
fabpot added a commit that referenced this pull request Oct 6, 2015
…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
fabpot added a commit that referenced this pull request Oct 6, 2015
…ages (xabbuh)

This PR was merged into the 2.8 branch.

Discussion
----------

[FrameworkBundle][VarDumper] tweak some deprecation messages

| Q             | A
| ------------- | ---
| Fixed tickets | #16129, #16135
| License       | MIT

I was a bit too slow with reviewing.

Commits
-------

536045f tweak some deprecation messages
@ogizanagi ogizanagi deleted the 2_8/delegating_loader_logger_deprecation branch October 6, 2015 20:57
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.

6 participants