8000 Migration from Form AbstractType setDefaultOptions() to configureOptions broken by concrete injection · Issue #14827 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Migration from Form AbstractType setDefaultOptions() to configureOptions broken by concrete injection #14827

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.

Sign up for GitHub

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

Jump to bottom
Closed
bryanagee opened this issue Jun 2, 2015 · 8 comments
Closed

Migration from Form AbstractType setDefaultOptions() to configureOptions broken by concrete injection #14827

bryanagee opened this issue Jun 2, 2015 · 8 comments
Labels
Form

Comments

@bryanagee
Copy link

Since ::setDefaultOptions() required an interface, you could inject any OptionResolver interface. The ::configureOptions method requires a concrete type instead. The largest impact is that any overriding method will throw a notice due to the mismatched signature.
@bryanagee
Copy link
Author

Is there a specific reason the signature was changed to concrete? I can see that @peterrehm made the change in 3d43cae, but I don't see an explanation for the switch.

@jakzal
Copy link
Contributor
jakzal commented Jun 2, 2015

see #12782 and #12891

When migrating you should obviously change the signature, not just the method name. A new method was chosen to make migration easier.

What's your issue about? Have you implemented the interface yourself?

@jakzal jakzal added the Form label Jun 2, 2015
@bryanagee
Copy link
Author

We haven't implemented the interface, but I'm curious as to why there is a break in the use of Interface Injection in this specific instance.

I can certainly go back and change all of those classes to use the new signature, but I'm not keen on it without understanding why the deviation from normal practice is there in the first place.

@jakzal
Copy link
Contributor
jakzal commented Jun 2, 2015

As written in UPGRADE-2.6.md:

  • The interface OptionsResolverInterface was deprecated, since
    OptionsResolver instances are not supposed to be shared between classes.
    You should type hint against OptionsResolver instead.

Closing, as this is not a bug.

@jakzal jakzal closed this as completed Jun 2, 2015
@bryanagee
Copy link
Author

So essentially, other implementations of OptionsResolver are no longer supported?

The quote from the UPGRADE document doesn't really address the question as to why. Sharing of the instances seems to have little to do with InterfaceInjection. Am I missing something? Forgive me if this is a stupid question.

@jakzal
Copy link
Contributor
jakzal commented Jun 2, 2015

We simply don't want to support alternative implementations of the OptionsResolver.

@Tobion
Copy link
Contributor
Tobion commented Jun 2, 2015

The point is that working with interfaces makes it hard to impossible to improve the optionsresolver itself since we cannot introduce new/changed methods in the inteface without breaking bc. But we can easily add new methods to the optionsresolver implementation. So in this case we decided it's not worth to provide an interface that nobody would reimplement anyway.

@bryanagee
Copy link
Author

@Tobion - makes sense; thank you for explaining the why. I will migrate those with the updated signature.

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Form
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jakzal @bryanagee @Tobion
0