[Form] setReadOnly and setAttribute on a Form after creation #2797
Conversation
|
Any good/bad comments on the issue? |
|
@bschussek can you give your mind here ? |
|
+1. I'm in the situation where I need to set my form to readonly depending on the data being set on it, which is impossible at the moment |
|
Any news on this one? |
|
Not that i know! Waiting for @bschussek blessings |
|
@stof can i do something with this one? It's kinda frustrating, sincerely... |
|
Seems like there is a lot of activity regarding #3193, so this PR should change the things from "read_only" to "disabled" |
|
The problem about this PR is that event listeners and data mappers access the form's attributes during initialization of a form. Changing the attributes after construction leaves the form in an inconsistent state, unless you call So, although I am not completely decided yet, I'm rather opposed to this change. |
|
I think in that case that should be a responsability of the coder to use it wisely. I only call this from the preSetData event, so i guess there are no problems there. Is there any test case i could build to break the thing? Or we should maybe mimic the |
|
still +1 I'd also use this in PreSetData only. Today I ran into a situation again, were i'd want to change the form after it was build, but before the data was set: I have a choicelist where the list of choices depends on the value being set. My only option now is to hack the choices into the view in buildViewBottomUp, while i'd actually want to do: $form->set('choice_list', new EntityChoiceList($em, $class, $choices)); |
|
@Burgov This is again an example why I'm against this change. Changing the "choice_list" attribute of a form after its initialization breaks the form and requires reconfiguration in case of expanded choice lists. This will confuse people, like it confused you already. In your case you should create a new choice field with the new list of choices. |
|
Maybe this pull request is too "open" and the problems arise when you change attributes like the 'choice_list' you mentioned. It should probably be limited to the readOnly var... |
|
@acasademont How do you want to decide which properties to restrict it too? What if a dev adds a choice type with attributes that in theory can be changed too without problems? How will users know which attributes they can change and which they can't? This gets way too messy IMO. |
|
"read_only" attribute it's treated very different from other attributes as it has its own private variable whereas the others are stored in an array. But yes, if it doesn't make sense for any attribute it doesn't make sense for the read_only either. The main goal of this PR is to minimize code duplicates, as now i have to have the exact same lines in my FormType and in my Form Subscribers if i want to change something on the form based on the current data. Even worse, as in the form subscriber i have to manually set the field_type of the form field, whereas in the FormType i have the type guesser available. And as you know there are some form types (collection basically) that have a lot of options and properties that it's a pain in the ass to write 2 times...with the problems that arise if i change something in my FormType and i forget to change it in the subscriber. That should be the main goal. Maybe this is not the correct approach for solving the original problem, that's possible :) |
|
You can minimize the duplicated code by always adding the affected field in event listeners. You can have the listener listen to PRE_BIND and PRE_SET_DATA and use an internal method for creating the field in both cases. Furthermore, you can inject the FormFactory into your listener to make use of field guessing (see |
|
That would be nice, but as far as i know, it's not possible to set the position in which a field should be added, so fields created in the listener would always be at the end of the form if they have not been previously set in the FormType. |
|
You should control the order of the fields in the template, not in the form type. |
|
Sure, and i do that with some complex forms whose layout must be carefully set. But for the majority of entities it suffices with the |
|
@acasademont Then what you really want is to pass the order of rendered fields to I'm closing this PR as there wasn't really any powerful argument yet for this functionality that warrants the risks that we introduce. We can discuss how to solve the desired use cases on the ML. |
|
Fair enough! |
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass:
Fixes the following tickets: -
Todo: -
In a From EventSubscriber i can manage to get the form via $event->getForm() but i am pretty limited about what i can do with that form. If i only want to change the read_only property i can't because it's private, so i end up creating the field again with the same name and overwriting it with
which is a bit time consuming since i only wanted to set the read_only property of that field to false.
I must say that i found it a bit strange that there weren't any options for doing this so please, tell me if there is any powerful reason for not being able to change the read_only (or other attributes) when the form is already created.