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

Add autoconfigure for handlers #670

Closed
wants to merge 1 commit into from

Conversation

magnetik
Copy link
Contributor

@magnetik magnetik commented Jul 11, 2018

Such like events we can auto configure handlers.
Also remove checks for registerForAutoconfiguration as symfony/dependency-injection required version is ^3.3.

@goetas
Copy link
Collaborator

goetas commented Jul 11, 2018

good! :)

{
$container = $this->getContainerForConfig(array());

if (!method_exists($container, 'registerForAutoconfiguration')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In which case the registerForAutoconfiguration does not exists? Shouldn't this check be done also in JMSSerializerExtension.php ?

Copy link
Contributor Author

@magnetik magnetik Jul 11, 2018

Choose a reason for hiding this comment

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

It's also done in the extension (line 22)

registerForAutoconfiguration was added in Symfony 3.3, so it's for <3.3 compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... and I just saw that symfony-dependency-injection require is ^3.3 so theses checks can be removed.

Copy link
Collaborator

@goetas goetas Jul 11, 2018

Choose a reason for hiding this comment

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

The minimum dependency injection is 3.3, so probably this check and also the other you found can be removed... Can you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

@magnetik
Copy link
Contributor Author

Just saw that there is a branch for 2.x, should I rebase on it?

@goetas
Copy link
Collaborator

goetas commented Jul 11, 2018

The 2.x branch has 2.3 as minimum dep version for the symfony dependency injection, so probably you will have to rework your PR.

Here is up to you... the 3.0 should be released in the next 30-50 days (but will require jms/serializer 2.0 and not all the depending libraries are already compatible with it).

@magnetik
Copy link
Contributor Author

another PR for 2.x, is that fine for you?

@goetas
Copy link
Collaborator

goetas commented Jul 11, 2018

Closing as #671 has been merged.
Changes from 2.x are regularly merged back to master

@goetas goetas closed this Jul 11, 2018
@goetas
Copy link
Collaborator

goetas commented Jul 11, 2018

@magnetik Thanks for your contribution!

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.

None yet

2 participants