-
Notifications
You must be signed in to change notification settings - Fork 48
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
Update AmqpMember.php #43
base: master
Are you sure you want to change the base?
Conversation
typos
* @param string $consumerTag | ||
*/ | ||
public function __construct(Connection $conn, $consumerTag = null) | ||
public function __construct(IConnection $conn, $consumerTag = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would break the contact of the interface. Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need use SSLConnection (https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Connection/AMQPSSLConnection.php) instead of Connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I can understand that, but this is not a good way to solve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? What is the correct way? Please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AmqpMember
and subclasses of it are using some methods of the given $conn
instance. With this change, you create dependency on interface, that doesn't require those methods -> it's breaking the contract of the interface. When in fact, you depend on broader interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. But then I can not think of a way to use class SSLConnection? Instead interface use abstract class AbstractConnection?
PS: sorry za angličtinu... můj skill není moc cool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je potřeba se podívat, jaky metody to používá :) Pokud AmqpMember
ani zadni jeho potomci nepotřebují nic z IConnection
, může se tam v pohodě hodit AbstractConnection
.
Na druhou stranu, bude ale potřeba podědit SSLConnection
a přidat tam věci z Kdyby\RabbitMq\Connection
, jinak přestane fungovat zbytek knihovny. Možná bych z toho dokonce udělal traitu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Momentálně to co využívám mi spolkne i typehint s tím interface. Ale chápu problém.
Teď přemýšlím jak jsi to přesně myslel.
Ideálně, aby v neonu pokud použiju třeba ssl:true se použilo Connection s funkčností z SSLConnection. Jestli si pamatuji, tak tvůj kód očekává v typehintech instance tvé Connection.
Knihovně by tak mělo být jedno jakou používá instanci. Jestli AMQPLazyConnection nebo AMQPSSLConnection. Nejbližší rodič obou dvou je AMQPStreamConnection a pak AbstractConnection.
Musel bych to asi promyslet, která cesta bude lepší... pokud to nepředěláš samozřejmě ty.
Traitu jak jsem psal úplně nevím jak do toho zakomponovat.
Jestli ji vložit do Connection a na základě parametrů, které dostane od DI volat vytváření contextu. Protože jediné co je tam navíc koukám je metoda create_ssl_context.
typos