-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
fix type assignement #16615
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
fix type assignement #16615
Conversation
|
@Tobion who can confirm this ? |
In light of this, the This way it would be possible to actually configure different types of one class in the DI. Otherwise it would only be possible with a custom FormExtension. |
The errors are not coming from this PR. |
@@ -88,7 +88,7 @@ public function getType($name) | |||
} | |||
} | |||
|
|||
$this->resolveAndAddType($type); | |||
$this->types[$name] = $this->resolveAndAddType($type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolveAndAddType()
is no longer adding a type, so the name is incorrect now.
@jakzal fixed |
@rande could you please add a test? |
@nicolas-grekas I did not change public methods, what kind of test do you want ? |
A test that will prevent any regression... You changed the behavior, this must be actionable from the outside, since you hit the issue :) |
This one looks important to me if that blocks the update of Sonata admin. Any comments before I merge it? ping @symfony/deciders |
At least, we need a test to prevent future regressions. Discussing whether or not multiple form types of the same class should be possible to register can be discussed in a different issue then. |
Ah I see, we already have #16611 for that. |
Looks like a good fix. We use |
Also looks good to me, once there is a test. @rande are you able to add a test? About @Tobion's comment, we could certainly re-add the |
Yeah let's keep
But I still think this should be changed as well. For core types it doesn't change anything, but it allows more flexibility. E.g. you can write a form type extension is that based on the name or for the class if you have multiple of the same. |
I don't really want to introduce aliases for types again. We managed to reduce complexity by removing one layer of abstraction, namely the mapping of strings to type classes. Using type classes with the I think the way to solve this problem is by sub-typing. If you have an abstract type and want more specific types inheriting from that type, you can do that. I know this is not as elegant as before, but it's not hard. In the long run, I want to also remove the "virtual inheritance" model, i.e. let types directly inherit their parent type. Then this will become even easier. For me, the overall goal is to reduce complexity of the component as much as possible. Necessarily this means that some things need to be changed for major version bumps. |
For the record, the following PR https://github.com/sonata-project/SonataCoreBundle/pull/212/files will re-introduce alias feature for Symfony3. I know it is bad, but it is one thing (among small others) that avoid making bundles compatible with SF2.7 => SF3.0. SF2.7 and SF2.8 are LTS versions of Symfony, we (sonata-project) cannot afford to maintain 2 branches. I don't see how alias add complexity, I see more complexity with the 3.0 implementation. |
The other solution for us will be to only support 2.8 and 3.0. Dropping support for 2.3 and 2.7 LTS. @lsmith77 what is the CMF policy on symfony version support ? |
right now we are hoping to continue to support 2.3 until May next year .. but maybe we need to stare reality into the eye .. /cc @dbu |
@rande eventually you would drop support for 2.7 LTS (i.e. when the LTS itself is not supported), and then you could stop using the form extension "alias" thing in Sonata and use multiple classes instead, correct? Correct me if I'm over-simplifying (because you are working in a real use-case), but the complexity is really caused by maintaining a bundle that works across multiple versions (where this new system exists versus doesn't exist), right? |
IMO, the In one of our application, we have abstracted all our models to something called Now, conssider we have more than 100+ model classes. Upgrading to newer version obliges us to create more than 100+ form types and also update the generic way we register our forms which defintively not simplify things for us. Additionally, I'm just talking about the choice forms here but we have also abstracted other form types... For now, if I have to upgrade this app, I would just say that I can't without heavily refactoring this system... Basically, the main issue is in Symfony 2.x we were able to map multiple form types to a single implementation (n:n relation) whereas now we are stuck to a 1:1 relation which sounds like a feature removal to me. |
@egeloen the SonataCoreBundle's PR will add the feature back and be used as a transition mechanism. |
So there are only 2 paths, correct?
|
I suggest you work on a community bundle that adds the form type aliasing. If I'm not mistaken, this should be possible by creating a decorator for the form factory that maintains an internal map from aliases to FQCNs. You can then add your own names dynamically to that map. |
@webmozart do you confirm that the form component will have a 1:1 relation and it will be that for Symfony 3.0 with no chance to change your mind on this topic? Using the community bundle will work but will just be a workaround solution since I would prefer to follow the internal form architecture. So, at a time, I will conssider upgrade this app anyway and so introduce plenty of form types just to be able to use them by FQCN... It will remember me when I have to upgrade forms from Symfony 2.0 to 2.1. |
Here the documentation of @webmozart proposal. Can be extract on its own if required .. https://github.com/sonata-project/SonataCoreBundle/pull/212/files#diff-76ab2cc9f90b0fbbd52c3d6cc001a8d4 @weaverryan actually, the AdminBundle can be compatible with sf2.3 and upper if this PR get merged. see https://gist.github.com/rande/e4f8de0330a7a9f6f1b7 . |
@egeloen In Symfony 3.0, the mapping from aliases to type classes was removed. This simplifies both using and creating form types for a majority of our users. Also, this makes it possible to remove the virtual inheritance model and improve performance in future releases. It's not possible to revert this, both due to our release policy and because then we won't be able to do necessary performance improvements. I'm sorry that your use case becomes more complicated. But as I said, creating an aliasing bundle might be a good solution. |
@rande that's what I meant by my option (2), because this PR effectively re-adds the ability to do aliasing via extensions, correct? Does your linked 212 PR require this PR to be merged? @webmozart do you have issues with this PR specifically? |
@weaverryan no, looks good 👍 |
@rande Would be great if you can extract it from the Sonata core and provide it as a standalone component/bundle. |
@weaverryan this PR fixes the original method signature from the PHPDoc. As a side effect, it will indeed make the sonata's PR works ;) |
@egeloen ok, I can do that. I will have to do a massive search and replace ;) How should we name this plugin => Symfony3LegacyBundle ... it will be an open door as people will want to restore.... hum ... the |
Thank you @rande. |
This PR was squashed before being merged into the 3.0-dev branch (closes #16615). Discussion ---------- fix type assignement | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR make sure the original key is used when the type is resolved. Also the code now respect the PHPDoc. Commits ------- cc26454 fix type assignement
@fabpot I want my black elephant ;) |
@rande What about |
@egeloen you don't want SonataCoreBundle ;) |
I don't know what is shipped witht the Sonata core bundle but probably much more that what I want :) |
how would this work for type extensions wanting to hook at different levels of the type hierarchy ? Or for form theming ? |
This PR make sure the original key is used when the type is resolved. Also the code now respect the PHPDoc.