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

adding table for controller as a service #5033

Merged
merged 2 commits into from
Mar 14, 2015
Merged

adding table for controller as a service #5033

merged 2 commits into from
Mar 14, 2015

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Feb 20, 2015

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

an old thing from my todo list. talked with @beberlei about this at some point. he had a couple of interesting blog posts about this topic but i can't find it anymore. and afaik the symfony doc does not want this kind of external links anyways.

``forward`` ``http_kernel`` ``$httpKernel->forward($controller, $path, $query)``
``generateUrl`` ``router`` ``$router->generate($route, $params, $absolute)``
``getDoctrine`` ``doctrine``
``getRequest`` - Use ``Request $request`` as parameter in your action method. The order of the parameters is not important, but the typehint must be there.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh. is there a way to better format tables in rst?

Copy link
Member

Choose a reason for hiding this comment

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

This syntax (which allows multiline too):

+---------+---------+-----+
| Column1 | Column2 | ... |
+=========+=========+=====+
| Value 1 | multi-  | ... |
|         | line 1  |     |
+---------+---------+-----+
| Value 2 | Value 2 | ... |
+---------+---------+-----+
| ...     | ...     | ... |
+---------+---------+-----+

Copy link
Member

Choose a reason for hiding this comment

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

The simple table syntax allows to wrap long lines in several shorter lines: http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#simple-tables

Copy link
Member

Choose a reason for hiding this comment

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

let's remove this row btw, as getRequest is deprecated

Copy link
Member

Choose a reason for hiding this comment

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

@wouterj one of these days we should standardize the table syntax to use in the documentation. Hopefully, we'll never recommend to use the rigid grid syntax ;)

@wouterj
Copy link
Member

wouterj commented Feb 20, 2015

I like this one :)


``getRequest`` has been deprecated. Instead, have an argument to your
controller action method called ``Request $request``. The order of the
parameters is not important, but the typehint must be provided.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

converted this to a separate tip. i think it does not hurt to mention this here again. the whole doc segment is for people afraid to look at the controller source code.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@dbu
Copy link
Contributor Author

dbu commented Feb 20, 2015

should we mention the table in the introduction of the cookbook article?

+-----------------------------+------------------------------------+----------------------------------------------------------------+
| Method | Service | PHP Code |
+=============================+====================================+================================================================+
| ``createForm`` | ``form.factory`` | ``$formFactory->create($type, $data, $options)`` |
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong. You are missing the ->getForm() call in the equivalent code

Copy link
Contributor Author

@dbu dbu Feb 20, 2015 via email

Choose a reason for hiding this comment

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

| | | $token = $securityContext->getToken(); |
| | | if (null !== $token && is_object($token->getUser())) { |
| | | $user = $token->getUser(); |
| | | } |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this renders quite badly (all code lines floating) but i found no way how to include multiline source code in a table cell... ideas? btw, the indention is missing on purpose as otherwise we get a compile error.

Copy link
Member

Choose a reason for hiding this comment

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

Tables are a hell in Sphinx to get things like this working nicely. I don't know of a nice solution other than not using table...

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to use a definition list instead:

:method:`Symfony\\Bundle\\FrameworkBundle\\Controller\\Controller::createForm` (service: ``form.factory``)
    .. code-block:: php

        $formFactory->create($type, $data, $options);

and so on

@dbu
Copy link
Contributor Author

dbu commented Feb 21, 2015

thanks @xabbuh , switched to definition list. the rendering looks ok to me.

@timglabisch
Copy link
Contributor

awesome PR 👍

:method:`Symfony\\Bundle\\FrameworkBundle\\Controller\\Controller::createNotFoundException`
.. code-block:: php

throw new NotFoundHttpException($message, $previous);
Copy link
Member

Choose a reason for hiding this comment

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

This is not exactly the same. The Controller class doesn't throw the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, indeed. what a weird method anyways. will fix

@dbu
Copy link
Contributor Author

dbu commented Feb 23, 2015

fixed the comments. and thanks @timglabisch! credit goes to @sixty-nine, he started this ages ago and i said i would do a PR with it, but never got around to do it until now.


$templating->renderResponse($view, $parameters, $response);

:method:`Symfony\\Bundle\\FrameworkBundle\\Controller\\Controller::renderViev` (service: ``templating``)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo : maybe change to renderView :)

@dbu
Copy link
Contributor Author

dbu commented Mar 9, 2015

thanks @Pierstoval , updated the things you noted.

@xabbuh
Copy link
Member

xabbuh commented Mar 9, 2015

Thanks @dbu! This looks really good to me.

@weaverryan weaverryan merged commit b17f422 into symfony:2.3 Mar 14, 2015
weaverryan added a commit that referenced this pull request Mar 14, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

adding table for controller as a service

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

an old thing from my todo list. talked with @beberlei about this at some point. he had a couple of interesting blog posts about this topic but i can't find it anymore. and afaik the symfony doc does not want this kind of external links anyways.

Commits
-------

b17f422 use definition list instead of table
8e82db4 adding table for controller as a service
weaverryan added a commit that referenced this pull request Mar 14, 2015
When I merge to 2.6, I'll properly change the service ids and variable names
weaverryan added a commit that referenced this pull request Mar 14, 2015
* 2.3:
  [#5033] Tweaking variable name to "match" the service id
  [#5017] Minor language tweaks
  use definition list instead of table
  adding table for controller as a service
  Suggested ConEmu in addition to ANSICON to solve the Windows coloring limitation
  Minor rewording
  Reworded the note about Windows console and output coloring
  Update routing.rst
  Minor rewording
  add a note about apc for php recent versions
  Removed a wrong link to a included file
  Rewording
  Minor rewording
  Fixed an internal link
  Added a note about data transformers not being applied with inherit_data option set
  Added a commented config useful when you use symlinks
  Minor rewording
  Added a note about the class option of the services defined via factories
  Added a note about the server_version DBAL option

Conflicts:
	components/dependency_injection/factories.rst
@weaverryan
Copy link
Member

I think this is really cool! Even if you don't make your controllers into services, I love showing "the magic behind the scenes" to newer devs. So, thank you very much @dbu!

I updated the security service names at sha: 2035d62

weaverryan added a commit that referenced this pull request Mar 14, 2015
* 2.6: (91 commits)
  [#5064] Minor language tweaks
  Fixing bad merge conflict (forgot to save!)
  Remove unnecessary component reference
  Correct RegisterListenersPass namespace
  Fix service id
  Switched the first example to a static constructor method
  added some more components for Tobion as a merger
  Fixed variable name in : Reference -> validation constraints -> count -> basic usage -> PHP
  [#5036] Typo fix (probably mine originally) caught by xabbuh
  reword to serves
  Adding a link to define "lts"
  Better wording
  Minor improvement for symfony-installer with LTS
  Updating for new security service names in 2.6
  [#5033] Tweaking variable name to "match" the service id
  [#5017] Minor language tweaks
  [#5015] Updating the security service name for 2.6 - thanks to Cordoval
  [#5015] Very small tweak
  [#5011] Adding one more fix I missed
  [#5011] Fixing minor build issue
  ...

Conflicts:
	book/security.rst
weaverryan added a commit that referenced this pull request Mar 14, 2015
* 2.7: (103 commits)
  Backporting some stuff from 2.7, that I think must have gotten merged only there by accident
  [#5064] Minor language tweaks
  Fixing bad merge conflict (forgot to save!)
  Remove unnecessary component reference
  Correct RegisterListenersPass namespace
  Fix service id
  Switched the first example to a static constructor method
  added some more components for Tobion as a merger
  Fixed variable name in : Reference -> validation constraints -> count -> basic usage -> PHP
  [#5036] Typo fix (probably mine originally) caught by xabbuh
  reword to serves
  Adding a link to define "lts"
  Better wording
  Minor improvement for symfony-installer with LTS
  Updating for new security service names in 2.6
  [#5033] Tweaking variable name to "match" the service id
  [#5017] Minor language tweaks
  [#5015] Updating the security service name for 2.6 - thanks to Cordoval
  [#5015] Very small tweak
  [#5011] Adding one more fix I missed
  ...
@dbu dbu deleted the controller-service branch October 18, 2018 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants