Skip to content

Conversation

devtronic
Copy link

Q A
License MIT

Copy link

@ghost ghost left a 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.

@@ -0,0 +1,8 @@
# Read the documentation: https://symfony.com/doc/master/bundles/FOSRestBundle/index.html
fos_user: ~
Copy link

Choose a reason for hiding this comment

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

"fos_user" should be removed as it is empty

Copy link

@ghost ghost left a 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.

# Read the documentation: https://symfony.com/doc/master/bundles/FOSUserBundle/index.html
fos_user:
# db_driver: orm # other valid values are 'mongodb' and 'couchdb'
firewall_name: main
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, it's better IMO to directly uncomment db_driver, firewall_name and user_class in the recipe as they are all mandatory anyway in the bundle.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, you're right. I changed the config in e129d94

Copy link

@ghost ghost left a 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.

Copy link

@ghost ghost left a 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.

Copy link

@ghost ghost left a 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.

"config/": "%CONFIG_DIR%/"
},
"env": {
"MAILER_USER": ""
Copy link
Contributor

Choose a reason for hiding this comment

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

I think MAILER_USER is inconsistent with possible configuration value. I think MAILER_SENDER would probably be more appropriate. A "user" option is more to configure the mailer with a user.

Copy link
Member

Choose a reason for hiding this comment

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

I agree

Copy link

@ghost ghost left a 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.


services:
validator.builder:
class: Symfony\Component\Config\Definition\Builder\ValidationBuilder
Copy link

Choose a reason for hiding this comment

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

Should end with a newline

sender_name: "%env(MAILER_USER)%"

services:
validator.builder:
Copy link
Contributor

Choose a reason for hiding this comment

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

That's absolutely not how the recipe should work. Instead, the package itself should require the Symfony package that installs this service, which is symfony/validator in this case.

This is a problem from the FOSUserBundle side that should be fixed before a recipe can be created.

Copy link
Author

Choose a reason for hiding this comment

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

uh... I understand, sorry.

Copy link
Member

Choose a reason for hiding this comment

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

This service should be removed. You use add symfony/validator to the dependencies section in your composer.json.

@Nyholm
Copy link
Member

Nyholm commented Feb 25, 2018

FYI. This is a duplicate of #270.

@devtronic devtronic closed this Feb 25, 2018
This was referenced Apr 12, 2018
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.

4 participants