[Form] Deprecated setDefaultOptions() in favor of configureOptions() #12891

peterrehm · 2014-12-08T07:12:08Z

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

This tries to provide a compatible API with the depreciation of the OptionResolverInterface. I would like to have this in 2.6.2 but I think that might not be possible? To me I think we should always provide an API where you do not need to use deprecated classes.

Also can you think of any way to trigger errors on the use of the deprecated setDefaultOptions() method? Since it is usually overwritten without calling the parent class this might be tricky. Maybe only in the resolver if we can check if actual options has been resolved in a call to setDefaultOptions.

stof · 2014-12-10T08:39:50Z

Your proposal is not BC

peterrehm · 2014-12-10T08:43:44Z

@stof What breaks BC?

stof · 2014-12-10T09:04:44Z

src/Symfony/Component/Form/Tests/ResolvedFormTypeTest.php

+    public function testGetOptionsResolverWithConfigureOptions()
+    {
+        if (version_compare(\PHPUnit_Runner_Version::id(), '3.7', '<')) {
+            $this->markTestSkipped('This test requires PHPUnit 3.7.');


This is useless. The Symfony testsuite already requires 4.2+ anyway

It is useless, but there are several of those conditions in this test class.

I have just copied it from the test class with the old configuration name.

Well, checking for <3.7 was useful 4 years ago, when 3.7 was just released and many people were still running 3.6. Now, it is not

Shall I remove it in the entire class? Can you provide some more information regarding BC breaks you mentioned?

We removed all those skips recently, please remove this one also.

peterrehm · 2014-12-10T13:49:23Z

@webmozart What is your opinion regarding this?

peterrehm · 2015-01-04T11:06:01Z

Any opinion on this PR?

stof · 2015-01-06T01:41:00Z

src/Symfony/Component/Form/AbstractType.php

+     *
+     * @param OptionsResolver $resolver The resolver for the options.
+     */
+    public function configureOptions(OptionsResolver $resolver)


setDefaultOptions must call configureOptions to be BC

It is BC compatible hence the checks for configureOptions in ResolvedFormType. See also my comments there.

stof · 2015-01-06T01:45:51Z

@fabpot I think this is a valuable change:

setDefaultOptions is a weird name given that the method is not only about setting the default options, but about configuring the whole OptionsResolver (the name was probably get close of the Symfony 2.0 getDefaultOptions even though the responsibility was larger)
using a different method names allows us to build a forward-compatibility layer for the interface deprecation.

fabpot · 2015-01-06T06:57:04Z

Indeed, this looks like a very good change.

@peterrehm Besides the comments from @stof, you must add the deprecation notices where applicable before we can merge this PR. There are some other things to take care of like moving tests for the old version into "legacy" tests, but I will let @nicolas-grekas comment on this.

nicolas-grekas · 2015-01-06T07:23:45Z

As @fabpot said, please trigger deprecations notices when the deprecated methods are used (see #13060).
Please also update any usage of the deprecated methods everywhere in the code base,
please verify also that composer.json files state correct minimum requirements (if the new methods are used cross-components)
last but not least, if tests remain that exercise the deprecated methods, please isolate them in test methods or classes with testLegacy (for methods) of Legacy*Test (for classes) so that we can easily drop them in 3.0.
I didn't look at the details of your PR, so maybe not all of this is applicable here, but this is the general process.

stof · 2015-01-06T08:19:24Z

to be clear, the case needing to throw a notice is when a type is overwriting setDefaultOptions instead of overwriting configureOptions. The code using the types should still call the setDefaultOptions method because we cannot change the interface until 3.0 (meaning we don't have a BC layer here, but a forward compatibility layer)

peterrehm · 2015-01-06T15:30:04Z

I have just added a way to check if the method has been overwritten in the ResolvedFormType.
That is the only way I can think of, however I do not know about the performance implications of this solution. What do you think about that?

stof · 2015-01-06T15:34:53Z

src/Symfony/Component/Form/ResolvedFormType.php

@@ -205,8 +205,30 @@ public function getOptionsResolver()

            $this->innerType->setDefaultOptions($this->optionsResolver);

+            $reflector = new \ReflectionMethod($this->innerType, 'setDefaultOptions');
+            $isOverwritten = ($reflector->getDeclaringClass()->getName() !== 'AbstractType');


This will never work, because the class name is not AbstractType. So it will always be reported as overwritten

Refactored with the FQN.

peterrehm · 2015-01-06T15:35:52Z

Most likely due to my local squash the comment by @stof got lost:

if someone does not use the AbstractType, they are required to implement setDefaultOptions because of the interface.

IMO, we should stick to the interface for the external usage of the FormTypeInterface, and build the forward-compatibility layer only in AbstractType, by making setDefaultOptions call configureOptions. This will not break applications

If someone uses the FormTypeInterface without AbstractType, they will have a hard BC break for 3.0 when we will replace the method in the interface, but I haven't seen any code recreating the implementation from scratch rather than extending the base class with empty implementations.

peterrehm · 2015-01-06T15:46:32Z

I have done a rewrite as per @stof's comment in a second commit. It could potentially break if someone has a custom OptionResolver implementation which is i assume unlikely. What do you think now?

stof · 2015-01-06T15:53:13Z

It could potentially break if someone has a custom OptionResolver implementation which is i assume unlikely.

This is totally unlikely (which is why the interface is deprecated btw) and even more in the case of forms given that the OptionsResolver is instantiated by Symfony

stof · 2015-01-06T15:54:18Z

src/Symfony/Component/Form/AbstractType.php

+    }
+
+    /**
+     * Configure the options for this type.


typo: Configures

peterrehm · 2015-01-06T15:55:08Z

Yes I just wanted to have it mentioned.

stof · 2015-01-06T15:55:22Z

UPGRADE-3.0.md

+ * The method `AbstractType::setDefaultOptions(OptionsResolverInterface $resolver)` and
+   `AbstractTypeExtension::setDefaultOptions(OptionsResolverInterface $resolver)` have been
+   removed. You should use `AbstractType::configureOptions(OptionsResolver $resolver)` and
+   `AbstractTypeExtension::configureOptions(OptionsResolver $resolver)` instead.


the actual change for 3.0 is renaming the method in the interfaces

But it is not only renamed, we change the signature as well. Shall i change it?

well, your message shows the typehint. Just keep it when showing the renaming

stof · 2015-01-08T08:38:42Z

for the components=high, this is because the subtree split of the Form component obviously does not contain the change for configureOptions yet. It should be fixed once the PR is merged and subtrees get updated.

For components=low, this is a combination of 2 things:

the subtrees for 2.7 are not updated
the low bound for the symfony/form constraints in other components should be bumped to 2.7 as they need the support for this new method, which is not there in 2.6 and older

peterrehm · 2015-01-09T07:36:04Z

Thanks for the explanation, that makes it clear. Then I am good from my side.

stof · 2015-01-09T11:22:12Z

no you're not. You need to bump the lower bound for the Form component in other components/bridges where you switched to configureOptions

peterrehm · 2015-01-09T16:14:40Z

I have adjusted the version requirements, Is this as you expected it?

peterrehm · 2015-01-10T11:05:39Z

Rebased. Now it looks like just the Tests in the Doctrine and Propel bridge are failing, I assume this is due to the missing subtree split.

wouterj · 2015-01-10T12:40:19Z

UPGRADE-3.0.md

@@ -106,6 +106,11 @@ UPGRADE FROM 2.x to 3.0

 ### Form

+ * The method `AbstractType::setDefaultOptions(OptionsResolverInterface $resolver)` and
+   `AbstractTypeExtension::setDefaultOptions(OptionsResolverInterface $resolver)` have been
+   renamed. You should use `AbstractType::configureOptions(OptionsResolver $resolver)` and


Isn't the standard to use something like: "... have been deprected in favor of ..."

Wait, sorry. this is the upgrade file. Maybe add a before/after example?

@wouterj I think it is fine as is since before/after is already in the text.

peterrehm · 2015-01-13T17:55:54Z

@fabpot Is there anything blocking this?

stof · 2015-01-13T18:02:47Z

src/Symfony/Component/Form/CHANGELOG.md

+2.7.0
+-----
+
+ * deprecated AbstractType::setDefaultOptions() in favor of AbstractType::configureOptions().


actually, what has been deprecated is overriding these methods

setDefaultOptions() has ben deprecated and the new method exists as preparation for the rename in 3.0.

no. the method itself is not deprecated (it is not possible as it must be implemented in form types and is being used.
What is deprecated is overwriting AbstractType::setDefaultOptions(), in favor of overwriting AbstractType::configureOptions(), to use a forward-compatibility layer.

This is not a BC layer here. We are making it possible to write your code as if you were using the Symfony 3 interface already, even though it is not the case yet, handling the differences in the parent class. This is not the same than providing a new API and deprecating the BC layer

Well I still consider this as deprecated. Do you want me to change the wording?

I would vote for updating the wording

@stof updated as per your request.

stof · 2015-01-13T18:03:19Z

The UPGRADE-2.7 file should mention the deprecation and describe the way to upgrade

peterrehm · 2015-01-13T18:27:57Z

@stof Added the upgrade information

peterrehm · 2015-01-14T18:18:10Z

@stof Anything else left?

stof · 2015-01-15T16:22:35Z

src/Symfony/Bridge/Twig/composer.json

@@ -22,7 +22,7 @@
    },
    "require-dev": {
        "symfony/finder": "~2.3|~3.0.0",
-        "symfony/form": "~2.6|~3.0.0",
+        "symfony/form": "~2.7|~3.0.0",


this is not needed actually AFAIK. The Twig bridge does not contain a form type

peterrehm · 2015-01-16T06:48:22Z

@stof Updated as per your comments.

fabpot · 2015-01-16T08:14:36Z

@peterrehm Tests do not pass (some problems on Propel seems related to this PR). Also, if not done already, rebase on current master. Thanks.

peterrehm · 2015-01-16T09:32:33Z

@fabpot the only failed tests are on components high and low. @stof mentioned that this is due to a missing subtree split and should be fixed after the merge. I rebased against 2.7, I assume this is what you meant. After this is merge I will update the corresponding PR for the master branch.

peterrehm · 2015-01-17T17:25:02Z

@fabpot So is it okay now?

fabpot · 2015-01-18T14:06:56Z

Thank you @peterrehm.

…igureOptions() (peterrehm) This PR was merged into the 2.7 branch. Discussion ---------- [Form] Deprecated setDefaultOptions() in favor of configureOptions() | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | | Fixed tickets | #12782 | License | MIT | Doc PR | symfony/symfony-docs#4786 This tries to provide a compatible API with the depreciation of the OptionResolverInterface. I would like to have this in 2.6.2 but I think that might not be possible? To me I think we should always provide an API where you do not need to use deprecated classes. Also can you think of any way to trigger errors on the use of the deprecated setDefaultOptions() method? Since it is usually overwritten without calling the parent class this might be tricky. Maybe only in the resolver if we can check if actual options has been resolved in a call to setDefaultOptions. Commits ------- 3d43cae Deprecated setDefaultOptions() in favor of configureOptions()

peterrehm · 2015-01-18T14:32:24Z

Perfect. Once you merge 2.7 into master I will update #13407.

…method (peterrehm) This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #4786). Discussion ---------- Replaced setDefaultOptions by the new configureOptions method | Q | A | ------------- | --- | Doc fix? | yes | New docs? | yes|no (PR symfony/symfony#12891) | Applies to | 2.7 | Fixed tickets | #4565 Pending on the merge of symfony/symfony#12891. With this PR all references to setDefaultOptions and the deprecated OptionResolverInterface are removed in 2.7. Commits ------- 728205f Replaced setDefaultOptions by the new configureOptions method

…ResolverInterface (peterrehm) This PR was merged into the 3.0-dev branch. Discussion ---------- [Form] Remove deprecated setDefaultOptions and OptionsResolverInterface | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#4786 | Related | #12891 Commits ------- 9b9158b Remove the deprecated OptionsResolverInterface 6026781 Removed deprecated setDefaultOptions methods

peterrehm changed the title ~~Deprecated setDefaultOptions() in favor of configureOptions()~~ [Form] Deprecated setDefaultOptions() in favor of configureOptions() Dec 8, 2014

peterrehm mentioned this pull request Dec 8, 2014

Remove deprecated OptionsResolverInterface references from documentation symfony/symfony-docs#4565

Closed

stof reviewed Dec 10, 2014
View reviewed changes

fabpot added the Form label Dec 18, 2014

xabbuh mentioned this pull request Jan 4, 2015

[Form] [3.0] Remove deprecated OptionsResolverInterface #12909

Closed

stof reviewed Jan 6, 2015
View reviewed changes

src/Symfony/Component/Form/AbstractType.php

}

/**

* Configure the options for this type.

Copy link

Member

stof Jan 6, 2015

Choose a reason for hiding this comment

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

typo: Configures

stof reviewed Jan 6, 2015
View reviewed changes

fabpot force-pushed the 2.7 branch from 77b6cca to dcfc338 Compare January 8, 2015 13:06

wouterj reviewed Jan 10, 2015
View reviewed changes

stof reviewed Jan 13, 2015
View reviewed changes

peterrehm mentioned this pull request Jan 13, 2015

[Form] Remove deprecated setDefaultOptions and OptionsResolverInterface #13407

Merged

stof reviewed Jan 15, 2015
View reviewed changes

Deprecated setDefaultOptions() in favor of configureOptions()

3d43cae

fabpot merged commit 3d43cae into symfony:2.7 Jan 18, 2015

peterrehm deleted the abstract-type branch January 18, 2015 14:18

thewilkybarkid mentioned this pull request Apr 9, 2015

Cater for Symfony Form/Validator deprecations misd-service-development/phone-number-bundle#51

Merged

jakzal mentioned this pull request Jun 2, 2015

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

Closed

[Form] Deprecated setDefaultOptions() in favor of configureOptions() #12891

[Form] Deprecated setDefaultOptions() in favor of configureOptions() #12891

Conversation

peterrehm commented Dec 8, 2014

stof commented Dec 10, 2014

peterrehm commented Dec 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterrehm commented Dec 10, 2014

peterrehm commented Jan 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jan 6, 2015

fabpot commented Jan 6, 2015

nicolas-grekas commented Jan 6, 2015

stof commented Jan 6, 2015

peterrehm commented Jan 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterrehm commented Jan 6, 2015

peterrehm commented Jan 6, 2015

stof commented Jan 6, 2015

Choose a reason for hiding this comment

peterrehm commented Jan 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jan 8, 2015

peterrehm commented Jan 9, 2015

stof commented Jan 9, 2015

peterrehm commented Jan 9, 2015

peterrehm commented Jan 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterrehm commented Jan 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jan 13, 2015

peterrehm commented Jan 13, 2015

peterrehm commented Jan 14, 2015

Choose a reason for hiding this comment

peterrehm commented Jan 16, 2015

fabpot commented Jan 16, 2015

peterrehm commented Jan 16, 2015

peterrehm commented Jan 17, 2015

fabpot commented Jan 18, 2015

peterrehm commented Jan 18, 2015