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

Handle custom body content types #37

Merged
merged 5 commits into from
May 10, 2017
Merged

Handle custom body content types #37

merged 5 commits into from
May 10, 2017

Conversation

Niktux
Copy link
Contributor

@Niktux Niktux commented Apr 26, 2017

First commit : refactor
Second one : methods to inject custom mapping 'content_type => TypedBodyFactory implementation'

Refers to #34

@Niktux Niktux requested a review from lebris April 26, 2017 14:07
@codecov
Copy link

codecov bot commented Apr 26, 2017

Codecov Report

Merging #37 into master will increase coverage by 0.84%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #37      +/-   ##
============================================
+ Coverage      62.2%   63.04%   +0.84%     
- Complexity      287      300      +13     
============================================
  Files            35       40       +5     
  Lines           918      966      +48     
============================================
+ Hits            571      609      +38     
- Misses          347      357      +10
Impacted Files Coverage Δ Complexity Δ
src/Commands/Worker/Run.php 0% <0%> (ø) 5 <0> (ø) ⬇️
src/Silex/AmqpServiceProvider.php 0% <0%> (ø) 5 <0> (ø) ⬇️
src/Workers/WorkerCommands.php 0% <0%> (ø) 2 <0> (ø) ⬇️
src/Workers/ProcessorInterfaceAdapter.php 89.47% <100%> (+0.58%) 5 <0> (ø) ⬇️
src/Workers/MessageAdapterFactory.php 100% <100%> (ø) 3 <3> (?)
src/Workers/MessageAdapter.php 100% <100%> (ø) 21 <1> (+1) ⬆️
src/Workers/ReadableMessageModifier.php 100% <100%> (ø) 19 <0> (ø) ⬇️
src/Messages/BodyFactories/Standard.php 100% <100%> (ø) 5 <5> (?)
src/Messages/TypedBodyFactories/Json.php 100% <100%> (ø) 1 <1> (?)
src/Messages/TypedBodyFactories/Text.php 100% <100%> (ø) 1 <1> (?)
... and 11 more

@Niktux Niktux changed the title [DO NOT MERGE] refactor: decoupled bodyFactory Handle custom body content types Apr 26, 2017
@Niktux Niktux added this to the 3.0.0 milestone May 2, 2017
];
}

public function handleContentType($contentType, TypedBodyFactory $factory)
Copy link
Member

Choose a reason for hiding this comment

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

Missing in interface BodyFactory ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope
BodyFactory is a contract for an object which can build Body instance from amqp message payload.
This method is to allow to customize the behaviour of this very implementation of BodyFactory

@@ -20,11 +21,12 @@ public function __construct(WorkerContext $workerContext)
{
$this->workerContext = $workerContext;
$this->eventDispatcher = new NullEventDispatcher();
$this->messageAdapterFactory = null;
Copy link
Member

Choose a reason for hiding this comment

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

unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used in L29 $message = $this->createMessageAdapter($message);
Explanation in https://github.com/puzzle-org/amqp/blob/2349818d10d230f30fe505fbb0219feeb0e075c6/src/Workers/MessageAdapterFactoryAware.php

It's a drawback of using traits :-(

@@ -26,6 +29,8 @@ public function __construct(Application $console, Client $client, WorkerProvider
$this->client = $client;
$this->workerProvider = $workerProvider;
$this->outputInterfaceAware = $outputInterfaceAware;

$this->messageAdapterFactory = null;
Copy link
Member

Choose a reason for hiding this comment

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

The messageAdapterFactory is mandatory, why using a setter instead of injecting it ?

Out of the box it is generating a fatal error

Catchable fatal error: Argument 1 passed to Puzzle\AMQP\Commands\Worker\Run::setMessageAdapterFactory() must be an instance of Puzzle\AMQP\Workers\MessageAdapterFactory, null given, called in /var/www/app/vendor/puzzle/amqp/src/Workers/WorkerCommands.php on line 46 and defined in /var/www/app/vendor/puzzle/amqp/src/Workers/MessageAdapterFactoryAware.php on line 10

The only thing that we can change in the MessageAdapterFactory is the BodyFactory.

The WorkerCommands could get the BodyFactory as a dependency, so it would be able to build the MessageAdapterFactory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point is the missing nullable in type hinting (as we are support php 5, we have to use default value to manage it ... or sadly drop the type hinting)

You're right with the BodyFactory point but in conception view I wanted to preserve some layers of abstraction. That's why we have 2 levels of factories.

For the mandatory part, I've considered that users that don't need to customize theses parts must have nothing to do at all (so no BC break in API). That's why dependencies are optional and that's why there are fallbacks to set standards implementations if they don't provide any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in f2c275d

Copy link
Member

@lebris lebris left a comment

Choose a reason for hiding this comment

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

If we change the content_type attribute of a message, it is overwritten when calling the Message method packAttributes with the content_type defined with the used Body.

cf.

@Niktux
Copy link
Contributor Author

Niktux commented May 10, 2017

fixed in 5ffc3c7

@Niktux Niktux merged commit a5920bd into master May 10, 2017
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.

2 participants