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

[Messenger] Add a Doctrine transport #29007

Open
wants to merge 1 commit into
base: master
from

Conversation

@vincenttouzet

vincenttouzet commented Oct 28, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#10616
DoctrineBundle PR doctrine/DoctrineBundle#868

As discussed with @sroze on PHPForum I've worked on adding a Doctrine transport to the Messenger component.

Actually AMQP is the only supported transport and it could be a good thing to support multiple transports. Having a Doctrine transport could help users to start using the component IMHO (Almost all projects use a database)

TODO

  • Add tests
  • Create DOC PR
  • PR on doctrine-bundle for transport factory

I am also considering to introduce a SetupableTransportInterface and a SetupTransportsCommand to avoid checking schema differencies at runtime. But it could be in a separated PR maybe ?

@vincenttouzet vincenttouzet requested a review from sroze as a code owner Oct 28, 2018

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch from ef46d12 to 536ed49 Oct 28, 2018

@Taluu

Looks good, but overall missing typehints / return hints as it's a feature for 4.3+, so php 7.1+. I'm guessing the missing tests are because it's a wip :}

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch from 536ed49 to 4fae182 Oct 29, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 29, 2018

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch 2 times, most recently from f0b0993 to fc59186 Oct 29, 2018

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch from fc59186 to f998853 Oct 29, 2018

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch 3 times, most recently from d174c1b to 4023eaa Oct 29, 2018

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch from 4023eaa to 9014d1b Oct 29, 2018

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch 2 times, most recently from 0a327b6 to c81751a Oct 30, 2018

@vincenttouzet

This comment has been minimized.

vincenttouzet commented Oct 30, 2018

@nicolas-grekas Hum I can understand that he is "scared" that people will use this instead of the enqueue project. But for me this won't replace enqueue at all.

This is a very simple implementation that people can use for fast prototyping for example. When using a transport like AMQP you need to have a server running RabbitMQ (or other implementation of AMQP).

Personnally for big projects that will run on production I will always use a RabbitMQ so the AMQPTransport or enqueue if I need Kafka, SQS, ... But for small projects or prototyping I think it's "too big".

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch 3 times, most recently from a02ce42 to 744b16e Oct 30, 2018

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch 2 times, most recently from 96c0797 to 47b343b Oct 30, 2018

@vincenttouzet

This comment has been minimized.

vincenttouzet commented Oct 31, 2018

Is there a problem with the Process component on AppVeyor ? The build faild since yesterday and I don't know why 🤔

@chalasr

This comment has been minimized.

Member

chalasr commented Oct 31, 2018

@vincenttouzet that's a random failure which has been fixed recently (#29015), rebasing your branch should avoid the failure on next pushes if any. I restarted your build, now green

@vincenttouzet

This comment has been minimized.

vincenttouzet commented Oct 31, 2018

@chalasr ok that what I thought. Thank you for relaunch (I don't know if I can relaunch by myself)

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch 2 times, most recently from a12832b to 3286572 Oct 31, 2018

@vincenttouzet

This comment has been minimized.

vincenttouzet commented Nov 1, 2018

@Taluu @ostrolucky Is it good for you now ?

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch from 3286572 to 6304dbf Nov 3, 2018

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch from 6304dbf to d1305b0 Nov 11, 2018

@vincenttouzet

This comment has been minimized.

vincenttouzet commented Nov 11, 2018

I've added a requeueMessages method on the Connection to requeue message that are in delivered status for a long time. (The timeout is configurable under the redeliver_timeout option)

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch 2 times, most recently from e386735 to 8b4514f Nov 11, 2018

@vincenttouzet

This comment has been minimized.

vincenttouzet commented Nov 20, 2018

Status: needs review

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch from 8b4514f to 23bf6c0 Nov 20, 2018

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch from 23bf6c0 to 9513301 Dec 3, 2018

@vincenttouzet vincenttouzet force-pushed the vincenttouzet:messenger-doctrine-transport branch from 9513301 to ac96153 Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment