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

[ReportBundle] Remove bi-directional dependency on the CoreBundle #3231

Merged

Conversation

adamelso
Copy link
Contributor

@adamelso adamelso commented Sep 7, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets
License MIT
Doc PR n/a

Thanks for merging the other pull request, now the real issue - The ReportBundle is completely unusable because of the following error:

Fatal error: Uncaught exception 'Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException' with message 'The service "sylius.form.type.report" has a dependency on a non-existent service "sylius.registry.report.data_fetcher"

These missing services are registered by the CoreBundle, so I've moved them to the ReportBundle.

Additionally, I've grouped and sorted the service definitions in alphabetical order (it was getting really confusing otherwise).

Don't merge yet, though, I need to do some more work. Cheers.

@pjedrzejewski pjedrzejewski changed the title [ReportBundle] Remove bi-directional dependency on the CoreBundle [WIP][ReportBundle] Remove bi-directional dependency on the CoreBundle Sep 7, 2015
@pjedrzejewski pjedrzejewski added this to the v0.16.0 milestone Sep 7, 2015
@pjedrzejewski
Copy link
Member

I added the [WIP] tag to this PR. Thanks Adam! Let us know when it is ready for review.

@adamelso adamelso force-pushed the report-bundle-cascade-dependencies branch from 481cdc0 to a495e0f Compare September 8, 2015 10:00
@adamelso adamelso changed the title [WIP][ReportBundle] Remove bi-directional dependency on the CoreBundle [ReportBundle] Remove bi-directional dependency on the CoreBundle Sep 8, 2015
@adamelso
Copy link
Contributor Author

adamelso commented Sep 8, 2015

Thanks Pawel, this is ready now. I managed to register the bundle in my project okay.

@pjedrzejewski
Copy link
Member

Nice! Requires a rebase though. :)

@adamelso adamelso force-pushed the report-bundle-cascade-dependencies branch from a495e0f to 6567cd8 Compare September 8, 2015 11:16
@adamelso
Copy link
Contributor Author

adamelso commented Sep 9, 2015

@pjedrzejewski Done :) Though, I see that #3110 is merged, will I need to do it again? Incidently, the build is failing on PHP 5.3 as Doctrine Migrations now requires 5.4.

@pamil
Copy link
Contributor

pamil commented Sep 9, 2015

@adamelso Yes, I'm afraid it will need one another rebase :)

@@ -34,7 +34,8 @@
"stof/doctrine-extensions-bundle": "~1.1",
"sylius/resource-bundle": "0.16.*",
"sylius/money-bundle": "0.16.*",
"sylius/report": "0.16.*"
"sylius/registry": "0.16.*",
"sylius/report": "0.16.*"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pamil okay great, because I need to correct this line here :D

@adamelso adamelso force-pushed the report-bundle-cascade-dependencies branch from 6567cd8 to cd5c5f5 Compare September 10, 2015 16:50
@adamelso
Copy link
Contributor Author

alright, good to go! :)

@stloyd
Copy link
Contributor

stloyd commented Sep 11, 2015

👍

pjedrzejewski pushed a commit that referenced this pull request Sep 14, 2015
…cies

[ReportBundle] Remove bi-directional dependency on the CoreBundle
@pjedrzejewski pjedrzejewski merged commit 96d81af into Sylius:master Sep 14, 2015
@pjedrzejewski
Copy link
Member

Thank you Adam! 👍

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.

None yet

4 participants