[Form] read_only and disabled attributes (closes #1974) #3193

helmer · 2012-01-26T16:05:24Z

Removed readOnly property from Form, as it is no longer required
Introduced disabled property to Form, behaves exactly like readOnly used to
Added disabled property to fields, defaults to false, renders as disabled="disabled"
A field with positive read_only property now renders as readonly="readonly"

stof · 2012-01-26T17:40:25Z

tests/Symfony/Tests/Component/Form/AbstractLayoutTest.php

@@ -1299,6 +1299,36 @@ public function testHidden()
        );
    }

+    public function testReadOnly()
+    {
+        $form = $this->factory->createNamed('text', 'na&me', null, array(


use a name instead of na&me. It will make things easier as the current refactoring will forbid &

Fixed. If you care to enlighten me, why is the ampersand included in the examples in the first place?

To test for correct escaping.

helmer · 2012-01-26T17:46:17Z

I changed Form and FormBuilder property readOnly to disabled. On second thought, this is perhaps not such good change - while readOnly somewhat implied the use-case, disabled no longer does.

Perhaps something else, like bindable (as not to confuse with read_only attribute of Fields)?

@bschussek, others, any thoughts?

helmer · 2012-01-29T17:11:05Z

src/Symfony/Component/Form/Form.php

@@ -697,10 +688,9 @@ public function isValid()
            return false;
        }

-        if (!$this->readOnly) {
+        if (!$this->disabled) {


Should probably use !$this->isDisabled() here (returns true if any parent is disabled), right?

Fixed. Did the same for bind function as well.

webmozart · 2012-01-31T06:53:59Z

Please prefix commits with the affected component, if applicable.

helmer · 2012-01-31T08:41:03Z

@bschussek Prefixed. Please also see see to this question

Commits ------- de253dd [Form] read_only and disabled attributes Discussion ---------- [Form] read_only and disabled attributes (closes #1974) 1. Removed ``readOnly`` property from ``Form``, as it is no longer required 2. Introduced ``disabled`` property to ``Form``, behaves exactly like ``readOnly`` used to 3. Added ``disabled`` property to fields, defaults to ``false``, renders as ``disabled="disabled"`` 4. A field with positive ``read_only`` property now renders as ``readonly="readonly"`` --------------------------------------------------------------------------- by helmer at 2012-01-26T17:46:17Z I changed ``Form`` and ``FormBuilder`` property ``readOnly`` to ``disabled``. On second thought, this is perhaps not such good change - while readOnly somewhat implied the use-case, disabled no longer does. Perhaps something else, like ``bindable`` (as not to confuse with read_only attribute of Fields)? @bschussek, others, any thoughts? --------------------------------------------------------------------------- by bschussek at 2012-01-31T06:53:59Z Please prefix commits with the affected component, if applicable. --------------------------------------------------------------------------- by helmer at 2012-01-31T08:41:03Z @bschussek Prefixed. Please also see see to [this question](#3193 (comment))

webmozart · 2012-02-02T09:06:57Z

This PR is still flawed in that the read_only option does not regard the read_only setting of parent fields.

@helmer: Can you fix this?

helmer · 2012-02-02T09:32:55Z

@bschussek Please explain with an usecase, I did not quite get it .. complex (ie collection) fields get rendered incorrectly?

webmozart · 2012-02-02T09:35:38Z

@helmer: exactly

helmer · 2012-02-02T10:46:51Z

@bschussek Hmm, my test-case passes (as you can see both attributes are there) .. please elaborate:

Controller

$opt = array(
    'choices'   => array('male', 'female'),
    'read_only' => true,
    'disabled'  => true,
    'expanded'  => true,
);
return $this->render('AppCodeInterviewBundle::test.html.twig', array(
    'form' => $this->createFormBuilder()->add('gender', 'choice', $opt)->getForm()->createView(),
));

View

{{ form_widget(form) }}

Output

<div id="form_gender">
    <input type="radio" id="form_gender_0" name="form[gender]" disabled="disabled" required="required" value="0" />
    <label for="form_gender_0" class=" required">male</label>
    <input type="radio" id="form_gender_1" name="form[gender]" disabled="disabled" required="required" value="1" />
    <label for="form_gender_1" class=" required">female</label>
</div>

webmozart · 2012-02-02T10:48:37Z

@helmer: Remove "disabled" and leave only "read_only". As you see, "readonly" is missing from the radio buttons.

helmer · 2012-02-02T11:55:33Z

@bschussek I must be getting blind, the pasted snippet clearly lacked readonly attribute [not my problem required resembles readonly alot, right? :) ]. I commited a fix here, please confirm before I open PR.

webmozart · 2012-02-02T12:00:31Z

@helmer This is not the correct way to solve it. Instead you should solve it generally in FieldType::buildView() by accessing the "read_only" attribute of the parent view object (because only here we already know the specific form hierarchy).

helmer · 2012-02-02T13:14:16Z

@bschussek Sorry, but the form/field/view relation and hierarchy is a bit beyond me. After dumping around for an hour I still cannot quite grasp it. Views with no parent take their read_only value from form attribute, views with parent should take it from parent view options. Still, the following stripped down version does not work and I'm suspecting some magic:

FieldType::buildView

if ($view->hasParent()) {
    $readOnly = $view->getParent()->get('read_only');
} else {
    $readOnly = $form->getAttribute('read_only');
}

This does not really make sense to me and I cannot invest more time in it currently.
PS: Asking parent form read_only attribute works just fine, except it bypasses your suggestion and my logic.
PS2: While there, translation_domain should also be inherited similarly?

webmozart · 2012-02-02T13:49:09Z

You're basically there :)

$readOnly = $form->getAttribute('read_only');

if ($view->hasParent()) {
    $readOnly = $readOnly || $view->getParent()->get('read_only');
}

Tobion · 2012-02-02T14:04:10Z

src/Symfony/Component/Form/FormBuilder.php

@@ -174,27 +174,27 @@ public function getData()
    }

    /**
-     * Set whether the form is read only
+     * Set whether the form is disabled


missing dot

helmer · 2012-02-02T14:29:05Z

@bschussek Fixed, please check. As for the translation_domain, it cannot be solved the same way and I couldn't quite figure how to do it atm.
@Tobion Very good point!

stof · 2012-02-02T15:39:00Z

the translation domain is already passed to the radio fields in the choice type: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php#L199

helmer · 2012-02-02T15:52:53Z

@stof True, though bschussek said it is not most correct way to do it, so it was rather a small refactor request.

Why I mentioned it is because Im a bit concerned - ie the logic of "disabled" fields is taken from "Form" class (in which resides the inheritance logic as well), whereas "readonly" inheritance is handled locally and not on "Form" object. Same as "disabled" goes for "required" .. it is all just a tad messy imo and takes time to comprehend. Perhaps a critical view on how the options are handled and what classes hold the logic and how to reduce the amount of complexity is necessary?

Or .. maybe its just me :)

fabpot · 2012-02-02T15:54:07Z

@helmer: Can you open a PR referencing this one so that we do not loose track of your work (as this PR is closed now)? Thanks.

stof · 2012-02-02T15:57:21Z

@helmer readonly is used only for the rendering. disabled is also used in the Form class to disable the binding (for a form and all its children). It is not just a rendering variable

helmer · 2012-02-02T16:17:31Z

@fabpot Yupp, in a sec.
@stof I understand, still its currently on the complexity level of brain surgery and requires alot of investigating, as to how things (field attributes) get passed on etc, Im just suggesting that perhaps it can be simplified and generalized.

stof · 2012-02-02T16:20:04Z

the point is that all options don't have the same effect in a form. They are not all used for the same sort of things

helmer · 2012-02-02T16:23:51Z

@stof You are missing my point here. Lets take for example disabled attribute. The fact that Form is disabled when Field is disabled is true, but when building view parameters, shouldn't we actually rely on the initial options passed (as readonly works today) rather than query it from Form? The logic is scattered and is hard to follow ..

webmozart · 2012-02-02T16:31:56Z

@helmer Why is it hard to follow? If the whole is disabled, parts of it are disabled. If the whole is read only, parts of it are read only too. This way you can also easily set a whole form (or subform) to read only.

stof · 2012-02-02T16:33:22Z

@helmer if you take it only from the option, you will not disable the field when its parent is disabled, except if you handle the inheritance of the disabled state in 2 places.

@fabpot

Commits ------- 8321593 [Form] DRYed ChoiceType 0753cee [Form] Fixed read_only attribute for expanded fields Discussion ---------- [Form] Fixed read_only attribute for expanded fields Expanded choice widgets lose the knowledge of read_only attribute value. Fixes bug introduced by #3193 --------------------------------------------------------------------------- by helmer at 2012-02-02T16:24:50Z Please hold before merging, @bschussek had some thoughts about my changes in ``ChoiceType``, waiting for feedback. --------------------------------------------------------------------------- by bschussek at 2012-02-02T16:33:12Z I'm fine with the refactoring then, but please split it into two commits at least. The changes in ChoiceType have nothing in common with the actual issue here. --------------------------------------------------------------------------- by helmer at 2012-02-02T19:40:39Z Tests added. --------------------------------------------------------------------------- by bschussek at 2012-02-03T10:14:32Z Great, thanks. @fabpot 👍

This reverts commit e71d157, reversing changes made to 253a8ff. Conflicts: src/Symfony/Component/Form/Form.php src/Symfony/Component/Form/Tests/FormFactoryTest.php

Commits ------- de253dd [Form] read_only and disabled attributes Discussion ---------- [Form] read_only and disabled attributes (closes #1974) 1. Removed ``readOnly`` property from ``Form``, as it is no longer required 2. Introduced ``disabled`` property to ``Form``, behaves exactly like ``readOnly`` used to 3. Added ``disabled`` property to fields, defaults to ``false``, renders as ``disabled="disabled"`` 4. A field with positive ``read_only`` property now renders as ``readonly="readonly"`` --------------------------------------------------------------------------- by helmer at 2012-01-26T17:46:17Z I changed ``Form`` and ``FormBuilder`` property ``readOnly`` to ``disabled``. On second thought, this is perhaps not such good change - while readOnly somewhat implied the use-case, disabled no longer does. Perhaps something else, like ``bindable`` (as not to confuse with read_only attribute of Fields)? @bschussek, others, any thoughts? --------------------------------------------------------------------------- by bschussek at 2012-01-31T06:53:59Z Please prefix commits with the affected component, if applicable. --------------------------------------------------------------------------- by helmer at 2012-01-31T08:41:03Z @bschussek Prefixed. Please also see see to [this question](symfony/symfony#3193 (comment))

stof reviewed Jan 26, 2012
View reviewed changes

acasademont mentioned this pull request Jan 27, 2012

[Form] setReadOnly and setAttribute on a Form after creation #2797

Closed

helmer reviewed Jan 29, 2012
View reviewed changes

[Form] read_only and disabled attributes

de253dd

fabpot merged commit de253dd into symfony:master Feb 2, 2012

Tobion reviewed Feb 2, 2012
View reviewed changes

helmer mentioned this pull request Feb 2, 2012

[Form] Fixed read_only attribute for expanded fields #3258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Form] read_only and disabled attributes (closes #1974) #3193

[Form] read_only and disabled attributes (closes #1974) #3193

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Form] read_only and disabled attributes (closes #1974) #3193

[Form] read_only and disabled attributes (closes #1974) #3193

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!