-
-
Notifications
You must be signed in to change notification settings - Fork 630
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
Add recipe for FOSUserBundle #270
Conversation
tattali
commented
Feb 1, 2018
Q | A |
---|---|
License | MIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request does not pass validation.
…fsymfony/user-bundle/2.1.x-dev/post-install.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request does not pass validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request does not pass validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Ping me when 2.1 is released and I'll be happy to merge.
Thanks I will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
user_class: App\Entity\User | ||
from_email: | ||
address: "address" | ||
sender_name: "name" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tattali FOSUserBundle can be used without templating
service. Add this to use twig directly:
fos_user:
service:
mailer: fos_user.mailer.twig_swift
It can be used even without mailer
service, if a bug will be fixed (PR is not accepted/merged yet). See FriendsOfSymfony/FOSUserBundle#2707 and FriendsOfSymfony/FOSUserBundle#2704
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sorry when I did the search I did not see your recipe. If you want we can use yours that looks more accomplished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no my recipe as there is no v2.1 release yet =) And it is not possible to create recipe right now, because even with new fos_user.service.mailer
parameter, FOSUB will require doctrine-bundle for fos_user.db_driver: orm
. I've created PR FriendsOfSymfony/FOSUserBundle#2708 ~~~but they ignore me =)~~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR. Can you make sure Travis is happy?
For now it's impossible to create a recipe for this bundle because it's necessary to install an orm (that you choose) and swiftmailer separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
Can you make sure travis is happy? |
This recipe work only on |
If you want to see a recipe for FOSUserBundle take a look and show your support by 👍 on : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request does not pass validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a little change but it looks good while waiting for compatibility so that Travis is happy.
By the way, could you update the post-install.txt file accordingly
mailer: fos_user.mailer.noop | ||
firewall_name: main | ||
user_class: App\Entity\User | ||
from_email: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use that :
from_email:
address: '%env(MAILER_SENDER_ADDRESS)%'
sender_name: '%env(MAILER_SENDER_NAME)%'
@@ -0,0 +1,18 @@ | |||
fos_user_security: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you didn't load the routes like that ?
fos_user:
resource: '@FOSUserBundle/Resources/config/routing/all.xml'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People may not need every routes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it seems good to me 👍
I can not wait to see this merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
me too I hope this PR FriendsOfSymfony/FOSUserBundle#2708 will be merged soon
"copy-from-recipe": { | ||
"config/": "%CONFIG_DIR%/", | ||
"src/": "%SRC_DIR%/" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the env var
,
"env": {
"MAILER_SENDER_ADDRESS": "[email protected]",
"MAILER_SENDER_NAME": "John Doe"
}
@@ -0,0 +1,9 @@ | |||
fos_user: | |||
db_driver: orm | |||
service: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move service
block between user_class
and from_email
for more readability please?
{ | ||
/** | ||
* @ORM\Column(type="integer") | ||
* @ORM\Id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you switch order with line above please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entity is maybe going to disappear because it's a doctrine orm entity and not mongo or couchdb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. This looks good.
#eu-fossa
@Nyholm @tattali The build failed for this PR. It includes config that is not supported (yet) by FOSUserBundle and now results in an error when you install the package. The PR for support of the |
Oh, @Devolicious, you might be correct. Do you want to "correct" this PR or what is the status on the |
@Nyholm For the time being I don't think there is any good solution except reverting this PR because the driver needs to be set and if you choose one of the supported drivers you need to have the corresponding packages as well which also breaks the build/install process if you do not have it installed. That's the main reason why they created the |
@maxhelias Can you give some more insight into this problem? |
@Devolicious In fact, FOSUserbundle does not depend on DoctrinBundle, but the db_driver option in the recipe requires another recipe to work properly as orm or mongodb. |
@Nyholm can you revert this PR? |
It was merge prematurely. Reverting symfony#270
@Devolicious, See #655 |
Initially added in symfony#270 and reverted in symfony#655
Initially added in symfony#270 and reverted in symfony#655