Skip to content

[Form] deprecate read_only option #14403

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

Closed
wants to merge 5 commits into from
Closed

Conversation

Tobion
Copy link
Contributor

@Tobion Tobion commented Apr 18, 2015

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #10658
License MIT
Doc PR symfony/symfony-docs#3782

Replaces #10676 with a slightly different implementation.

  • fixes BC break when 'read_only' => true option and at the same time custom attributes are used by using a normalizer
  • adds deprecation notice
  • keeps legacy tests

@Tobion Tobion force-pushed the deprecate-read-only branch from f1ea295 to 83db940 Compare April 19, 2015 00:50
@Tobion Tobion changed the title [WIP][Form] deprecate read_only option [Form] deprecate read_only option Apr 19, 2015
@Tobion Tobion force-pushed the deprecate-read-only branch from 6d6edaa to 40f4440 Compare April 19, 2015 01:41
@Tobion
Copy link
Contributor Author

Tobion commented Apr 19, 2015

@symfony/deciders ready


$readOnlyNormalizer = function (Options $options, $readOnly) {
if (null !== $readOnly) {
trigger_error('The form option "read_only" is deprecated since version 2.7 and will be removed in 3.0. Use "attr[\'readonly\']" instead.', E_USER_DEPRECATED);
Copy link
Contributor

Choose a reason for hiding this comment

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

Must be 2.8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@xabbuh
Copy link
Member

xabbuh commented Apr 19, 2015

Shouldn't we bump the dev dependency for the Form component in the FrameworkBundle to ~2.8. We now have failing tests for using the deprecated read_only option because it uses an outdated version of the AbstractLayoutTest.

@Tobion
Copy link
Contributor Author

Tobion commented Apr 19, 2015

yes

@Tobion
Copy link
Contributor Author

Tobion commented Apr 20, 2015

Fixed

@xabbuh xabbuh added the Form label Apr 21, 2015
@webmozart
Copy link
Contributor

Looks good! 👍

@fabpot
Copy link
Member

fabpot commented May 20, 2015

Thank you @Tobion.

@fabpot fabpot closed this May 20, 2015
fabpot added a commit that referenced this pull request May 20, 2015
This PR was squashed before being merged into the 2.8 branch (closes #14403).

Discussion
----------

[Form] deprecate read_only option

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #10658
| License       | MIT
| Doc PR        | symfony/symfony-docs#3782

Replaces #10676 with a slightly different implementation.
- fixes BC break when 'read_only' => true option and at the same time custom attributes are used by using a normalizer
- adds deprecation notice
- keeps legacy tests

Commits
-------

53330e1 [Form] deprecate read_only option
@Tobion Tobion deleted the deprecate-read-only branch May 20, 2015 11:36
@fabpot fabpot mentioned this pull request Nov 16, 2015
));

$html = $this->renderWidget($form->createView());

// compare plain HTML to check the whitespace
$this->assertSame('<input type="text" id="text" name="text" readonly="readonly" disabled="disabled" required="required" maxlength="10" pattern="\d+" class="foobar" data-foo="bar" value="value" />', $html);
$this->assertSame('<input type="text" id="text" name="text" disabled="disabled" required="required" readonly="readonly" maxlength="10" pattern="\d+" class="foobar" data-foo="bar" value="value" />', $html);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand, this doesn't preserve BC here? The test was changed to support the new way (through attributes), but the old way is not tested anymore.

This created a BC break for us when migrating from 2.7 to 2.8. We have to switch from the read_only variable to the attr key.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mnapoli what exactly is breaking except the position in the string ? One would just expect to get the attribute here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Before (2.7) passing read_only as a variable was working, after (2.8) it isn't. We had to put readonly in the attr array instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the change we had to do:

 {% block _form_category_name_widget %}
-    {% set read_only = true %}
     {% set attr = {
         'data-target-show': '.choose-categories',
+        'readonly': true,
     } %}
     {{ block('form_widget_simple') }}
{% endblock %}

HeahDude added a commit to HeahDude/symfony that referenced this pull request Mar 23, 2016
HeahDude added a commit to HeahDude/symfony that referenced this pull request Mar 23, 2016
HeahDude added a commit to HeahDude/symfony that referenced this pull request Mar 25, 2016
fabpot added a commit that referenced this pull request Mar 27, 2016
This PR was merged into the 2.8 branch.

Discussion
----------

[Form] Fix BC break introduced in #14403

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | -

Commits
-------

5f48c6a [Form] Fix BC break introduced in #14403
fabpot added a commit that referenced this pull request Mar 27, 2016
* 2.8:
  improved comment
  [FileSystem] Add support for Google App Engine.
  [2.8] fix mocks
  [Form] Fix BC break introduced in #14403
fabpot added a commit that referenced this pull request Mar 27, 2016
This reverts commit 56fa798, reversing
changes made to dcc3fce.
This was referenced Mar 27, 2016
fabpot added a commit that referenced this pull request Mar 27, 2016
* 3.0:
  bumped Symfony version to 2.8.5
  updated VERSION for 2.8.4
  updated CHANGELOG for 2.8.4
  Revert "bug #18275 [Form] Fix BC break introduced in #14403 (HeahDude)"
  improved comment
  [FileSystem] Add support for Google App Engine.
  [2.8] fix mocks
  bumped Symfony version to 2.7.12
  [Form] Fix BC break introduced in #14403
  updated VERSION for 2.7.11
  updated CHANGELOG for 2.7.11
  [Request] Fix support of custom mime types with parameters
  fix mocks
  fix mocks
  [Validator] do not treat payload as callback
@fabpot fabpot mentioned this pull request Mar 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants