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.

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

Amqp interop based module. #4624

Closed
wants to merge 2 commits into from

Conversation

makasim
Copy link

@makasim makasim commented Nov 14, 2017

The PR add another AMQP module but based on AMQP interop. I tried to keep the module as close as possible to currently existing so It might be a replacement for it in future.

There are several reasons for that:

  • The code does not rely on an implementation but interoperable interfaces.
  • The biggest advantage among others is that it would work with not only php-amqplib but amqp-ext, and bunny, or any amqp interop compatible libraries.
    cleaner OO API.
  • less code, fewer bugs.

public function _initialize()
{
$factoryClass = $this->config['factory_class'];
if (

Choose a reason for hiding this comment

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

Expected 0 spaces after opening bracket; newline found

'vhost' => $this->config['vhost'],
]);

$this->context = $factory->createContext();;

Choose a reason for hiding this comment

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

Each PHP statement must be on a line by itself

Copy link
Contributor

Choose a reason for hiding this comment

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

double semicolon

@DavertMik
Copy link
Member

another AMQP module

Do we really need another? Yes, this one can be more stable and have fewer bugs but who knows?
Yeah, It's really cool that you have multiple backends supported. But currently, I'm not so happy to keep AMQP in core (but it can't be moved out before v3) and it has no meaning to keep two of them. Such modules should be available as a separate package.

What you can do:

  • publish AMQP-Interop into a package
  • add this package to Addons section
  • (optionally) send PR to Codeception to AMQP.php mentioning an alternative module in documentation.

@DavertMik DavertMik closed this Nov 16, 2017
@makasim
Copy link
Author

makasim commented Nov 16, 2017

This PR was meant to be a replacement for current AMQP module. I don't touch the current one to make a transition simpler. The plan is

  • I am adding a new code
  • then the current module is deprecated
  • and removed.

So, in the end, you have only one module.

@DavertMik
Copy link
Member

Yeah, but I planned to remove AMQP module completely from the core - that's the plan.
As I said it was not so widely used it is not even tested well on our CI.
So I'd like to remove it as a dependency in next major version of Codeception.
There is no reason to keep everything in the core if we have Composer )

@makasim
Copy link
Author

makasim commented Nov 16, 2017

👍

@makasim makasim deleted the amqp-interop branch November 16, 2017 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants