[Messenger] Move Transports to separate packages #35422

Nyholm · 2020-01-21T16:16:09Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
Deprecations?	no
Tickets
License	MIT
Doc PR	coming

I think it is a good idea to have the transports in a separate package. The benefits a many:

It allows us to have usage statistics
The core messenger package is smaller
Transports can have dependencies specified in composer.json instead of just suggests

This PR will not break BC but it requires to configure subtree split.

src/Symfony/Component/Messenger/composer.json

fabpot · 2020-01-22T07:11:37Z

I would do it the same way as with the Mailer and Notifier in terms of namespaces. That would mean creating aliases between the old and the new namespaces, but I think it's worth.

src/Symfony/Component/Messenger/composer.json

Nyholm · 2020-01-22T09:41:25Z

I've updated the PR:

I moved the 3 transports to Symfony\Component\Messenger\Bridge.
I added class aliases
I added deprecation notices

Tests passes on all 4 packages. I've been careful and I think I've done everything correct.

Nyholm · 2020-01-22T09:45:39Z

Oh, no.. The big diff looks awful. It might be easier to review this PR commit by commit.

Added meta files: 9f56b5a
Moved all the transports: cd335b1
Added class aliases: e96e55e

src/Symfony/Component/Messenger/Transport/AmqpExt/AmqpFactory.php

ro0NL · 2020-01-22T14:20:39Z

src/Symfony/Component/Messenger/composer.json

+        "symfony/messenger-amqp": "^5.1",
+        "symfony/messenger-doctrine": "^5.1",
+        "symfony/messenger-redis": "^5.1"


these requires kinda defeat the need for separate packages IMHO.

We should not add them IMHO, as we can never remove them because of BC.

Alternatively keep the legacy transport in place, and let the new bridges extend from those meanwhile.

We can (and will) remove them in 6.0.

These transports should have been in Bridge namespace in the first place. I moving these now, since we are about to add more transports.

so i can require symfony/messenger, while using this new symfony/messenger-amq transport, then updating to 6.0 my transport is removed .. and it crashes?

see #29229 which triggered me. Nevertheless, maybe we should do it in this case.

You are correct.

Assuming you ignored deprecation messages, the changelog and the UPGRADE docs.

(I just added upgrade docs)

im considering new users, they require symfony/messenger tomorrow and are able to start using the new transports wihout any hint they should actually require it separately.

if we can remove those requires in 6.0 then it's fine, but im worried in Nov 2021 we'll consider it a BC break. In this case, not adding them now would avoid that.

If you require symfony/messenger tomorrow and start using the transports, you will get a deprecation warning. The deprecation warnings are actually really nice:

The "Symfony\Component\Messenger\Transport\Doctrine\DoctrineTransportFactory" class is deprecated since Symfony 5.1, use "Symfony\Component\Messenger\Bridge\Doctrine\Transport\DoctrineTransportFactory" instead. The Doctrine transport has been moved to package "symfony/messenger-doctrine" and will not be included by default in 6.0. Run "composer require symfony/messenger-doctrine".

So the deprecation layer is there - and (unless you ignore the deprecation warnings) you'll know to require the individual packages.

are you sure @weaverryan? It triggers the deprecation only if you load the legacy class. Which is not the case for a simple usage when you let the Bundle loading the transport (see changes in src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.xml).

For a migration path shouldn't we:

let code in symfony/messenger

create a class in symfony/bridge-transport that reuse the code (trait or extends) (not an issue as the bridge already requires symfony/messenger)

update the bundle to use symfony/bridge-transport class when exist, and fallback to symfony/messenger

trigger a deprecation when people use class from symfony/messenger

weaverryan

I just played with this code using our (fairly advanced) SfCasts Messenger tutorial and it worked flawlessly. The deprecation warnings are really nice - each instructing me which package to require to fix the deprecation. I can't test requiring the new packages yet (since they don't exist), but that's easy to try after merge and creating the sub-tree splits.

One missing piece I discussed with Tobias (and he agreed, but for a different PR) is to add code to suggest (with an exception) that you require a specific package if you try to use a "core" transport. We should add that "now" (but in a future PR), though users won't see this message until 6.0 when they try to use a transport that's not installed.

+1 from me - I can't find any issues.

src/Symfony/Component/Messenger/Bridge/AmqpExt/CHANGELOG.md

UPGRADE-6.0.md

Nyholm · 2020-01-24T16:18:43Z

I moved class_alias to the new class after discussions with Nicolas.

The following does not work:

// FILE: old.php
namespace Old;
class_alias(\New\Foo::class, \Old\Foo::class);

if (false) {
    class Foo{}
}

// FILE: new.php
namespace New;
class Foo{}

// FILE: index.php
function foo(\Old\Foo $old) {};

// This will break. 
foo(new \New\Foo());

We need to move the class_alias to the new.php.

// FILE: old.php
namespace Old;
// Load the new class
class_exists(\New\Foo::class);

if (false) {
    class Foo{}
}

// FILE: new.php
namespace New;
class Foo{}
class_alias(Foo::class, Old\Foo::class);

// FILE: index.php
function foo(\Old\Foo $old) {};

// This will work
foo(new {New\Foo());

nicolas-grekas

The BC layer looks good enough for me.
Thank you.

src/Symfony/Component/Messenger/Bridge/AmqpExt/composer.json

Nyholm · 2020-01-24T20:33:27Z

Thank you for all the reviews and carefully thinking out the BC layer.

src/Symfony/Component/Messenger/Bridge/AmqpExt/composer.json

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.xml

...Symfony/Component/Messenger/Bridge/AmqpExt/Middleware/RejectRedeliveredMessageMiddleware.php

Tobion · 2020-01-26T21:45:24Z

The transports are currently inside the bridge folder of the symfony/messenger package. But for 6.0 we don't want to include the transports automatically. But I don't see how that is going to work without moving the files again? Shouldn't we create new packages outside the symfony/messenger for this to be equipped for 6.0? Otherwise symfony/messenger is a meta-package like symfony/security used to be, which is not what we want.
We still require the transports for BC from symfony/messenger, but the files should not be located there.

So ideally some structure like

Symfony/Component/Messenger/
- Core/
- Bridge/AmqpExt
- Bridge/RedisExt

The problem is only that Core is not part of the messenger namespace and would require some autoloading adaptions for symfony/symfony. Another alternative would be to move the transports to Symfony/Bridge/... namespace like Symfony/Bridge/MessengerAmqpExt

Nyholm · 2020-01-27T08:15:11Z

For what I understand, we can configure subtreesplit so it works as expected. Just like with the Notifier and Mailer component.

So we dont need to move the files again.

Tobion · 2020-01-27T12:49:04Z

I see. If it works like in mailer that's fine for me. I just think it's not an ideal setup because people will not realize that the Bridges are not part of symfony/messenger from reading the source of symfony/symfony on Github. I also didn't expect that subtree splits delete files.

fabpot

I'm not a big fan of the names: why not Amqp instead of AmqpExt? Same for Redis. The DSN use redis and amqp anyway, so I don't see a good reason to use Ext in the namespace (and IIRC, we are doing it anywhere else).

Nyholm · 2020-01-28T08:21:23Z

Sure. I'll rename it. If we want to support the amqp-lib in a separate transport, it could be named AmqpLib.

Nyholm · 2020-01-28T08:52:31Z

The namespace is renamed, and the tests are green.

fabpot · 2020-01-28T08:57:16Z

Thank you @Nyholm.

Nyholm · 2020-01-28T09:06:22Z

Thank you for merging and thank you for all reviews.

Tobion · 2020-07-20T16:52:38Z

This broke BC for the stamps, see #37616

stof · 2020-07-20T17:09:39Z

UPGRADE-5.1.md

+---------
+
+ * Deprecated AmqpExt transport. It has moved to a separate package. Run `composer require symfony/amqp-messenger` to use the new classes.
+ * Deprecated Doctrine transport. It has moved to a separate package. Run `composer require symfony/doctrine-messenger` to use the new classes.


some nitpick. I would rename this one to doctrine-dbal-messenger. The Doctrine organization has many separate projects related to persistence. This bridge integrates with Doctrine DBAL, not with the whole Doctrine projects (having a single bridge for the whole Doctrine makes the dependency management a pain for symfony/doctrine-bridge btw, so if we ever have another transport using a different Doctrine project, I'd rather have a separate bridge for it)

…alling symfony/messenger (ogizanagi) This PR was merged into the 5.2-dev branch. Discussion ---------- [Messenger] Don't require doctrine/persistence when installing symfony/messenger | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no  | Deprecations? | no  | Tickets | Fix #36790  | License | MIT | Doc PR | N/A As of today, [installing `symfony/messenger` installs `symfony/doctrine-messenger`](https://github.com/symfony/symfony/blob/3867fb2489ef0d07dec4859cf461bb7bcc7618e0/src/Symfony/Component/Messenger/composer.json#L23) as well. Which means adding `doctrine/persistence` as a hard dep of the latest will install it for any user requiring `symfony/messenger` in 5.2. I think it should stay a soft-dependency until we remove `symfony/doctrine-messenger` as a `symfony/messenger` dependency as of Symfony 6.0 (as mentioned in #35422). --- related: #36785 Commits ------- 5c05455 [Messenger] Don't require doctrine/persistence when installing symfony/messenger

Nyholm requested a review from sroze as a code owner January 21, 2020 16:16

carsonbot added Status: Needs Review Messenger labels Jan 21, 2020

OskarStark reviewed Jan 21, 2020

View reviewed changes

src/Symfony/Component/Messenger/composer.json Outdated Show resolved Hide resolved

nicolas-grekas added this to the next milestone Jan 21, 2020

OskarStark approved these changes Jan 22, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 22, 2020

jderusse reviewed Jan 22, 2020

View reviewed changes

src/Symfony/Component/Messenger/composer.json Outdated Show resolved Hide resolved

Nyholm force-pushed the messenger-package branch from dea28ea to e96e55e Compare January 22, 2020 09:43

jderusse reviewed Jan 22, 2020

View reviewed changes

src/Symfony/Component/Messenger/Transport/AmqpExt/AmqpFactory.php Outdated Show resolved Hide resolved

Nyholm force-pushed the messenger-package branch 2 times, most recently from 342a1f2 to cbbd57c Compare January 22, 2020 14:16

ro0NL reviewed Jan 22, 2020

View reviewed changes

Nyholm force-pushed the messenger-package branch 2 times, most recently from 2e52edf to bc400dc Compare January 22, 2020 15:04

weaverryan approved these changes Jan 24, 2020

View reviewed changes

nicolas-grekas reviewed Jan 24, 2020

View reviewed changes

src/Symfony/Component/Messenger/Bridge/AmqpExt/CHANGELOG.md Outdated Show resolved Hide resolved

nicolas-grekas reviewed Jan 24, 2020

View reviewed changes

UPGRADE-6.0.md Outdated Show resolved Hide resolved

Nyholm mentioned this pull request Jan 24, 2020

Updated service definition to support Messenger 5.1 doctrine/DoctrineBundle#1131

Merged

nicolas-grekas approved these changes Jan 24, 2020

View reviewed changes

fabpot approved these changes Jan 24, 2020

View reviewed changes

jderusse reviewed Jan 24, 2020

View reviewed changes

src/Symfony/Component/Messenger/Bridge/AmqpExt/composer.json Outdated Show resolved Hide resolved

jderusse reviewed Jan 25, 2020

View reviewed changes

src/Symfony/Component/Messenger/Bridge/AmqpExt/composer.json Outdated Show resolved Hide resolved

Tobion suggested changes Jan 26, 2020

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.xml Outdated Show resolved Hide resolved

...Symfony/Component/Messenger/Bridge/AmqpExt/Middleware/RejectRedeliveredMessageMiddleware.php Outdated Show resolved Hide resolved

Tobion approved these changes Jan 27, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Jan 27, 2020

chalasr approved these changes Jan 27, 2020

View reviewed changes

fabpot reviewed Jan 28, 2020

View reviewed changes

fabpot approved these changes Jan 28, 2020

View reviewed changes

[Messenger] Move Transports to separate packages

2990c8f

fabpot force-pushed the messenger-package branch from 267d999 to 2990c8f Compare January 28, 2020 08:56

fabpot mentioned this pull request Jan 28, 2020

[Messenger] Move Transports to separate packages symfony/symfony-docs#12998

Closed

fabpot closed this in 3945a5c Jan 28, 2020

fabpot merged commit 2990c8f into symfony:master Jan 28, 2020

Nyholm deleted the messenger-package branch January 28, 2020 09:06

wouterj mentioned this pull request Apr 11, 2020

[Csrf] Moved CSRF component outside Security namespace #36423

Closed

nicolas-grekas modified the milestones: next, 5.1 May 4, 2020

fabpot mentioned this pull request May 5, 2020

Release v5.1.0-BETA1 #36713

Merged

tyx mentioned this pull request May 7, 2020

[Messenger] Do not require doctrine/messenger #36740

Closed

Tobion mentioned this pull request Jul 20, 2020

[Messenger] BC break in SF 5.0 for stamps due to changes stamp classes #37616

Closed

stof reviewed Jul 20, 2020

View reviewed changes

ogizanagi mentioned this pull request Aug 13, 2020

[Messenger] Don't require doctrine/persistence when installing symfony/messenger #37823

Merged

wouterj mentioned this pull request May 11, 2021

class_alias and class definition inside if (false) { } don't work correctly vimeo/psalm#5750

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Move Transports to separate packages #35422

[Messenger] Move Transports to separate packages #35422

Nyholm commented Jan 21, 2020

fabpot commented Jan 22, 2020

Nyholm commented Jan 22, 2020

Nyholm commented Jan 22, 2020

ro0NL Jan 22, 2020

Nyholm Jan 22, 2020

Nyholm Jan 22, 2020

ro0NL Jan 22, 2020

ro0NL Jan 22, 2020 •

edited

Loading

Nyholm Jan 22, 2020

ro0NL Jan 22, 2020 •

edited

Loading

weaverryan Jan 24, 2020

jderusse Jan 24, 2020 •

edited

Loading

weaverryan left a comment

Nyholm commented Jan 24, 2020

nicolas-grekas left a comment

Nyholm commented Jan 24, 2020

Tobion commented Jan 26, 2020 •

edited

Loading

Nyholm commented Jan 27, 2020

Tobion commented Jan 27, 2020 •

edited

Loading

fabpot left a comment

Nyholm commented Jan 28, 2020

Nyholm commented Jan 28, 2020

fabpot commented Jan 28, 2020

Nyholm commented Jan 28, 2020

Tobion commented Jul 20, 2020

stof Jul 20, 2020

[Messenger] Move Transports to separate packages #35422

[Messenger] Move Transports to separate packages #35422

Conversation

Nyholm commented Jan 21, 2020

fabpot commented Jan 22, 2020

Nyholm commented Jan 22, 2020

Nyholm commented Jan 22, 2020

ro0NL Jan 22, 2020

Choose a reason for hiding this comment

Nyholm Jan 22, 2020

Choose a reason for hiding this comment

Nyholm Jan 22, 2020

Choose a reason for hiding this comment

ro0NL Jan 22, 2020

Choose a reason for hiding this comment

ro0NL Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

Nyholm Jan 22, 2020

Choose a reason for hiding this comment

ro0NL Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

weaverryan Jan 24, 2020

Choose a reason for hiding this comment

jderusse Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

Nyholm commented Jan 24, 2020

nicolas-grekas left a comment

Choose a reason for hiding this comment

Nyholm commented Jan 24, 2020

Tobion commented Jan 26, 2020 • edited Loading

Nyholm commented Jan 27, 2020

Tobion commented Jan 27, 2020 • edited Loading

fabpot left a comment

Choose a reason for hiding this comment

Nyholm commented Jan 28, 2020

Nyholm commented Jan 28, 2020

fabpot commented Jan 28, 2020

Nyholm commented Jan 28, 2020

Tobion commented Jul 20, 2020

stof Jul 20, 2020

Choose a reason for hiding this comment

ro0NL Jan 22, 2020 •

edited

Loading

ro0NL Jan 22, 2020 •

edited

Loading

jderusse Jan 24, 2020 •

edited

Loading

Tobion commented Jan 26, 2020 •

edited

Loading

Tobion commented Jan 27, 2020 •

edited

Loading