Skip to content
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

[2.8] [Form] Rename CollectionType options for entries #15025

Merged
merged 1 commit into from
Oct 11, 2015

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jun 18, 2015

Description

Replaces #13820 for the 2.8 branch.

Original description:

type and options are extremely generic. Prefixing them with entry_ makes it clear what they are configuring.

About the property deprecation it is the same story as #13717 and I don't know which direction you want me to go.

I've tried to apply the comments in the previous PR, but got a bit lost in the normalizers/default closure stuff. I hope I did everything correctly, but please review :)

PR Info Table

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

@@ -52,12 +52,12 @@ class ResizeFormListener implements EventSubscriberInterface
*/
private $deleteEmpty;

public function __construct($type, array $options = array(), $allowAdd = false, $allowDelete = false, $deleteEmpty = false)
public function __construct($entryType, array $entryOptions = array(), $allowAdd = false, $allowDelete = false, $deleteEmpty = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revert the variable renames in this method? They don't really make sense now IMO.

@stof
Copy link
Member

stof commented Jun 18, 2015

We need a few legacy tests using the old option names

@webmozart webmozart added the Form label Jun 18, 2015
@fabpot
Copy link
Member

fabpot commented Oct 5, 2015

@wouterj We need to finish this one this week if we want to include it in 2.8.

@wouterj
Copy link
Member Author

wouterj commented Oct 10, 2015

@fabpot I'm sorry for the delay. fixed all comments & added some legacy tests. Should be ready to merge now

@@ -791,7 +791,7 @@ public function testSubmitSingleExpandedRequiredEmptyNoChoices()
$this->assertTrue($form->isSynchronized());
}

public function testSubmitSingleExpandedRequiredFalse()
public function testSubmiCollectiontSingleExpandedRequiredFalse()
Copy link
Member

Choose a reason for hiding this comment

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

missing t of Submit

Copy link
Member Author

Choose a reason for hiding this comment

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

This change seems like a mistake, reverted it

@wouterj
Copy link
Member Author

wouterj commented Oct 10, 2015

I cannot relate any of the build failures to changes in this PR

@fabpot
Copy link
Member

fabpot commented Oct 11, 2015

Thank you @wouterj.

@fabpot fabpot merged commit 942a237 into symfony:2.8 Oct 11, 2015
fabpot added a commit that referenced this pull request Oct 11, 2015
… (WouterJ)

This PR was merged into the 2.8 branch.

Discussion
----------

[2.8] [Form] Rename CollectionType options for entries

Description
---

Replaces #13820 for the 2.8 branch.

Original description:

 > `type` and `options` are extremely generic. Prefixing them with `entry_` makes it clear what they are configuring.

 > About the property deprecation it is the same story as #13717 and I don't know which direction you want me to go.

I've tried to apply the comments in the previous PR, but got a bit lost in the normalizers/default closure stuff. I hope I did everything correctly, but please review :)

PR Info Table
---

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

Commits
-------

942a237 Rename CollectionType options for entries
@wouterj wouterj deleted the issue_7831 branch October 11, 2015 09:33
@craue
Copy link
Contributor

craue commented Oct 13, 2015

I'm using type and expected to see the deprecation notice, but it's not triggered (when running tests with the PHPUnit bridge enabled).

weaverryan added a commit to symfony/symfony-docs that referenced this pull request Oct 15, 2015
This PR was submitted for the 2.7 branch but it was merged into the 2.8 branch instead (closes #5051).

Discussion
----------

Rename CollectionType entry options

| Q | A
| --- | ---
| Doc fix? | no
| New docs? | yes (symfony/symfony#15025)
| Applies to | 2.7+
| Fixed tickets | -

Commits
-------

1b00278 Rename CollectionType entry options
craue added a commit to craue/CraueConfigBundle that referenced this pull request Oct 20, 2015
@fabpot fabpot mentioned this pull request Nov 16, 2015
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.

6 participants