[Messenger] Move doctrine deps to require-dev #36785

tyx · 2020-05-12T07:20:14Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #36740
License	MIT

To avoid requiring all doctrine stuff when require symfony/messenger
(that require symfony/doctrine-messenger to ensure BC)

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

nicolas-grekas

Wait, no, this is not correct, this bridge has to require these.

What could be changed is the composer.json of the messenger component itself: it should maybe not list any symfony/*-messenger. Instead, ppl should install each bridge they use.

To make things smooth, legacy classes in Transport (eg https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Messenger/Transport/Doctrine/DoctrineTransport.php) should throw a LogicException telling which bridge ppl should install.

tyx · 2020-05-12T09:58:32Z

CI Failures seems unrelated

tyx · 2020-05-12T10:06:13Z

@nicolas-grekas I'm 👍 with this solution also. But is it not too "hard" for an minor update ?

nicolas-grekas · 2020-05-12T10:08:07Z

We did it before, this is trivially fixed by ppl that will be affected - and this is not covered by the BC policy :)

Nyholm · 2020-05-12T10:36:18Z

After sleeping on this and some discussions with Nicolas. I think I was wrong with #36740 (comment)

This is the proper wrong way forward:

Remove these dependencies from symfony/messenger:

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

This will do two things:

It will allow us to specify ext-amqp, ext-redis and doctrine dependencies in the bridges.
Force the user to run composer require symfony/*-messenger.

We want to create a nice message for the user in build time so the user knows they need to run composer require. To do that, we need to throw an exception when:

the deprecated classes in Symfony\Component\Messenger\Transport is being used AND
the new transport is missing.

@tyx, could you please update this PR?

nicolas-grekas · 2020-05-12T11:31:42Z

Hm, after talking with @fabpot, we settled on your approach. Can you please add some LogicException thrown when dbal is not installed? See the other bridges for inspiration, they do something similar already.

nicolas-grekas · 2020-05-12T11:46:54Z

Oh, I posted my comment without noticing yours @Nyholm
The approach I proposed (and you mention in your comment) is cleaner, but the approach you implemented is smoother. I changed my mind and I think you did the right thing then, it's very pragmatic.

I'm OK with removing dbal/etc from the doctrine-messenger bridge. We just need these extra LogicException I mentioned in my previous comment.

Sorry for the confusion.

tyx · 2020-05-13T05:22:16Z

Hi !

So just to be sure I understand:

We keep the current behavior implemented here (aka removing dbal from doctrine/messenger)
I need to throw an exception on new DoctrineTransport present in the new bridge when dbal is missing.

nicolas-grekas · 2020-05-13T05:37:07Z

Correct, for persistence also

tyx · 2020-05-13T19:39:15Z

Ok, I updated with the check asked.
I failed to find same usecase in other bridge, so feel free to correct me !

shouze · 2020-05-14T13:10:35Z

@tyx and some tests are now failing because of the exception

Nyholm · 2020-05-14T13:13:48Z

I think we need to throw exception when the class is used. Currently we throw them when the class is loaded.

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/DoctrineTransport.php

stof · 2020-05-14T14:48:22Z

If we remove the requirements in the bridge and we ask projects to require doctrine/dbal and doctrine/persistence themselves, we are actually not making them prepare for the state we want in Symfony 6, where symfony/messenger does not force installing bridges (otherwise, we should drop the bridges entirely and put these classes directly in symfony/messenger). So the migration path being implemented here looks like the wrong one to me.

Nyholm · 2020-05-14T14:52:02Z

The bridges were a part of symfony/messenger in 5.0. We did not have any checks if doctrine was installed.

When the bridges was moved to a separate package we (mistakenly) added doctrine dependencies. That lead to us forcing the user to install doctrine when they wanted symfony/messegner.

@stof, We want to have the same behaviour as SF 5.0 and a smooth upgrade path.

stof · 2020-05-14T14:55:19Z

@Nyholm this won't be a smooth upgrade path to 5.0, as you will never tell users to add a requirement on the new bridge (which is what they need to do to upgrade to them in a future-proof way)

Nyholm · 2020-05-14T15:51:00Z

Are you sure? There are already deprecation notices telling you to require the new package. //Tobias Nyholm Skickat på språng

…

On 14 May 2020, at 16:55, Christophe Coevoet ***@***.***> wrote: @Nyholm this won't be a smooth upgrade path to 5.0, as you will never tell users to add a requirement on the new bridge (which is what they need to do to upgrade to them in a future-proof way) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

To avoid requiring all doctrine stuff when require symfony/messenger (that require symfony/doctrine-messenger to ensure BC)

nicolas-grekas

Actually, because dbal and persistence are only used in constructors, they will never need to be in "require", even in 6.0. The reason is that you cannot instantiate the bridge if you don't have them already.

The good thing is that the wiring happens in doctrine-bundle, where dbal+persistence are already required.

This means this is perfect as is.

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/DoctrineTransport.php

nicolas-grekas · 2020-05-14T16:08:23Z

this won't be a smooth upgrade path to 5.0, as you will never tell users to add a requirement on the new bridge

@stof that's true. There is no better way IMHO. Either we ask ppl to require the bridge now, or we postpone this for 6.0.

In 6.0 is the smoothest to me. Ppl will quickly figure our anyway.

stof · 2020-05-14T16:54:47Z

@nicolas-grekas The third solution is to keep the deprecated class independent of the new class. So they will need to require the new packages when migrating to the new classes (as that would be the way to get the new classes).

Are you sure? There are already deprecation notices telling you to require the new package.

They can totally use the new classes without doing that, relying on the transitive dependency of symfony/messenger adding those bridges.

nicolas-grekas · 2020-05-14T16:59:45Z

The third solution is to keep the deprecated class independent of the new class

Hmm. Duplicating the code, right?
That'd be quite annoying to maintain, ensuring bug fixes are correctly applied to both files.

I think we're good enough with the current approach, let's take the simple path for us once :)

nicolas-grekas · 2020-05-16T09:17:43Z

Thank you @tyx.

…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

tyx requested a review from sroze as a code owner May 12, 2020 07:20

carsonbot added Status: Needs Review Messenger Bug labels May 12, 2020

shouze approved these changes May 12, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels May 12, 2020

nicolas-grekas added this to the 5.1 milestone May 12, 2020

nicolas-grekas approved these changes May 12, 2020

View reviewed changes

nicolas-grekas reviewed May 12, 2020

View reviewed changes

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

tyx force-pushed the fix/doctrine-messenger branch from aebe6d1 to 35579f1 Compare May 12, 2020 09:26

nicolas-grekas requested changes May 12, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels May 12, 2020

tyx force-pushed the fix/doctrine-messenger branch from 35579f1 to 33d65dc Compare May 13, 2020 19:34

stephanvierkant reviewed May 14, 2020

View reviewed changes

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/DoctrineTransport.php Outdated Show resolved Hide resolved

Move doctrine deps to require-dev

b73b26e

To avoid requiring all doctrine stuff when require symfony/messenger (that require symfony/doctrine-messenger to ensure BC)

nicolas-grekas force-pushed the fix/doctrine-messenger branch from 33d65dc to b73b26e Compare May 14, 2020 15:53

nicolas-grekas changed the title ~~[Messenger] Move doctrine deps to require-dev until 6.0~~ [Messenger] Move doctrine deps to require-dev May 14, 2020

nicolas-grekas approved these changes May 14, 2020

View reviewed changes

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/DoctrineTransport.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Work labels May 14, 2020

Tobion approved these changes May 15, 2020

View reviewed changes

nicolas-grekas merged commit 02cedc3 into symfony:master May 16, 2020

tyx deleted the fix/doctrine-messenger branch May 18, 2020 09:16

ogizanagi mentioned this pull request Aug 13, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Move doctrine deps to require-dev #36785

[Messenger] Move doctrine deps to require-dev #36785

tyx commented May 12, 2020

nicolas-grekas left a comment •

edited

Loading

tyx commented May 12, 2020

tyx commented May 12, 2020

nicolas-grekas commented May 12, 2020 •

edited

Loading

Nyholm commented May 12, 2020 •

edited

Loading

nicolas-grekas commented May 12, 2020

nicolas-grekas commented May 12, 2020

tyx commented May 13, 2020 •

edited

Loading

nicolas-grekas commented May 13, 2020

tyx commented May 13, 2020

shouze commented May 14, 2020

Nyholm commented May 14, 2020

stof commented May 14, 2020

Nyholm commented May 14, 2020

stof commented May 14, 2020

Nyholm commented May 14, 2020 via email

nicolas-grekas left a comment •

edited

Loading

nicolas-grekas commented May 14, 2020

stof commented May 14, 2020

nicolas-grekas commented May 14, 2020 •

edited

Loading

nicolas-grekas commented May 16, 2020

[Messenger] Move doctrine deps to require-dev #36785

[Messenger] Move doctrine deps to require-dev #36785

Conversation

tyx commented May 12, 2020

nicolas-grekas left a comment • edited Loading

Choose a reason for hiding this comment

tyx commented May 12, 2020

tyx commented May 12, 2020

nicolas-grekas commented May 12, 2020 • edited Loading

Nyholm commented May 12, 2020 • edited Loading

This is the proper wrong way forward:

nicolas-grekas commented May 12, 2020

nicolas-grekas commented May 12, 2020

tyx commented May 13, 2020 • edited Loading

nicolas-grekas commented May 13, 2020

tyx commented May 13, 2020

shouze commented May 14, 2020

Nyholm commented May 14, 2020

stof commented May 14, 2020

Nyholm commented May 14, 2020

stof commented May 14, 2020

Nyholm commented May 14, 2020 via email

nicolas-grekas left a comment • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented May 14, 2020

stof commented May 14, 2020

nicolas-grekas commented May 14, 2020 • edited Loading

nicolas-grekas commented May 16, 2020

nicolas-grekas left a comment •

edited

Loading

nicolas-grekas commented May 12, 2020 •

edited

Loading

Nyholm commented May 12, 2020 •

edited

Loading

tyx commented May 13, 2020 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading

nicolas-grekas commented May 14, 2020 •

edited

Loading