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

[Mailer] Changed EventDispatcherInterface dependency from Component to Contracts #31956

Merged
merged 1 commit into from Jun 11, 2019

Conversation

Projects
None yet
5 participants
@Koc
Copy link
Contributor

commented Jun 8, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? almost yes, see #31956 (comment)
Fixed tickets -
License MIT
Doc PR -

Follow up of #31946 (comment) . I hope this kind of changes are allowed for experimental components.

BTW, @nicolas-grekas , why Psr14 interface is optional for Contract's `EventDispatcherInterface https://github.com/symfony/symfony/blob/4.4/src/Symfony/Contracts/EventDispatcher/EventDispatcherInterface.php#L16 ?

@Koc Koc force-pushed the Koc:mailer-change-event-dispatcher-dependency branch from e3efc9e to 848fce2 Jun 8, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 8, 2019

@nicolas-grekas nicolas-grekas changed the title Changed EventDispatcherInterface dependency from Component to Contracts [Mailer] Changed EventDispatcherInterface dependency from Component to Contracts Jun 8, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Makes sense to me, don't miss bumping the bridge's lowest version in composer.json

why Psr14 interface is optional for Contract's `EventDispatcherInterface

technically: because we are >=PHP7.1 while PSR14 is >=7.2
also because PSR14 doesn't need to be a hard requirement.

@Koc Koc force-pushed the Koc:mailer-change-event-dispatcher-dependency branch from d39cada to bdb6217 Jun 9, 2019

@Koc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@nicolas-grekas I've updated minimum required versions for both Bridge -> Mailer and Mailer -> Bridge, but deps=high job still fails because current dev-master hasn't my changes. How can I fix that or should we just ignore this failed job?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

How can I fix that or should we just ignore this failed job?

deps=high can fail in this situation, until the patch is merged to master.

@fabpot

fabpot approved these changes Jun 11, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Thank you @Koc.

@fabpot fabpot merged commit bdb6217 into symfony:4.4 Jun 11, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 11, 2019

feature #31956 [Mailer] Changed EventDispatcherInterface dependency f…
…rom Component to Contracts (Koc)

This PR was merged into the 4.4 branch.

Discussion
----------

[Mailer] Changed EventDispatcherInterface dependency from Component to Contracts

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | almost yes, see #31956 (comment)
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Follow up of #31946 (comment) . I hope this kind of changes are allowed for experimental components.

BTW, @nicolas-grekas , why Psr14 interface is optional for Contract's `EventDispatcherInterface https://github.com/symfony/symfony/blob/4.4/src/Symfony/Contracts/EventDispatcher/EventDispatcherInterface.php#L16 ?

Commits
-------

bdb6217 Changed EventDispatcherInterface dependency from Component to Contracts
4.4.0
-----

* [BC BREAK] Transports depend on `Symfony\Contracts\EventDispatcher\EventDispatcherInterface`

This comment has been minimized.

Copy link
@stof

stof Jun 11, 2019

Member

that's not actually a BC break if the component interface extends the contracts one.

This comment has been minimized.

Copy link
@Koc

Koc Jun 11, 2019

Author Contributor

Interface from component has methods like addListener, addSubscriber but interface from contracts has only dispatch method. So if some of transports call addListener - now it could get fatal error.

@Koc Koc deleted the Koc:mailer-change-event-dispatcher-dependency branch Jun 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.