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 db_driver parameter optional with "no_driver" default value #2708

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

covex-nn
Copy link

FOSUserBundle does not require doctrine/doctrine-bundle and never will, i guess. So, it is not possible to create Flex recipe for this bundle without changes.

My proposal is to make db_driver parameter optional, with new default value "no_driver" to get all errors in runtime, not on configuration stage.

With this PR, fos_user.user_manager.default and fos_user.group_manager.default services will throw \RuntimeException on every implemented method.

@XWB
Copy link
Member

XWB commented Feb 6, 2018

This doesn't feel right. Why no setting a default driver in the recipe?

@covex-nn
Copy link
Author

covex-nn commented Feb 6, 2018

@XWB it is because using default value orm for parameter fos_user.db_driver will require doctrine/doctrine-bundle and without this package travis for symfony/recipes-contrib will fail.

@XWB
Copy link
Member

XWB commented Feb 6, 2018

Then we should move the package from require-dev to require in https://github.com/FriendsOfSymfony/FOSUserBundle/blob/master/composer.json#L33

@covex-nn
Copy link
Author

covex-nn commented Feb 6, 2018

I was not sure, this could be done =(
See @stof's comment about SwiftmailerBundle as a required dependency #2562 (comment) - i guess with doctrine/doctrine-bundle will be the same: as this would hurt anyone not using it (c)

@XWB
Copy link
Member

XWB commented Feb 6, 2018

I'm starting to feel a recipe for this bundle is not possible at all.

@covex-nn
Copy link
Author

covex-nn commented Feb 6, 2018

It would be possible with this PR and after fixing bug about resetting: false =)
See #2704 (comment)

@covex-nn
Copy link
Author

covex-nn commented Feb 8, 2018

@XWB the main goal of this PR is to make possible clean installation of this bundle via Flex. To achive that, FOSUserBundle should not require any additional services, not configured with installed bundles from composer.json

Right now all supported db_drivers (orm, mongodb and couchdb) require some other services, that are not installed by default. All, except one: 'custom', and i could use this value for fos_user.db_driver in recipe, but then i would had to set up fos_user.services.user_manager (see Configuration.php#L66) ! Unfortunately FOSUserBundle does not contain appropriate class for this. That is why i suggested to add no_driver as a new supported db_driver.

@JVerstry
Copy link

JVerstry commented Feb 8, 2018

I am a newbie to FOSUserBundle, so I am the kind of person who would be the most impacted or puzzled by this configuration/requirement issue. What I have really appreciated so far is the feedback I got at runtime about Symfony configuration issues when I ran my apps in dev mode. It was easy to understand and to fix. I got enough guidance to solve my issue as a dev. What I appreciated less is that solving dependencies from "old" to Flex was tricky. I really love this idea of recipes.
This idea of setting no_driver as a default value to enable the proper creation of a recipe seems like a great idea to me. It also fits with the Flex (as in flexibility) philosophy. A proper error/warning message indicating some extra configuration is required (orm, whatever...) is enough. It forces the dev to think about its implementation and it is a good thing. This just has to be properly documented too.

@XWB
Copy link
Member

XWB commented Feb 16, 2018

@covex-nn I understand, but this PR feels like a hack. Configuration stuff should be handled in Configuration. I wonder if we could come up with a different approach? Maybe @stof has some ideas.

@devtronic
Copy link

@XWB Can you explain why this PR feels to you like a hack? For the mailer also exists a "noop" service:

<service id="fos_user.mailer.noop" class="FOS\UserBundle\Mailer\NoopMailer" public="false" />

@covex-nn
Copy link
Author

@XWB just FYI. We implemented a similar feature for sonata-project/media-bundle (see sonata-project/SonataMediaBundle#1420). And now SonataMediaBundle has its own recipe. A bundle is not functional out from the box, and there will be an exception thrown when a developer will try to use it. But! There is another recipe for sonata-project/media-orm-pack, that complements a bundle's recipe.

And now, after executing composer require sonata-project/media-orm-pack, developer will recieve:

  1. sonata-project/media-bundle and sonata-project/doctrine-orm-admin-bundle from pack's dependencies; doctrine/doctrine-bundle with doctrine/orm will be installed too
  2. A default configuration from MediaBundle recipe
  3. A database storage configuration for MediaBundle from MediaOrmPack recipe
  4. Three Entity classes for medias and galleries

So, after executing doctrine:schema:update console command, a bundle will be fully functional. Also soon there will be published a recipe for another pack with MongoDB support for SonataMediaBundle.

I just wanted to say, i still believe that FOSUserBundle potentially can have a recipe for Flex too =)

@tattali
Copy link

tattali commented Jul 3, 2018

Is that possible to add a script to demand which package he wants to install ?

@covex-nn
Copy link
Author

covex-nn commented Jul 3, 2018

@tattali Flex will never be interactive, see symfony/flex#344. So, there won't be any "on demand" =(

@devtronic
Copy link

@XWB Can you please respond to #2708 (comment)?
Maybe we find a better solution.

@tattali
Copy link

tattali commented Aug 6, 2018

In fact this is possible to inform users after recipe install by adding a post-install.txt file.

We can tell them in that file the change they need to do to finish FOSUser installation

post-install.txt example

@devtronic
Copy link

@tattali Yes, but without the change from this PR, the recipe can not be published because symfony flex tests will not pass.

@tattali
Copy link

tattali commented Aug 6, 2018

Yes of course but maybe @XWB and @stof wasn't know.

This PR has been opened 8 months ago. So a solution should be find.

If the changes of this PR are merged. The instructions about the dependencies installation can be put in the post-install.txt file.

If not we should think about changing the documentation to balance the absence of recipe...

@maxhelias
Copy link

@covex-nn what is the status of this feature ?

@juanmiguelbesada
Copy link

What about using prepend extension. Just check if DoctrineBundle is installed and set de config according.

@vladdnepr
Copy link

@covex-nn hi, symfony flex + FOSUserBundle still not working. Have you any news?

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.

8 participants