Skip to content

[Form] Fix FormEvents::* constant and value matching #24631

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

Merged
merged 3 commits into from
Oct 19, 2017

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Oct 19, 2017

Q A
Branch? 4.0
Bug fix? no
New feature? no
BC breaks? yes (ppl relying on const value directly, very weird)
Deprecations? no
Tests pass? yes
Fixed tickets #24615
License MIT
Doc PR -

#24615 (comment) by @stof:
Yeah, I think we could change this in 4.0 without a big impact (btw, I think our BC policy even allows it for this case).
There is one case where people will use the event name rather than the constant: the kernel.event_listener tag (and recent versions can even use the constant in YAML files). But this won't be the case for this event, as form events are not dispatched in the main dispatcher anyway.

@dmaicher
Copy link
Contributor

Should this be mentioned inside UPGRADE-4.0.md?

@yceruto
Copy link
Member Author

yceruto commented Oct 19, 2017

Thanks @dmaicher, done.

@yceruto yceruto force-pushed the formevents branch 3 times, most recently from fe514e0 to 0ee856a Compare October 19, 2017 17:51
@yceruto
Copy link
Member Author

yceruto commented Oct 19, 2017

Thanks a lot @javiereguiluz, much better! :)

* The values of the `FormEvents::*` constants have been updated to match the
constant names. You should only update your application if you relied on the
constant values instead of their names.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause UPGRADE-4.0 in master and 3.4 going out of sync no? Which btw is already happening (https://www.diffchecker.com/5W6GeCnZ).. again :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

So the upgrade entry should be updated in 3.4 too?

Copy link
Member

Choose a reason for hiding this comment

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

yes, it should be synchronized

@fabpot
Copy link
Member

fabpot commented Oct 19, 2017

Thank you @yceruto.

@fabpot fabpot merged commit 944931a into symfony:master Oct 19, 2017
fabpot added a commit that referenced this pull request Oct 19, 2017
…yceruto, javiereguiluz)

This PR was merged into the 4.0-dev branch.

Discussion
----------

[Form] Fix FormEvents::* constant and value matching

| Q             | A
| ------------- | ---
| Branch?       | 4.0
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes (ppl rely on const value directly, very weird)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24615
| License       | MIT
| Doc PR        | -

> #24615 (comment) by @stof:
Yeah, I think we could change this in 4.0 without a big impact (btw, I think our BC policy even allows it for this case).
There is one case where people will use the event name rather than the constant: the kernel.event_listener tag (and recent versions can even use the constant in YAML files). But this won't be the case for this event, as form events are not dispatched in the main dispatcher anyway.

Commits
-------

944931a Minor reword
0ee856a Update UPGRADE-4.0.md
0fc2282 fix the constant value to be consistent with the name
@yceruto yceruto deleted the formevents branch October 19, 2017 18:53
fabpot added a commit that referenced this pull request Oct 19, 2017
This PR was merged into the 3.4 branch.

Discussion
----------

Synchronizing Upgrade 4.0 notes in 3.4

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24631 (comment)
| License       | MIT
| Doc PR        | -

Commits
-------

c4f64a6 Update UPGRADE-4.0.md
@fabpot fabpot mentioned this pull request Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants