Skip to content

BC break in MarkingStoreInterface #30524

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.

Sign up for GitHub

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

Jump to bottom
Closed
jenschude opened this issue Mar 11, 2019 · 5 comments
Closed

BC break in MarkingStoreInterface #30524

jenschude opened this issue Mar 11, 2019 · 5 comments
Labels
BC Break Bug Status: Needs Review Workflow

Comments

@jenschude
Copy link
Contributor

jenschude commented Mar 11, 2019 •
edited
Loading

Symfony version(s) affected: 4.3.0-dev

With the PR #29146 the MarkingStoreInterface has been changed. This is a BC break according to https://symfony.com/doc/current/contributing/code/bc.html#using-our-interfaces

How to reproduce

The issue can be seen here https://3v4l.org/ch3JX

Possible Solution

A possible fix is to introduce a new MarkingStoreInterface extending the old one with the newly added parameter. See https://3v4l.org/8C8Gc
@xabbuh
Copy link
Member

xabbuh commented Mar 11, 2019

I suggest we instead trigger a deprecation when an implementation of this interface does not have the second argument so that we can add it in 5.0.

@nicolas-grekas nicolas-grekas mentioned this issue Mar 11, 2019
Merged
@lyrixx
Copy link
Member

lyrixx commented Mar 12, 2019

@xabbuh How could I achieve that ? I mean, where could I put this code ? I need the reflection to do that I guess. It could lives in the framework bundle, but for people the component without the bundle, it will not work. Or I could put it in the component, but this comes with a slowdown of the component :/

@lyrixx lyrixx mentioned this issue Mar 12, 2019
Merged
@lyrixx
Copy link
Member

lyrixx commented Mar 12, 2019

See: #30530

@lyrixx
Copy link
Member

lyrixx commented Mar 12, 2019

I choose to use the reflection to cover all cases.

@jenschude Thanks for your feedback 👍

@jenschude
Copy link
Contributor Author

Actually kudos to @nikossvnk for finding the issue ;)

lyrixx added a commit that referenced this issue Mar 13, 2019
@lyrixx
bug #30530 [Workflow] Fixed BC break with `MarkingStoreInterface::set…
9305fe5 
…Marking()` (lyrixx)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Workflow] Fixed BC break with `MarkingStoreInterface::setMarking()`

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #30524
| License       | MIT
| Doc PR        |

Commits
-------

7a94e5e [Workflow] Fixed BC break with `MarkingStoreInterface::setMarking()`
@lyrixx lyrixx closed this as completed Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BC Break Bug Status: Needs Review Workflow
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@lyrixx @xabbuh @jenschude @carsonbot