-
-
Notifications
You must be signed in to change notification settings - Fork 622
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 support for Roukmoute Hashids bundle #282
Conversation
roukmoute
commented
Feb 7, 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.
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.
@roukmoute thanks for this contribution! We try to minimize the number of env vars because they are very problematic to manage for lots of people. So, I'm asking if you think that these four config options should really be env vars or they could be just normal config params. We do that in most of our recipes. For example in https://github.com/symfony/recipes-contrib/blob/master/doctrine/mongodb-odm-bundle/3.3/config/packages/doctrine_mongodb.yaml we just define two env vars for some really special params ... but the rest are normal params. Thanks! |
@javiereguiluz Of course I can do that :) |
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.
Isn’t these settings same as the default config? I suggest remove all except for “salt” and them add a value like “change me”. It would also be great with a link to the docs.
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 passes validation.
Done @Nyholm :) |
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.
Great. Can you just make sure Travis is happy?
I try but currently I have some troubles to test it in local:
|
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.
I like this PR. We just going to make sure Travis is happy.
@@ -0,0 +1,8 @@ | |||
{ | |||
"bundles": { | |||
"Roukmoute\\HashidsBundle": ["all"] |
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.
Travis complains here. This line should be:
Roukmoute\\HashidsBundle\\RoukmouteHashidsBundle
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 passes validation.
@Nyholm What do you want me to do? Now the error is caused by To configure the ORM layer, you must first install the doctrine/orm package I can add in my What do you suggest? |
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.
Yeah, why do you get this error? Do you require doctrine/orm in your bundle? |
@Nyholm that's actually related to our DoctrineBundle recipe. The DoctrineBundle recipe configures the ORM layer, but the ORM is an optional dependency of DoctrineBundle, not a required one (so people wanting to use only the DBAL layer have to remove stuff after installing the recipe to get things working). @roukmoute your dependency on DoctrineBundle alone looks weird to me though. You don't need the DBAL layer at all. Your only place depending on DoctrineBundle is for the param converter, and this will only work with the ORM enabled. So you should either have a requirement on the ORM (not only on DoctrineBundle) or make DoctrineBundle an optional dependency and disable the param converter when you don't have both DoctrineBundle and the ORM in the project. |
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. Make sure to add Symfony 4 support so all users can use this bundle
Yes it will be the next version, I will do that quickly 👍 |
@symfony-flex-server review again |
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.