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

Bump service bus package #10

Merged
merged 10 commits into from
Feb 19, 2017
Merged

Bump service bus package #10

merged 10 commits into from
Feb 19, 2017

Conversation

mablae
Copy link
Member

@mablae mablae commented Jan 3, 2017

WIP

@mablae mablae force-pushed the prooph_v7 branch 2 times, most recently from 8a0a7bb to 34a0441 Compare January 3, 2017 21:37
@codeliner
Copy link
Member

travis is red. travis.yml needs update like https://github.com/prooph/event-store/blob/develop/.travis.yml#L6

@codeliner
Copy link
Member

some updates in the package are required, too.
The PluginsPass will no longer work:
https://github.com/prooph/service-bus-symfony-bundle/blob/master/src/DependencyInjection/Compiler/PluginsPass.php#L39

New set up is:
$plugin->attachToMessageBus($messageBus)
See: https://github.com/prooph/service-bus/blob/develop/src/Container/AbstractBusFactory.php#L133

So we need something like we already use in the event-store-symfony-bundle:
https://github.com/prooph/event-store-symfony-bundle/blob/master/src/DependencyInjection/Compiler/PluginsPass.php
and
https://github.com/prooph/event-store-symfony-bundle/blob/master/src/EventStoreFactory.php#L31

Unfortunately, a lot of changes required for the service-bus-bundle :(
@mablae Do you want to continue? Let me know if something is not clear.

@mablae mablae changed the title Bump service bus package WIP: Bump service bus package Jan 4, 2017
@mablae
Copy link
Member Author

mablae commented Jan 4, 2017

@codeliner Thanks for your valuable feedback. I will continue with this PR tonight and then look into
event-store-bundle, too.

Also it should be a new v2.0.0 instead of a v0.3 version with prooph v7 I guess?

This PR was not meant to be merged yet, I forgot to add the WIP

@prolic
Copy link
Member

prolic commented Jan 4, 2017

v0.3 is correct

@UFOMelkor
Copy link
Member

How about tagging the current version as v0.3 and the prooph-v7 one as v0.4?

@prolic
Copy link
Member

prolic commented Jan 4, 2017 via email

@prolic
Copy link
Member

prolic commented Jan 4, 2017

@UFOMelkor
Copy link
Member

Thx :-)

@mablae mablae force-pushed the prooph_v7 branch 3 times, most recently from 88fa39f to 89454cb Compare January 8, 2017 18:08
  - Introduce MessageBusFactory to control the reversed way of plugin attaching
  - Add onEvent and handle method to Fixture Model handlers and listeners
  - Test for factory
  - Use factory in abstract service definitions
@mablae
Copy link
Member Author

mablae commented Jan 8, 2017

@codeline @prolic I have updated and rebased everything. This PR depends on prooph/service-bus#151

Copy link
Member

@UFOMelkor UFOMelkor left a comment

Choose a reason for hiding this comment

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

Some small notes …

.travis.yml Outdated

before_install:
- if [[ $EXECUTE_TEST_COVERALLS != 'true' ]]; then phpenv config-rm xdebug.ini || return 0 ; fi
- composer self-update
- composer update --prefer-dist $DEPENDENCIES
Copy link
Member

Choose a reason for hiding this comment

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

Replacement of self-update with update intended?

.travis.yml Outdated

before_install:
- if [[ $EXECUTE_TEST_COVERALLS != 'true' ]]; then phpenv config-rm xdebug.ini || return 0 ; fi
- composer self-update
- composer update --prefer-dist $DEPENDENCIES
- if [[ $EXECUTE_TEST_COVERALLS == 'true' ]]; then composer require --no-update satooshi/php-coveralls:dev-master ; fi

install:
- travis_retry composer install --no-interaction --prefer-source
Copy link
Member

Choose a reason for hiding this comment

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

Does this have any effect after composer update in before_install?

@@ -35,6 +36,7 @@
'query' => QueryBus::class,
];


Copy link
Member

Choose a reason for hiding this comment

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

Two empty lines intended?

@@ -118,51 +120,65 @@ private function busLoad(
*/
private function loadBus(string $type, string $name, array $options, ContainerBuilder $container)
{
$serviceBusName = sprintf('prooph_service_bus.%s', $name);
Copy link
Member

Choose a reason for hiding this comment

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

String concatenation is used 8 times while sprintf is used three times. Perhaps we can unify this to string concatenation?

@@ -118,51 +120,65 @@ private function busLoad(
*/
private function loadBus(string $type, string $name, array $options, ContainerBuilder $container)
{
$serviceBusName = sprintf('prooph_service_bus.%s', $name);
Copy link
Member

Choose a reason for hiding this comment

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

$serviceBusId would be more consistent.

$containerPluginId = 'prooph_service_bus.container_plugin';
$pluginIds = array_filter(array_merge($options['plugins'], [$containerPluginId, $messageFactoryPluginId, $routerId]));


Copy link
Member

Choose a reason for hiding this comment

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

Two empty lines intended?

<service id="prooph_service_bus.command_bus" class="%prooph_service_bus.command_bus.class%" abstract="true" >
<factory service="prooph_service_bus.command_bus_factory" method="create"/>
</service>
<service id="prooph_service_bus.command_bus_factory" class="%prooph_service_bus.message_bus_factory.class%"/>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure but factory might be set to public=false

<service id="prooph_service_bus.event_bus" class="%prooph_service_bus.event_bus.class%" abstract="true" >
<factory service="prooph_service_bus.event_bus_factory" method="create"/>
</service>
<service id="prooph_service_bus.event_bus_factory" class="%prooph_service_bus.message_bus_factory.class%"/>
Copy link
Member

Choose a reason for hiding this comment

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

See comment above.

<service id="prooph_service_bus.query_bus" class="%prooph_service_bus.query_bus.class%" abstract="true" >
<factory service="prooph_service_bus.query_bus_factory" method="create"/>
</service>
<service id="prooph_service_bus.query_bus_factory" class="%prooph_service_bus.message_bus_factory.class%"/>
Copy link
Member

Choose a reason for hiding this comment

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

See comment above.

composer.json Outdated
@@ -71,7 +73,7 @@
"extra": {
"branch-alias": {
"dev-master": "1.0-dev",
"dev-develop": "1.1-dev"
"dev-prooph_v7": "0.3-dev"
Copy link
Member

@UFOMelkor UFOMelkor Jan 9, 2017

Choose a reason for hiding this comment

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

As v0.3 has been released this should be 0.4-dev (or completely removed after finishing?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was a bit in struggle with these dependecies setup. Maybe the alias should be removed, true.

@mablae
Copy link
Member Author

mablae commented Jan 9, 2017

@UFOMelkor Thank you for the review, I will adjust the stuff.

composer.json Outdated
"container-interop/container-interop": "^1.1"
},
"require-dev": {
"phpunit/phpunit": "^5.3",
"phpunit/php-invoker": "^1.1.4",
"fabpot/php-cs-fixer": "^1.11",
"bookdown/bookdown": "1.x-dev as 1.0.0",
"tobiju/bookdown-bootswatch-templates": "1.0.x-dev"
"tobiju/bookdown-bootswatch-templates": "^1.0",
Copy link
Member

Choose a reason for hiding this comment

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

prooph now has custom templates!

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will apply that.

@mablae
Copy link
Member Author

mablae commented Feb 17, 2017

Ping @UFOMelkor , @codeliner , @prolic - Sorry I totally forgot about this branch.

I just did some updates. Travis passes now and the wrong branch alias also was removed.
Let me know if anything else is missing.

@codeliner
Copy link
Member

thx @mablae

@codeliner codeliner merged commit 0b483a1 into prooph:master Feb 19, 2017
@codeliner codeliner changed the title WIP: Bump service bus package Bump service bus package Feb 19, 2017
@mablae mablae deleted the prooph_v7 branch April 6, 2017 22:15
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.

4 participants