Skip to content
This repository has been archived by the owner on Oct 11, 2023. It is now read-only.

Added sf 4.3 compatibility #6

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

karser
Copy link

@karser karser commented Jun 12, 2019

Hi @thePanz !

The signature of TransportFactoryInterface::createTransport has changed, as of Symfony 4.3 it requires 3rd parameter: SerializerInterface.

My changes would probably break 4.2 compatibility, maybe you have better idea?

@thePanz
Copy link
Owner

thePanz commented Jun 21, 2019

Unfortunately the changes introduced in SF 4.3 are not compatible anymore.
Check the changes in the TransportInterface. This would need more work

@karser
Copy link
Author

karser commented Jul 1, 2019

That was my first attempt to switch to v4.3 but not all dependencies are ready. I'll continue working on this when I finally switch so I can test everything thoroughly.

@karser
Copy link
Author

karser commented Jul 26, 2019

@thePanz Please, review my changes. I refactored the code according to the difference between AmqpExt v4.2 and v4.3.

Overall, looks like it is functioning, I tested it locally with a few thousands messages. I'm going to use this version in the dev environment.

Also there are some considerations:

  • there are ack/nack were introduced in v4.3, I added stub methods in Connection and marked them as TODO
  • In v4.3 was introduced MessageCountAwareInterface. Do we need it? If so, we need to implement it on FilesystemTransport and Connection
  • Since it introduces breaking changes, is it going to be v0.2?

@drzraf
Copy link

drzraf commented Sep 24, 2019

This (much needed) component [why isn't it included upstream?] needs this (much needed) update.
(I can't review the PR, but depend on it to switch to messenger on 4.3)

@thePanz
Copy link
Owner

thePanz commented Sep 24, 2019

@drzraf the changes are quite trivial, as ack/nack requires to be able to access specific messages in the queue. This is tricky as we do not have an index of messages that we can use as a db, specifically removing a message would mean to store sparse data in our .queue file.

I am not sure on which solution would be better here. Any inputs?

@karser
Copy link
Author

karser commented Sep 24, 2019

This is tricky as we do not have an index of messages that we can use as a db, specifically removing a message would mean to store sparse data in our .queue file.

Key/value storage is required. We might change format for .queue file.. It could be json, csv or even php. Or keep the same format but create multiple files for each message.

I am not sure on which solution would be better here. Any inputs?

It depends on the use case. I use this plugin only for local development and ack/nack is not that critical for me.

@thePanz
Copy link
Owner

thePanz commented Nov 21, 2019

@karser I am planning to deprecate and remove this transport.
A better suited replacement is the doctrine-transport (or the rabbitbmq).

@karser
Copy link
Author

karser commented Nov 21, 2019

That's sad. I'd say your transport better fits for local development than the alternatives you specified since it doesn't require any dependencies.
I'll stick with my fork then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants