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

Make REST Bundle optional #753

Merged
merged 1 commit into from Nov 23, 2015
Merged

Make REST Bundle optional #753

merged 1 commit into from Nov 23, 2015

Conversation

core23
Copy link
Member

@core23 core23 commented Apr 10, 2015

As the REST bundle is not required for the this bundle, the api form and controllers definitions should only be loaded, when the bundles are used.

@SonataCI
Copy link
Collaborator

SonataCI commented Aug 8, 2015

Can one of the admins verify this patch?

}
$bundles = $container->getParameter('kernel.bundles');

if (isset($bundles['FOSRestBundle']) && isset($bundles['NelmioApiDocBundle'])) {
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 that NelmioApiDocBundle is required to get REST working.

@core23
Copy link
Member Author

core23 commented Aug 10, 2015

I copied the detection from the NewsBundle. Thought this would be the same logic

@core23
Copy link
Member Author

core23 commented Oct 1, 2015

@rande same problem like sonata-project/SonataClassificationBundle#42

@core23
Copy link
Member Author

core23 commented Oct 12, 2015

ping @rande

@core23
Copy link
Member Author

core23 commented Oct 29, 2015

Sorry for bothering you, but can you have a quick look? @rande

@core23
Copy link
Member Author

core23 commented Nov 1, 2015

This bug is really annoying... Can you have a look @soullivaneuh as @rande seems a little busy.


$loader->load(sprintf('api_form_%s.xml', $config['db_driver']));
if (isset($bundles['FOSRestBundle']) && isset($bundles['NelmioApiDocBundle'])) {
if (!in_array(strtolower($config['db_driver']), array('doctrine_orm', 'doctrine_mongodb', 'doctrine_phpcr'))) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you move this code ? seems to be unrelated to bundles detections.

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 for your advice. I moved it to the origin @rande

@OskarStark
Copy link
Member

@core23 it is not optional IMO, see: #877

we need this use statements and have the API Doc there, so i'm 👍 for closing this PR

@core23
Copy link
Member Author

core23 commented Nov 9, 2015

It IS optional, as I added a check to load the API controllers only when the API bundles are available.

@OskarStark
Copy link
Member

ah ok, sorry you are right, i was confused :)

@@ -40,7 +38,9 @@
"aws/aws-sdk-php": "~2.7",
"doctrine/mongodb-odm": "~1.0",
"jackalope/jackalope-doctrine-dbal": "~1.1",
"symfony/phpunit-bridge": "~2.7|~3.0"
"symfony/phpunit-bridge": "~2.7|~3.0",
"friendsofsymfony/rest-bundle": "~1.1",
Copy link
Member

Choose a reason for hiding this comment

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

are you sure, that require-dev ist correct? not suggest?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are required(-dev) for the travis build.

Copy link
Member

Choose a reason for hiding this comment

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

hmm, but for the development it looks weird, so on my local machine a would have an api with docs and on prod not... 👎

Copy link
Member Author

Choose a reason for hiding this comment

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

Have a look at the composer docs (https://getcomposer.org/doc/04-schema.md#require-dev)

The dev packages are only loaded if you have them in your root composer.json. No require-dev package of a child composer.json is loaded.

@core23
Copy link
Member Author

core23 commented Nov 14, 2015

Any news for this? @rande @OskarStark

@OskarStark
Copy link
Member

I'll let @rande decide

@core23
Copy link
Member Author

core23 commented Nov 23, 2015

Sorry for pinging you that much, but that needless dependency is really annoying... @rande

rande added a commit that referenced this pull request Nov 23, 2015
Make REST Bundle optional
@rande rande merged commit 5a2f3fa into sonata-project:master Nov 23, 2015
@core23
Copy link
Member Author

core23 commented Nov 23, 2015

Thank you ❤️ @rande

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

5 participants