Skip to content

Add autoconfigure for services extending PsrProcess interface #452

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

Merged
merged 1 commit into from
Aug 4, 2018

Conversation

mnavarrocarter
Copy link
Contributor

@mnavarrocarter mnavarrocarter commented May 31, 2018

This will allow to Symfony users that have autoconfigure/autowiring enabled to avoid defining
explicitly the processor service and its tag. Now, the symfony container does it automatically.

@makasim
Copy link
Member

makasim commented May 31, 2018

This would work for processors that implement TopicSubscriber or CommandSubscriber interfaces. It would fail If none of them is implemented cuz in that case a developer should add extra parameters to the tag.

Could we do autotag for TopicSubscriber or CommandSubscribe interfaces only?

@mnavarrocarter
Copy link
Contributor Author

Okay I get it. Question is, can anyone use TopicSubscriber or CommandSubscriber without implementing PsrProcessor? I did the test and I can. Maybe what we can do is what you say plus make CommandSubscriber and TopicSubscriber extend from the PsrProcessor interface. Just a suggestion.

@mnavarrocarter
Copy link
Contributor Author

@makasim Have you tought about what I said? If you don't think is a good idea let me know and I can just do what you say. :)

@makasim
Copy link
Member

makasim commented Jun 7, 2018

plus make CommandSubscriber and TopicSubscriber extend from the PsrProcessor interface

@mnavarrocarter I dont have any particular reasons against it though I tend to keep it as is. Can we do it later ?

@mnavarrocarter
Copy link
Contributor Author

@makasim Sure we can. I'll work on the autoconfig of the two interfaces you mentioned. Should be ready in a couple of hours.

@mnavarrocarter
Copy link
Contributor Author

@makasim I think we are done here. I also kept in mind #410 and made the services public. I know is not best practice, but until you guys come with a better solution, that's what it is. :)

@mnavarrocarter
Copy link
Contributor Author

Also, just a side note. I tend to squash all my commits before merging, but your merging strategy is "merge commit". If you use "fast-forward" merge you'll get a better history for feature branches that are squashed in a single commit, and you would get rid of those merge commits in the middle. :)

@makasim
Copy link
Member

makasim commented Jun 7, 2018

@mnavarrocarter the auttag feature should be enabled for newer versions of symfony see failed tests https://travis-ci.org/php-enqueue/enqueue-dev/jobs/389233722

@mnavarrocarter
Copy link
Contributor Author

@makasim Here we go again!

@makasim
Copy link
Member

makasim commented Jun 7, 2018

The last thing. There are code style issues, Could you please fix them https://travis-ci.org/php-enqueue/enqueue-dev/jobs/389255972?

…or TopicSubscriberInterface

This will allow to symfony users that have autowiring/autoconfigure enabled to be able to rapidly
create their processors without worrying registering them as a service and adding the tag.

This closes php-enqueue#409 and php-enqueue#405. Also, keeps in mind php-enqueue#410. That's why services are public until a better solution
is implemented.
@mnavarrocarter
Copy link
Contributor Author

I think that we are done here! That PR was tougher than I thought!

@makasim
Copy link
Member

makasim commented Jun 8, 2018

fixes #406, #410

@daften
Copy link

daften commented Jun 8, 2018

Thanks @mnavarrocarter for this. I tried the PR, but didn't have time (and aren't skilled enough with symfony) to finish it in time. @makasim too. You guys make open source rock!

@mnavarrocarter
Copy link
Contributor Author

Thanks for your kind words @daften Glad to be able to help a bit. Hope this gets released soon! :)

'transport' => [],
]], $container);

if (method_exists($container, 'registerForAutoconfiguration')) {
Copy link
Member

Choose a reason for hiding this comment

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

The test has to be split into two. One test the case where the method is not available and whet it presents

@makasim
Copy link
Member

makasim commented Jun 11, 2018

@mnavarrocarter could you please fix the test? I'd be happy to merge this

@mnavarrocarter
Copy link
Contributor Author

@makasim Sure I can. Sorry I've been busy at work. You'll have it fixed later today. :)

@dkarlovi
Copy link
Contributor

dkarlovi commented Jul 6, 2018

Can we merge here?

@makasim
Copy link
Member

makasim commented Jul 6, 2018

@dkarlovi sure, as soon as the test is fixed.

@makasim makasim merged commit efd9543 into php-enqueue:master Aug 4, 2018
@makasim
Copy link
Member

makasim commented Aug 4, 2018

fixes #405

@intergalactisch
Copy link

intergalactisch commented Sep 9, 2018

Hi guys, awesome bundle but due to the autoconfiguration I'm bumping into an issue where the Processor is cached/defined twice and therefore consumes a message twice.

My services.yaml has autoconfigure on true, and has defined the processor as:
import_count_processor: class: 'App\Service\ImportCountProcessor' public: true tags: - { name: 'enqueue.client.processor' } calls: - [setContainer, ['@service_container']]

With this, the processor looks like this:
`
namespace App\Service;

use Interop\Queue\PsrContext;
use Interop\Queue\PsrMessage;
use Interop\Queue\PsrProcessor;
use Enqueue\Client\TopicSubscriberInterface;

use Symfony\Component\DependencyInjection\ContainerAwareInterface;
use Symfony\Component\DependencyInjection\ContainerAwareTrait;
use Doctrine\ORM\EntityManagerInterface;
use Psr\Container\ContainerInterface;

class ImportCountProcessor implements PsrProcessor, TopicSubscriberInterface, ContainerAwareInterface
{

use ContainerAwareTrait;

protected $em;
protected $container;

/**
 * Importer constructor.
 *
 * @param EntityManagerInterface $em
 */
public function __construct(EntityManagerInterface $em, ContainerInterface $container)
{
    $this->em = $em;
    $this->container = $container;
}

public function process(PsrMessage $message, PsrContext $session)
{

    echo "Import Count" . PHP_EOL; //for testing purposes

    return self::ACK;
}

public static function getSubscribedTopics()
{
    return ['importCountTopic'];
}

}
`
With this the container is needed to eventually send a new event to a topic.

Any ideas on how I can fix the duplication of the processor? Do I just need to remove the import_count_processor definition? [edited for code markup]

@makasim
Copy link
Member

makasim commented Sep 9, 2018

set autoconfigure: false and keep your tag or remove your tag and let Symfony tag it.

@intergalactisch
Copy link

Okido thanks for the quick reply, removing the tag from services.yaml fixed my issue

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

Successfully merging this pull request may close these issues.

5 participants