Skip to content

adding table for controller as a service#5033

Merged
weaverryan merged 2 commits intosymfony:2.3from
dbu:controller-service
Mar 14, 2015
Merged

adding table for controller as a service#5033
weaverryan merged 2 commits intosymfony:2.3from
dbu:controller-service

Conversation

@dbu
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

wouterj commented Feb 20, 2015

I like this one :)

Copy link
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@dbu
Copy link
Copy Markdown
Contributor Author

dbu commented Feb 20, 2015

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

Copy link
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

dbu commented Feb 21, 2015

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

@timglabisch
Copy link
Copy Markdown
Contributor

awesome PR 👍

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

dbu commented Mar 9, 2015

thanks @Pierstoval , updated the things you noted.

@xabbuh
Copy link
Copy Markdown
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
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