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

[DI] Dont trigger deprecation for event_dispatcher service #22223

Merged
merged 1 commit into from
Apr 3, 2017

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Mar 30, 2017

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22208
License MIT
Doc PR n/a

Mute deprecations for the event_dispatcher service keeping only relevant ones i.e when the api of the deprecated class is used intentionally, ugly but prevent breaking test suites like the LexikJWTAuthenticationBundle one.


$service = null === $r->getConstructor() ? $r->newInstance() : $r->newInstanceArgs($arguments);
// don't trigger deprecations for internal uses, to be removed in 4.0 along with the deprecated class
Copy link
Member

Choose a reason for hiding this comment

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

personally, I try to always add the @deprecated string to ease doing grep. I don't expect anyone to read all the source code looking for specific notices about 4.0 (or we'd expect to miss some for sure)

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍 with minor comment

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Apr 3, 2017
@chalasr
Copy link
Member Author

chalasr commented Apr 3, 2017

@deprec annotation added

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍

@fabpot
Copy link
Member

fabpot commented Apr 3, 2017

Thank you @chalasr.

@fabpot fabpot merged commit a49fe25 into symfony:master Apr 3, 2017
fabpot added a commit that referenced this pull request Apr 3, 2017
… (chalasr)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Dont trigger deprecation for event_dispatcher service

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22208
| License       | MIT
| Doc PR        | n/a

Mute deprecations for the event_dispatcher service keeping only relevant ones i.e when the api of the deprecated class is used intentionally, ugly but prevent breaking test suites like the [LexikJWTAuthenticationBundle one](https://travis-ci.org/lexik/LexikJWTAuthenticationBundle/jobs/216664013#L278).

Commits
-------

a49fe25 [DI] Dont trigger deprecation for event_dispatcher service
@chalasr chalasr deleted the event_dispatcher branch April 3, 2017 23:11
@garak
Copy link
Contributor

garak commented Jun 2, 2017

This is not working for me. I'm on 3.3.0 and I still get deprecations, both in tests and in profiler: "The Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher class is deprecated since version 3.3 and will be removed in 4.0. Use EventDispatcher with closure-proxy injection instead: 160x"

@chalasr
Copy link
Member Author

chalasr commented Jun 2, 2017

@garak the deprecation you are getting is not the one fixed here.
Please open a new issue, with reproducer if possible (I just tried on a fresh SE with 3.3 and can't reproduce your problem).

@garak
Copy link
Contributor

garak commented Jun 2, 2017

You can reproduce by installing KnpPaginatorBundle, so composer require knplabs/knp-paginator-bundle and adding new Knp\Bundle\PaginatorBundle\KnpPaginatorBundle(), in your AppKernel.
Even if you could think that the culprit is that bundle, I cannot find any evidence of ContainerAwareEventDispatcher in its source.
This is the relevant part of code where the deprecation notice is tracked, in appDevDebugProjectContainer.php:

protected function getDebug_EventDispatcherService()
{
        $this->services['debug.event_dispatcher'] = $instance = new \Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher(new \Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher($this), ${($_ = isset($this->services['debug.stopwatch']) ? $this->services['debug.stopwatch'] : $this->get('debug.stopwatch')) && false ?: '_'}, ${($_ = isset($this->services['monolog.logger.event']) ? $this->services['monolog.logger.event'] : $this->get('monolog.logger.event', ContainerInterface::NULL_ON_INVALID_REFERENCE)) && false ?: '_'});
// ...
}

@xabbuh
Copy link
Member

xabbuh commented Jun 3, 2017

I was not able to reproduce this by just adding the KnpPaginatorBundle to a blank Symfony 3.3 Standard Edition. Please open a new issue with steps to perform to be able to reproduce your issue if this persists for you.

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