-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Updated the article about data collectors #5592
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
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 would add comments before the icon and text variable to explain what they are
|
All comments have been addressed. This is ready for the second round of reviews. Thanks! |
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.
it will not be in the WebProfilerBundle though
|
I've fixed the last comments. Everything ready for the last review before merge. Thanks! |
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 the previous version was correct.
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.
After a bit googling, I find many people saying that it should always be singular. Apart from this one:
Most of the time means usually or very often; most of the times is a bit different and more specific, meaning almost every time (when); almost at every single attempt.
Using this definition, it seems like plural is correct here, but I would stick with time.
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 find "in your application" a bit too verbose, what about removing it? "To enable a data collector, define it as a regular service and tag it with data_collector:"
|
I've made all the suggested changes. Thanks! |
afb86c7 to
93bd994
Compare
|
If you agree with the latest version, please merge this PR so I can start working on this other PR: #5716 |
|
👍 |
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.
The class namespace is missing.
|
Thanks Javier, I think this is ready to be merged now. |
…reguiluz) This PR was merged into the 2.3 branch. Discussion ---------- Updated the article about data collectors | Q | A | ------------- | --- | Doc fix? | yes | New docs? | yes | Applies to | all | Fixed tickets | - First, the existing contents have been updated with the usual "modern Symfony treatment" (AppBundle, short services names, etc.) Second, some explanations have been expanded and some features are newly explained, like the data collector priority. If this PR is accepted, I'll submit a new one for 2.8 with all the new things to consider for the redesigned toolbar. Commits ------- 69bd677 Fixed some issues 93bd994 Tweaks and fixes e3a80a4 Implemented all suggestions f1de079 Updated the article about data collectors
|
Thank you @javiereguiluz! This is one of those very old cookbook article's that haven't got much love after they were published. Good to have someone taking care of them :) I've created #5774 with more rewriting of this chapter. The biggest thing I did there was using a real use-case as an example (and adding an example of showing the collected information). |
First, the existing contents have been updated with the usual "modern Symfony treatment" (AppBundle, short services names, etc.) Second, some explanations have been expanded and some features are newly explained, like the data collector priority.
If this PR is accepted, I'll submit a new one for 2.8 with all the new things to consider for the redesigned toolbar.