Skip to content
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

Removed the Internals chapter from the Symfony book #4901

Merged
merged 12 commits into from
May 23, 2015

Conversation

javiereguiluz
Copy link
Member

Q A
Doc fix? no
New docs? no
Applies to all
Fixed tickets -

The two main reasons for this change are: 1) the traffic to that book chapter is marginal; 2) all the things explained in this chapter are better explained in other parts of the documentation. This chapter was written before the documentation of the components was available.

The only important thing about this chapter is the information about kernel events. I propose to create a new reference document about events. Let's bootstrap that document with this existing info and let's add in the future the rest of Symfony events (security, forms, etc.)

@wouterj
Copy link
Member

wouterj commented Jan 22, 2015

components/http_kernel/introduction already has an overview of all these events. Internal listeners are listed there and a full list in reference/dic_tags#kernel-event-listener.

So we basically already have this info, we maybe just have to group them on a bit more logical place (the list in full dic_tags isn't a very nice location imo).

.. index::
single: Profiler; Using the profiler service

Accessing the Profiling information
Copy link
Member

Choose a reason for hiding this comment

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

do we have this convered somewhere? If not, we should create a cookbook article (or add it to an existing one).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! I've created the cookbook here: 7a833ef

@javiereguiluz
Copy link
Member Author

@wouterj considering your comments, maybe the best thing is to actually create that new reference document to group all events.

@stof
Copy link
Member

stof commented Jan 23, 2015

Should the redirection map be updated to avoid having a 404 on this URL ?

@javiereguiluz
Copy link
Member Author

@stof Good idea! I've just added a redirection to the new events reference document.

:method:`Symfony\\Component\\HttpKernel\\Profiler\\Profiler::loadProfileFromResponse`
method to access to its associated profile::

$profile = $container->get('profiler')->loadProfileFromResponse($response);
Copy link
Member

Choose a reason for hiding this comment

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

we should not advocate the usage of $contaier->get() imo. It also doesn't tell important information, such as: Where should I place this?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should at least do something like:

// ... $profiler is the 'profiler' service

$profile = $profiler->loadProfileFromResponse($response);

@wouterj
Copy link
Member

wouterj commented Jan 31, 2015

Great job @javiereguiluz! This really shows that rewritting the docs and removing things can bring the docs to a higher level.

I think we should add a table listing all priorities used by framework listeners (just move it from the dic_tags reference). This would make it very easy to determine which priority you should use for a custom listener.

Btw, there also seems to be a build failure.

@@ -110,7 +110,7 @@ Glossary
The *Kernel* is the core of Symfony. The Kernel object handles HTTP
requests using all the bundles and libraries registered to it. See
:ref:`The Architecture: The Application Directory <the-app-dir>` and the
:doc:`/book/internals` chapter.
:doc:`Internal Events Reference </reference/events.rst>`.
Copy link
Member

Choose a reason for hiding this comment

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

You just have to omit the file extension here. :)

@javiereguiluz javiereguiluz force-pushed the book_remove_internals_chapter branch from 4b7e4d3 to 7ee9465 Compare February 2, 2015 16:28
@javiereguiluz
Copy link
Member Author

As suggested, I've moved the tables about built-in event listeners from DIC Tags reference to the new Events reference.

Do you have any other suggestions or can we consider this PR as finished? Thanks.

@xabbuh
Copy link
Member

xabbuh commented Feb 2, 2015

You have to remove the reference to the stable API chapter from the /book/index.rst file.

@javiereguiluz
Copy link
Member Author

@xabbuh done!

@xabbuh
Copy link
Member

xabbuh commented Feb 2, 2015

The same also applies to /book/map.rst.inc.

@javiereguiluz
Copy link
Member Author

@xabbuh done! I'm sorry for not doing it right at first :(

| `EmailSenderListener`_ | 0 |
+-------------------------------------------------------------------------------------------+----------+
For the reference of Event Listeners associated with each kernel event, see the
:doc:`Symfony Events Reference reference/events`.
Copy link
Member

Choose a reason for hiding this comment

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

Angle brackets and the leading slash for the document are missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! Thanks.

@javiereguiluz javiereguiluz deleted the book_remove_internals_chapter branch February 18, 2015 16:22
@javiereguiluz javiereguiluz restored the book_remove_internals_chapter branch February 18, 2015 16:41
@javiereguiluz javiereguiluz reopened this Feb 18, 2015
@wouterj
Copy link
Member

wouterj commented May 22, 2015

Hi @javiereguiluz. Can you please rebase?

@javiereguiluz javiereguiluz force-pushed the book_remove_internals_chapter branch from 352bcba to 417dae6 Compare May 22, 2015 16:08
@javiereguiluz
Copy link
Member Author

@wouterj done. Let's see if conflicts are gone now.

@wouterj
Copy link
Member

wouterj commented May 22, 2015

👍

@weaverryan weaverryan merged commit 417dae6 into symfony:2.3 May 23, 2015
weaverryan added a commit that referenced this pull request May 23, 2015
…viereguiluz)

This PR was merged into the 2.3 branch.

Discussion
----------

Removed the Internals chapter from the Symfony book

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | no
| Applies to    | all
| Fixed tickets | -

The two main reasons for this change are: 1) the traffic to that book chapter is marginal; 2) all the things explained in this chapter are better explained in other parts of the documentation. This chapter was written before the documentation of the components was available.

The only important thing about this chapter is the information about kernel events. I propose to create a new reference document about events. Let's bootstrap that document with this existing info and let's add in the future the rest of Symfony events (security, forms, etc.)

Commits
-------

417dae6 Improved a lot of things thanks to the comments made by reviewers
4c5c851 Fixed the link of an internal cross reference
0f6141a Removed reference to Stable API chapter from book map file
bf33953 Removed the reference to the deleted Stable API book chapter
8fcda08 Moved the table of event listeners from the DIC tags article to the events reference
8dabfb1 Implemented most of the changes suggested by reviewers
650d82d Fixed another wrong internal cross reference
f6432ad Fixed some wrong cross references
fb4a63f Updated the references to the Symfony Profiler documentation
81e4cfe Added a redirection to avoid 404 errors
fc85752 Created a new cookbook about getting profiler data programmatically
67831b3 Removed the Internals chapter from the Symfony book
weaverryan added a commit that referenced this pull request May 23, 2015
@weaverryan
Copy link
Member

Great! Merged with some minor updates at #5297.

Thanks Javier!

weaverryan added a commit that referenced this pull request May 23, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

Kernel Events Proofreading after #4901

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3+
| Fixed tickets | n/a

Hi guys!

This is a proofread of #4901. I removed the ticks in the titles - iirc, the ticks don't work in titles. I also fixed a few minor bugs I believe.

Thanks!

Commits
-------

c7326da another fix!
106bda9 re-adding literals
cd820d7 Proofreading after #4901
javiereguiluz pushed a commit to javiereguiluz/symfony-docs that referenced this pull request Sep 2, 2015
@javiereguiluz javiereguiluz deleted the book_remove_internals_chapter branch January 3, 2018 16:34
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.

7 participants