Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update AmqpMember.php #43

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Kdyby/RabbitMq/AmqpMember.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ abstract class AmqpMember extends Nette\Object
{

/**
* @var Connection
* @var IConnection
*/
protected $conn;

Expand Down Expand Up @@ -94,10 +94,10 @@ abstract class AmqpMember extends Nette\Object


/**
* @param Connection $conn
* @param IConnection $conn
* @param string $consumerTag
*/
public function __construct(Connection $conn, $consumerTag = null)
public function __construct(IConnection $conn, $consumerTag = null)
Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

@fprochazka fprochazka Jul 8, 2016 •
edited
Loading

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.

Copy link
Author

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.

Copy link
Member

@fprochazka fprochazka Jul 8, 2016 •
edited
Loading

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.

Copy link
Author

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.

{
$this->conn = $conn;
$this->consumerTag = empty($consumerTag) ? sprintf("PHPPROCESS_%s_%s", gethostname(), getmypid()) : $consumerTag;
Expand Down