-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Added missing mandatory parameter #6122
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
Conversation
yceruto
commented
Jan 9, 2016
| Q | A |
|---|---|
| Doc fix? | yes |
| New docs? | no |
| Applies to | all |
| Fixed tickets | n/a |
cookbook/profiler/data_collector.rst
Outdated
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 think we should use either { link: true } or { link: false } here. Using profiler_url here looks like you could pass the actual URL here.
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 agree with @xabbuh here.
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.
@xabbuh I agree.
I think this variable could be named show_link instead?
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.
@xabbuh however, the symfony core uses profile_url instead true:
|
👍 |
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.
Or shouldn't we better set this to falselike it is done in the example before? Otherwise it might look strange for the reader that we changed the value.
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.
It should be true because the example requires access to profile panel. In the example before, the link is not necessary.
|
perhaps we need to add any comments to talk about the |
|
👍 |
This PR was submitted for the 2.7 branch but it was merged into the 2.3 branch instead (closes #6122). Discussion ---------- Added missing mandatory parameter | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | all | Fixed tickets | n/a Commits ------- 0368933 Added parameter mandatory
* remove wrong default value description * fix code style[#6122] some tweaks
|
Thanks @yceruto, I have merged your PR into the |
|
👍 |