search

prooph / psr7-middleware

Consume prooph messages with a PSR7 middleware
http://getprooph.org/
BSD 3-Clause "New" or "Revised" License
14 stars 8 forks source link

introduce a MetadataGatherer concept #7

Closed basz closed 8 years ago

basz commented 8 years ago

first stab at #6, please have a look

basz commented 8 years ago

Q. Should the EventMiddleware have this capability too?

codeliner commented 8 years ago

I'd say yes

basz commented 8 years ago

yeah, that's a problem. Better remove it here and add a note in the readme that the "metadata_gatherer" should be available in the container.

like so?

basz commented 8 years ago

@codeliner shall I start on fixing / adding tests? Are we in agreement that this is good enough structurally speaking?

codeliner commented 8 years ago

@basz yeah, it looks good. Feel free to start with the tests

basz commented 8 years ago

tests are now failing with new version of phpunit. I had 5.2.11 (no updated) which shows no errors, but 5.2.12 spits out some.

https://travis-ci.org/prooph/psr7-middleware/jobs/116326628

Can you advise I should fix those or if this is a regression on phpunit's part? Not very familiar with prophetizing

codeliner commented 8 years ago

@basz good question. I can check it in the evening if no one else has an idea //cc @sandrokeil , @prolic

sandrokeil commented 8 years ago

@basz I think you must remove the prophecy for the gatherer on some tests, because it's not called in some cases. See here which is tested by ProophTest\Psr7Middleware\CommandMiddlewareTest::it_calls_next_with_exception_if_command_name_attribute_is_not_set.

basz commented 8 years ago

hm, i'm sorry but i can't wrap my head around this.

i think that this is incorrect anyway's

QueryMiddlewareTest::it_calls_next_with_exception_if_dispatch_failed()

$queryBus->dispatch(Argument::type(Message::class))->shouldBeCalled()->willThrow(
    new \Exception('Error')
);

this test should call 'dispatch' and 'createMessageFromArray', but it claims it does not.

probably the 'fromPromise was added at a later point (?) and phpunit had a bug that didn't catched it. also it claims $responseStrategy->fromPromise should NOT be called while it actually should...

basz commented 8 years ago

i fixed my test, advise me on the above and i'l do it too ...

codeliner commented 8 years ago

I'm going to merge the PR with the failing test as it does not belong to the implementation. I'll send a PR with a fix afterwards. @basz thank you for your great work on the gatherer. Nice new feature.