Updated the article about data collectors #5592
Conversation
|
|
||
| {% block toolbar %} | ||
| {# This toolbar item may appear along the top or bottom of the screen.#} | ||
| {% set icon %} |
There was a problem hiding this comment.
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! |
|
|
||
| .. code-block:: jinja | ||
|
|
||
| {{ include('@WebProfiler/Icon/my_collector.svg') }} |
There was a problem hiding this comment.
it will not be in the WebProfilerBundle though
|
I've fixed the last comments. Everything ready for the last review before merge. Thanks! |
| @@ -46,106 +46,160 @@ access to in local properties. | |||
| store objects that cannot be serialized (like PDO objects), or you need | |||
| to provide your own ``serialize()`` method. | |||
|
|
|||
| Most of the time, it is convenient to extend | |||
| Most of the times, it is convenient to extend | |||
There was a problem hiding this comment.
I think the previous version was correct.
There was a problem hiding this comment.
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.
| ------------------------------- | ||
|
|
||
| To enable a data collector, add it as a regular service in one of your | ||
| configuration, and tag it with ``data_collector``: | ||
| To enable a data collector, define it as a regular service in your application |
There was a problem hiding this comment.
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! |
javiereguiluz
force-pushed
the
improve_data_collectors
branch
from
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 |
|
👍 |
| @@ -51,101 +51,169 @@ Most of the time, it is convenient to extend | |||
| populate the ``$this->data`` property (it takes care of serializing the | |||
| ``$this->data`` property):: | |||
|
|
|||
| class MemoryDataCollector extends DataCollector | |||
| // src/AppBundle/DataCollector/MyCollector.php | |||
|
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.