Movatterモバイル変換

src/Symfony/Component/Messenger/Transport/NullTransport.php OutdatedShow resolvedHide resolved

Copy link

Contributor

ogizanagi commentedNov 5, 2018

Note regarding#29097 (comment): that would mean making the MemoryTransportMessageBusInterface $bus constructor argument optional or extract dispatch in a dedicated listener.
No strong opinion yet.

nicolas-grekas added this to thenext milestone

Nov 6, 2018

webignition reviewed

src/Symfony/Component/Messenger/Tests/Transport/NullTransportFactoryTest.php OutdatedShow resolvedHide resolved

webignition reviewed

src/Symfony/Component/Messenger/Tests/Transport/NullTransportFactoryTest.php OutdatedShow resolvedHide resolved

webignition reviewed

src/Symfony/Component/Messenger/Tests/Transport/NullTransportFactoryTest.php OutdatedShow resolvedHide resolved

webignition reviewed

src/Symfony/Component/Messenger/Tests/Transport/NullTransportFactoryTest.php OutdatedShow resolvedHide resolved

webignition reviewed

[Messenger] add a transport that processes messages on kernel.terminate#28746

src/Symfony/Component/Messenger/Tests/Transport/NullTransportFactoryTest.php OutdatedShow resolvedHide resolved

Copy link

Member

javiereguiluz commentedNov 7, 2018

@GaryPEGEOT thanks for this contribution! If this is finally merged, please review thelist of Messenger issues andlist of pending Messenger pull requests to see if there are issues/PRs conflicting with this change. If there are not, please open an issue inhttps://github.com/symfony/symfony-docs/issues to track this change. You don't have to provide the docs yourself if you don't want, but this would help us track the change. Thanks!

ogizanagi mentioned this pull request

Nov 10, 2018

Closed

weaverryan mentioned this pull request

Mar 12, 2019

[Messenger] Making it Shine#30540

Closed

36 tasks

sroze approved these changes

Mar 19, 2019

Copy link

Contributor

sroze left a comment

Choose a reason for hiding this comment

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

Except these stylish details to be changed, looks good to me 👍

src/Symfony/Component/Messenger/Transport/NullTransport.php OutdatedShow resolvedHide resolved

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md OutdatedShow resolvedHide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Mar 19, 2019

sroze reviewed

Mar 20, 2019

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.mdShow resolvedHide resolved

Copy link

Member

stof commentedMar 20, 2019

Just a random thought: Isn't a null transport just a special case of amemory transport without any listener?

If you never flush your MemoryTransport, it would leak memory. So using it instead of a NullTransport looks wrong (having to flush something when you expect it to be a blackhole is not intuitive)

fabpot reviewed

Mar 20, 2019

src/Symfony/Component/Messenger/Transport/NullTransport.php Outdated

		*/
		publicfunctionsend(Envelope$envelope):Envelope
		{
		$this->sent[] =$envelope;

Copy link

Member

fabpotMar 20, 2019

Choose a reason for hiding this comment

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

As we accumulate envelopes over time, do we need to implementResetableInterface to flush the envelopes from one request to the next?

Copy link

ContributorAuthor

GaryPEGEOTMar 23, 2019

Choose a reason for hiding this comment

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

@sroze,@fabpot I've implementedSymfony\Contracts\Service\ResetInterface and I now see that there is anack anreject method in the Transport interface. Do you think we need to store which message has been ack / nack?

Copy link

Contributor

ogizanagi commentedMar 21, 2019•
edited
Loading

having to flush something when you expect it to be a blackhole is not intuitive)

But it's not just a blackhole (null transport might be misleading in this way): this transport is storing the sent envelopes (to be used in tests for instance). So I would have implementedResetableInterface on theMemoryTransport and just use a different factory registering it.

Copy link

Contributor

sroze commentedMar 23, 2019

@GaryPEGEOT can you implement theResetableInterface and test that the messages are properly removed?

Copy link

Contributor

sroze commentedMar 23, 2019 via email

Indeed, it would make sense.

…

On Sat, 23 Mar 2019 at 22:48, Gary PEGEOT ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Symfony/Component/Messenger/Transport/NullTransport.php <#29097 (comment)>: > + } + + /** + * ***@***.***} + */ + public function stop(): void + { + $this->stopped = true; + } + + /** + * ***@***.***} + */ + public function send(Envelope $envelope): Envelope + { + $this->sent[] = $envelope;@sroze <https://github.com/sroze>,@fabpot <https://github.com/fabpot> I've implemented Symfony\Contracts\Service\ResetInterface and I now see that there is an ack an reject method in the Transport interface. Do you think we need to store which message has been ack / nack? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29097 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHEZUH-p7C_wteK6h840k5Nr_c8SPSks5vZkzOgaJpZM4YOsl_> .

Copy link

ContributorAuthor

GaryPEGEOT commentedMar 23, 2019

@sroze I added thegetAcknowledged andgetRejected methods

ogizanagi reviewed

Mar 23, 2019

src/Symfony/Component/Messenger/Transport/NullTransport.php Outdated

		*
		* @author Gary PEGEOT <garypegeot@gmail.com>
		*/
		class NullTransportimplements TransportInterface, ResetInterface

Copy link

Contributor

ogizanagiMar 23, 2019•
edited
Loading

Choose a reason for hiding this comment

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

Note implementingResetInterface isn't enough for this to be properly integrated with the framework and stored envelopes to be cleared from one request to the next. The service definition must be tagged withkernel.reset.

But as the service is built using a factory and might use an env var for the DSN, it may not be possible to add the tag conditionally from the extension (compile time). Instead, you could implement ResetInterface on the factory too and keep track of created services to clear them from here (so only the factory will be tagged withkernel.reset).

onEXHovia reviewed

Mar 24, 2019

src/Symfony/Component/Messenger/Tests/Transport/NullTransportFactoryTest.php Outdated

		*
		* @author Gary PEGEOT <garypegeot@gmail.com>
		*/
		class NullTransportFactoryTestextends \PHPUnit\Framework\TestCase

Copy link

Contributor

onEXHoviaMar 24, 2019

Choose a reason for hiding this comment

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

maybe worth importing TestCase?
use PHPUnit\Framework\TestCase;

Copy link

Member

fabpot commentedMar 31, 2019

I agree with@ogizanagi that I would not expect anull transport to store things.

Copy link

Contributor

sroze commentedMar 31, 2019

in-memory://?

Copy link

Member

weaverryan commentedMar 31, 2019

Well, we also have#28746, which is a "memory" transport where the messages are sent onkernel.terminate. Probably we should only merge one... call itmemory:// (?), and have the "send on kernel.terminate` part configurable, so that you could use it for testing (and not handle on kernel.terminate) or decide that you DO want to send it using that.

Copy link

Contributor

sroze commentedMar 31, 2019

I agree it should be the same implementation. Though... it could actually be two DSNsin-memory:// &symfony://kernel.terminate IMHO. Let's make this one justin-memory:// and ask thekernel.terminate to be changed to update this transport. WDYT?

Copy link

ContributorAuthor

GaryPEGEOT commentedMar 31, 2019

Should it be a black-hole and do nothing at all (no storing nor receiving of message) to avoid duplication with memory listener?

Copy link

Contributor

sroze commentedMar 31, 2019

to avoid duplication with memory listener

What do you mean? 🤔

Copy link

ContributorAuthor

GaryPEGEOT commentedMar 31, 2019

to avoid duplication with memory listener
What do you mean? thinking

memorytransport, my bad!, both this transport and the one in#28746 store message in memory, so are kind of duplicate. Should this one donothing instead, ie not storing any message?

Copy link

Member

weaverryan commentedMar 31, 2019

I think the idea is this: This one SHOULD store in memory, and be calledin-memory. And then#28746 could actually use this transport behind the scenes to read the stored messages and send them.

Copy link

Contributor

sroze commentedMar 31, 2019

Exactly 👍

Copy link

ContributorAuthor

GaryPEGEOT commentedMar 31, 2019

I think the idea is this: This one SHOULD store in memory, and be calledin-memory. And then#28746 could actually use this transport behind the scenes to read the stored messages and send them.

Renamed toInMemoryTransport, as well as the dsn

GaryPEGEOT changed the title~~[Messenger] Add a "null://" transport~~[Messenger] Add a "in-memory://" transport

Mar 31, 2019

sroze suggested changes

Mar 31, 2019

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md OutdatedShow resolvedHide resolved

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.xml OutdatedShow resolvedHide resolved

src/Symfony/Component/Messenger/Transport/InMemoryTransport.phpShow resolvedHide resolved

src/Symfony/Component/Messenger/Transport/InMemoryTransport.php OutdatedShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Reviewed labels

Mar 31, 2019

javiereguiluz added the ⭐️ EU-FOSSA Hackathon label

Apr 2, 2019

carsonbot added Status: Reviewed and removed Status: Needs Work labels

sroze approved these changes

Add a "null://" transport

fe75920

Ocramius reviewed

src/Symfony/Component/Messenger/Transport/InMemoryTransportFactory.phpShow resolvedHide resolved

Make the in-memory transport resettable

8f8c82e

fabpot approved these changes

src/Symfony/Component/Messenger/Transport/InMemoryTransport.php

		useSymfony\Contracts\Service\ResetInterface;

		/**
		* Transport that stay in memory. Useful for testing purpose.

Copy link

Member

fabpotApr 6, 2019

Choose a reason for hiding this comment

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

stays

Copy link

Member

fabpot commentedApr 6, 2019

Thank you@GaryPEGEOT.

fabpot merged commit8f8c82e intosymfony:master

fabpot added a commit that referenced this pull request