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.

Sign up for GitHub

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

Jump to bottom

Symfony 4 components support, drop < 3.3 support #33

Merged

codeliner merged 17 commits into prooph:master from mateuszsip:feature/symfony4

Feb 21, 2018

Contributor

mateuszsip commented Dec 23, 2017 •

edited

Loading

Meant to resolve #30

@UFOMelkor do you have some specific tests in mind? Can I help somehow?

Compiler passes should get own tests for sure.

TODO:

Don't mark as public services that should stay private
Write compiler passes tests

Post rebase TODO:

Commands are not registered (only in test kernel?)
Enrichers are broken - failing tests

Contributor

sstok commented Jan 2, 2018

You properly want to have a look at https://github.com/SymfonyTest especially https://github.com/SymfonyTest/SymfonyDependencyInjectionTest

Contributor Author

mateuszsip commented Jan 8, 2018

Tests needs some cleanup and more scenarios for ProjectorPass, but feel free to review.

UFOMelkor requested changes

View reviewed changes

src/DependencyInjection/ProophEventStoreExtension.php Outdated

                       }
                       if (! empty($config['projections'])) {
+                          // FIXME: Missing second argument

Member

UFOMelkor Jan 8, 2018

The complete block can be safely removed. projections is not available in config.

src/DependencyInjection/ProophEventStoreExtension.php Outdated

                                   )
+                                  // TODO: Remove me
+                                  ->setPublic(true)
                                   ->setFactory([new Reference('prooph_event_store.repository_factory'), 'create'])

Member

UFOMelkor Jan 8, 2018

Factory can be moved to prooph_event_store.repository_definition.

src/DependencyInjection/ProophEventStoreExtension.php Outdated

-                          ->setClass('%prooph_event_store.metadata_enricher_aggregate.class%');
+                          // TODO: Remove me
+                          ->setPublic(true)
+                          ->setClass(MetadataEnricherAggregate::class);

Member

UFOMelkor Jan 8, 2018

Is setClass really necessary? It is inherited from prooph_event_store.metadata_enricher_aggregate_definition, isn't it?

src/DependencyInjection/ProophEventStoreExtension.php Outdated

-                          ->setClass('%prooph_event_store.metadata_enricher_plugin.class%');
+                          // TODO: Remove me
+                          ->setPublic(true)
+                          ->setClass(MetadataEnricherPlugin::class);

Member

UFOMelkor Jan 8, 2018

Same as above

src/DependencyInjection/ProophEventStoreExtension.php Outdated

                       $eventStoreDefinition = $container
                           ->setDefinition(
                               $eventStoreId,
-                              new DefinitionDecorator('prooph_event_store.store_definition')
+                              new ChildDefinition('prooph_event_store.store_definition')
                           )
                           ->setFactory([new Reference('prooph_event_store.store_factory'), 'createEventStore'])

Member

UFOMelkor Jan 8, 2018

Factory can be moved to prooph_event_store.store_definition

test/DependencyInjection/Fixture/TestServices.php Outdated

+              abstract class TestServices
+              {
+                  public const EVENT_STORE_SERVICE_ID_PREFIX = 'prooph_event_store.';

Member

UFOMelkor Jan 8, 2018

TBH I don't see a real value in adding a class for this constant.

Contributor Author

mateuszsip Jan 28, 2018

I feel more confident with constant while refactoring, but it's not something I wll fight for.
Little value, 2 votes against - inlined in 12ebe0c

test/DependencyInjection/Compiler/MetadataEnricherPassTest.php Outdated

+                      $this->compile();
+                      $this->assertContainerBuilderHasServiceDefinitionWithArgument(
+                          sprintf('prooph_event_store.%s.%s', 'metadata_enricher_aggregate', 'foo'),

Member

UFOMelkor Jan 9, 2018

sprintf necessary?

test/DependencyInjection/Compiler/MetadataEnricherPassTest.php Outdated

+                      );
+                      $this->assertContainerBuilderHasServiceDefinitionWithArgument(
+                          sprintf('prooph_event_store.%s.%s', 'metadata_enricher_aggregate', 'bar'),

Member

UFOMelkor Jan 9, 2018

same as above

bgaleotti commented Jan 17, 2018

In https://github.com/kejwmen/event-store-symfony-bundle/blob/e2f7b6ce2b75fcca69509d449b43bd7090282b41/src/DependencyInjection/ProophEventStoreExtension.php#L71, it should be:

use Prooph\EventStore\Projection\ProjectionManager;

//

            $projectionManagerDefintion
                ->setClass(ProjectionManager::class)
                ->setFactory([new Reference('prooph_event_store.projection_factory'), 'createProjectionManager'])

Otherwise, given the foobar_projection_manager it will trigger an error when building the container:

Please add the class to service "prooph_event_store.projection_manager.foobar_projection_manager" even if it is constructed by a factory since we might need to add method calls based on compile-time checks.

mateuszsip force-pushed the feature/symfony4 branch from e2f7b6c to 12ebe0c Compare

January 28, 2018 15:04

Contributor Author

mateuszsip commented Jan 28, 2018

Thanks for review @UFOMelkor @bgaleotti.

mateuszsip force-pushed the feature/symfony4 branch from 774310f to 862bc60 Compare

January 28, 2018 15:26

Contributor Author

mateuszsip commented Jan 28, 2018

Sorry for delay, I forgot about this PR :D

Today I want to finish it, do we want to merge this: #34 before? Looks like conflicts and (maybe?) duplicated test scenarios, I can rebase and fix it, just let me know.

Contributor Author

mateuszsip commented Jan 28, 2018 •

edited

Loading

What about testing private services?
I started with storing prefixed aliases in separate config file, do we have a better option?

DefaultEventStoreFactory use container to attach plugins.
What is the best way to handle that? Looks like a job for service locator, what do you think?

Introduced these changes in 4479ab1

I know test_aliases.yml looks messy, but I had to introduce aliases for services registered in tests using config fixtures. These tests can load additional file with services configuration, is it the way to go?

Member

UFOMelkor commented Jan 29, 2018

@kejwmen May I ask you to review #34? Merging without having a second person looking on it … ;-)

Member

UFOMelkor commented Jan 31, 2018

👍 for the plugin-ServiceLocator.

In addition, services not meant to be used by the application directly, should be defined as private.
Best Practices for Reusable Bundles

While I personally dislike using the container directly, I think the event-stores and the repositories should be public. Your thoughts on this?

Member

UFOMelkor commented Jan 31, 2018 •

edited

Loading

So, for now the last review-comment 😉

The projection commands are accessing the container which will fail because the accessed services are not public. Perhaps another ServiceLocator as solution?

The MetadataEnricher will also need some love. Currently they are also fetched from the container. Tagging them as plugins instead of handling them seperately might be a simple solution.

I know test_aliases.yml looks messy, but I had to introduce aliases for services registered in tests using config fixtures.

I would suggest to put the aliases directly into the respective test/DependencyInjection/Fixture/config/xml|yml file.

Thank you for your commitment and your endurance!

Contributor Author

mateuszsip commented Feb 3, 2018

While I personally dislike using the container directly, I think the event-stores and the repositories should be public. Your thoughts on this?

Heart says no, brain says 👍 - I don't see a valid use case to fetch them from container instead of injecting, but if there is any that can be painful to users.

The projection commands are accessing the container which will fail because the accessed services are not public. Perhaps another ServiceLocator as solution?

ServiceLocator sounds like a good idea to start.
#34 includes tests for these commands (thanks!), I will take care of that after rebase.

The MetadataEnricher will also need some love. Currently they are also fetched from the container. Tagging them as plugins instead of handling them seperately might be a simple solution.

Good idea, simplifies things a lot.
Implemented in e93636f

I would suggest to put the aliases directly into the respective test/DependencyInjection/Fixture/config/xml|yml file.

I forgot we can put service definitions there :D
Done.

Feel free to review latest commits, I will try to finish what's left tomorrow.

mateuszsip force-pushed the feature/symfony4 branch from e93636f to edba340 Compare

February 4, 2018 16:35

UFOMelkor reviewed

View reviewed changes

Member

UFOMelkor left a comment

Looks good to me 👍

src/DependencyInjection/Compiler/MetadataEnricherPass.php

@@ @@ -41,7 +43,8 @@ public function process(ContainerBuilder $container) @@
                           $metadataEnricherId = sprintf('prooph_event_store.%s.%s', 'metadata_enricher_plugin', $name);
                           $metadataEnricherDefinition = $container->getDefinition($metadataEnricherId);
-                          $metadataEnricherDefinition->setClass('%prooph_event_store.metadata_enricher_plugin.class%');
+                          $metadataEnricherDefinition->setClass(MetadataEnricherPlugin::class);
+                          $metadataEnricherDefinition->addTag(sprintf('prooph_event_store.%s.plugin', $name));

Member

UFOMelkor Feb 5, 2018

Move this to ProophEventStoreExtension, it will fix the enrichers (MetadataEnricherPass is executed after the PluginPass).

test/Command/Fixture/TestKernel.php Outdated

+                          new ProophEventStoreBundle(),
+                      ];
+                  }

Member

UFOMelkor Feb 5, 2018

If you add the Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass via build method and add the commands as services, commands should be recognized.

mateuszsip changed the title ~~[WIP] Symfony 4 components support, drop < 3.3 support~~ Symfony 4 components support, drop < 3.3 support

Member

lunetics commented Feb 12, 2018

Testing your WIP, without any config i get:

The service "Prooph\Bundle\EventStore\Command\ProjectionNamesCommand" has a dependency on a non-existent parameter "prooph_event_store.projection_managers".

Member

lunetics commented Feb 12, 2018

You should add the "event_store" key (definition) to the configuration (was type before)
https://github.com/kejwmen/event-store-symfony-bundle/blob/43d5ebf53892d25c669944fbea95ad75b132d8b5/src/DependencyInjection/Configuration.php#L86

enumag mentioned this pull request

Support for symfony 4.x #39

Closed

Member

codeliner commented Feb 20, 2018

@kejwmen and/or @UFOMelkor can you resolve the conflicts? I tried it but tests are failing with:

1) ProophTest\Bundle\EventStore\DependencyInjection\XmlEventStoreExtensionTest::it_creates_an_event_store
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The "prooph_event_store.projection_manager.main_projection_manager" service or alias has been removed or inlined when the container was compiled. You should either make it public, or stop using the container directly and use dependency injection instead.

Projection manager should also be a public service I guess

Contributor Author

mateuszsip commented Feb 20, 2018

@codeliner I will take care of that later today

Member

codeliner commented Feb 20, 2018

thx

Member

UFOMelkor commented Feb 20, 2018

@kejwmen This will remove the necessity for an own service locator for the projection managers. (more public services, the heart is bleeding^^).
Projections and read models on the other hand should still have their own service locator.

Actually this should have it own test and be not just part of the it_creates_an_event_store test, but perhaps we focus on making projection managers public and getting this finally merged.

Member

codeliner commented Feb 20, 2018 •

edited

Loading

but perhaps we focus on making projection managers public and getting this finally merged.

👍 test cases can be added in a separate PR. Let's finish it as soon as possible, so that teams can finally upgrade to symfony 4.0

Member

codeliner commented Feb 20, 2018

(more public services, the heart is bleeding^^).

Started a poll: https://twitter.com/prooph_software/status/965946184706723842
I'd say we go for the public option in this PR and if majority of people say we should make it private then we can do this in another PR

mateuszsip added 5 commits

February 20, 2018 21:28


          Bumped deps, some refactoring

b917375


          PluginsPassTest and MetadataEnricherPassTest

d7b6f04


          Basic ProjectorPassTest


          tests codestyle

1a9bf5f


          Simplified service definitions, codestyle fixes

mateuszsip added 12 commits

February 20, 2018 21:29


          Inlined TestServices::EVENT_STORE_SERVICE_ID_PREFIX

924ad69


          Added prooph_event_store.projection_definition

5963bb9


          Services are no longer public, added plugins service locator, introdu…

5957c96

…ced test aliases


          Enricher plugin is tagged as plugin and included in service locator

d3e09ab


          Moved fixture-specific test aliases to fixtures

5b8c61d


          post-rebase fixes, stores and repos are public

cbb5c18


          Additional locators, commands explicitly registered as services

74c84fc


          codestyle

bb784d4


          Default prooph_event_store.projection_managers value

62f6333


          event_store key is required in store configuration

245e5be


          always execute loadProjectionManagers to set defaults for related par…

4dc00a6

…ameters


          ProjectionManagers are public by default

f30dd90

mateuszsip force-pushed the feature/symfony4 branch from 3ef98ad to f30dd90 Compare

February 20, 2018 20:35

Contributor Author

mateuszsip commented Feb 20, 2018

Rebased on master and made ProjectionManagers public, but dedicated locator is still there. Should I remove it before merging?

@UFOMelkor @codeliner

coveralls commented Feb 20, 2018

Pull Request Test Coverage Report for Build 144

62 of 73 (84.93%) changed or added relevant lines in 8 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.8%) to 81.549%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/Command/AbstractProjectionCommand.php	0	11	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/Command/AbstractProjectionCommand.php	1	0.0%

Totals
Change from base Build 143:	0.8%
Covered Lines:	358
Relevant Lines:	439

💛 - Coveralls

Member

codeliner commented Feb 20, 2018

@UFOMelkor Can we merge? Once merged I'd release a new version of the bundle to communicate symfony 4.0 support 🎉

I'd say further improvements can be made in separate PRs

Member

UFOMelkor commented Feb 21, 2018

LGTM 👍

codeliner merged commit da6386c into prooph:master

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

Labels

None yet