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
from

Conversation

Projects
None yet
6 participants
@covex-nn

covex-nn commented Jan 10, 2018

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

This comment has been minimized.

Member

XWB commented Feb 6, 2018

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

@covex-nn

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

XWB commented Feb 6, 2018

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

@covex-nn

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Devtronic commented May 7, 2018

@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

This comment has been minimized.

covex-nn commented May 28, 2018

@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

This comment has been minimized.

tattali commented Jul 3, 2018

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

@covex-nn

This comment has been minimized.

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

This comment has been minimized.

Devtronic commented Jul 4, 2018

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

@tattali

This comment has been minimized.

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

This comment has been minimized.

Devtronic commented Aug 6, 2018

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

@tattali

This comment has been minimized.

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 maxhelias referenced this pull request Aug 15, 2018

Closed

FOSUserBundle recipe #452

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