[Form] Alternate syntax for form_theme #3576
Conversation
|
do you really need |
|
it's not needed but I find it more clear (It can be drop if a consensus is reached) |
|
+1 for |
|
+1 for |
|
|
|
@vicb supporting a string instead of an array should be possible when you need only one element. supporting several ones and turning it into an array is the mistake we made for 2.0 |
|
+1 for the new syntax |
|
+1 for new syntax. But it should be possible to use strings instead of arrays. |
|
@vicb Having one wyntax using |
|
@Baachi not stringS as it is precisely what we are trying to solve :) |
|
Oh sry. I mean string. :) |
|
+1 for supporting a string or an array with the new syntax as using only one element is probably the most common use case. But then, why not supporting any valid Twig expression? |
|
Something like the latest commit ? (Tests have to be updated). @fabpot What is the best place to handle array / non-array ? This is currenlty handled in the node but the parser might be a better place. |
|
@vicb: I would just remove the special array case in the node as it's not needed anymore. |
|
... and update FormExtension::setTheme() to also accept a string in which case we convert it to an array there. |
| foreach ($this->getNode('resources') as $resource) { | ||
| $resources = $this->getNode('resources'); | ||
|
|
||
| if ($resources instanceof \Twig_Node_Expression_Array) { |
There was a problem hiding this comment.
what if it is a variable containing an array ? Will it be a Twig_Node_Expression_Array ?
There was a problem hiding this comment.
I mean no... but we don't care any more as there is now a single generic case
|
I'd prefer a named argument instead of an ubiquitous "with" keyword which does not really tell me what's coming next. Something like |
|
@schmittjoh it would only make sense if there are multiple named arguments. With only one available it seems redundant. |
|
I tend to agree with @Tobion but I'll have a closer look at assetic to see if we can make things more consistent. 8000 |
|
This would be more consistent with assetic, but assetic isn't really consistent with anything else in twig, although I see the benefits in that particular case for swapping and omitting parameters. |
|
My goal was not really consistency, but I simply find it more obvious, To me explicit naming is superior, but just my 2c On Wed, Mar 14, 2012 at 4:36 AM, Jordi Boggiano <
|
|
When I first saw this tag I didn't understand the role of first parameter. |
|
Before we complicate this any further can I add another thing here. |
|
@mvrhov that is not the good place to discuss this (both this particular issue and GH as this is a support request). Have you tried |
|
Where do you think we should go:
Let's discuss the structure first & not the details (i.e. src vs templates). |
|
I tend to |
|
@vicb: I like 1) more than 2) as this how the built-in tags work. To keep BC even further, can we just remove the |
|
For newcomers 2) is definitely easier to understand. But it would also only make sense if you can change the parameter order, so |
|
@fabpot removing the |
Commits
-------
0c83c5d [Form] Alternate syntax for form_theme
Discussion
----------
[RFC][Form] Alternate syntax for form_theme
before
`{% form_theme form _self "::base.html.twig" %}`
after
`{% form_theme form with "::base.html.twig" %}`
`{% form_theme form with varTheme %}`
`{% form_theme form with [_self, "::base.html.twig"] %}`
_the former syntax is still supported_
---------------------------------------------------------------------------
by stof at 2012-03-12T15:42:32Z
do you really need ``with`` ?
---------------------------------------------------------------------------
by vicb at 2012-03-12T15:50:41Z
it's not needed but I find it more clear (It can be drop if a consensus is reached)
---------------------------------------------------------------------------
by fabpot at 2012-03-12T17:05:46Z
+1 for `with`. Documentation for master should be updated as well.
---------------------------------------------------------------------------
by Tobion at 2012-03-13T02:26:22Z
+1 for `with`, but the syntax without array like `{% form_theme form with "::base.html.twig" %}` should also be supported
---------------------------------------------------------------------------
by vicb at 2012-03-13T07:16:55Z
`[]` are nice as they clearly indicate the ability to use multiple themes (which I think is yet to be documented). We'll pick the most popular syntax only.
---------------------------------------------------------------------------
by stof at 2012-03-13T08:16:40Z
@vicb supporting a string instead of an array should be possible when you need only one element. supporting several ones and turning it into an array is the mistake we made for 2.0
---------------------------------------------------------------------------
by hhamon at 2012-03-13T08:16:45Z
+1 for the new syntax
---------------------------------------------------------------------------
by vicb at 2012-03-13T08:29:45Z
@stof @Tobion what about using the former syntax then ?
---------------------------------------------------------------------------
by Baachi at 2012-03-13T08:32:09Z
+1 for new syntax. But it should be possible to use strings instead of arrays.
---------------------------------------------------------------------------
by stof at 2012-03-13T08:33:07Z
@vicb Having one wyntax using ``with`` and the other without will confuse users IMO. this is why I suggested allowing to pass a Twig array without adding an extra word
---------------------------------------------------------------------------
by stof at 2012-03-13T08:40:02Z
@Baachi not stringS as it is precisely what we are trying to solve :)
---------------------------------------------------------------------------
by Baachi at 2012-03-13T08:42:03Z
Oh sry. I mean __string__. :)
---------------------------------------------------------------------------
by fabpot at 2012-03-13T11:16:30Z
+1 for supporting a string or an array with the new syntax as using only one element is probably the most common use case. But then, why not supporting any valid Twig expression?
---------------------------------------------------------------------------
by vicb at 2012-03-13T11:54:51Z
Something like the latest commit ? (Tests have to be updated).
@fabpot What is the best place to handle array / non-array ? This is currenlty handled in the node but the parser might be a better place.
---------------------------------------------------------------------------
by fabpot at 2012-03-13T13:23:08Z
@vicb: I would just remove the special array case in the node as it's not needed anymore.
---------------------------------------------------------------------------
by fabpot at 2012-03-13T13:24:15Z
... and update FormExtension::setTheme() to also accept a string in which case we convert it to an array there.
---------------------------------------------------------------------------
by schmittjoh at 2012-03-13T14:26:17Z
I'd prefer a named argument instead of an ubiquitous "with" keyword which does not really tell me what's coming next.
Something like ``{% form_theme _form templates=[a, b, c] %}``. This is pretty nicely done for the assetic tags "javascripts", and "stylesheets".
---------------------------------------------------------------------------
by Tobion at 2012-03-13T16:04:26Z
@schmittjoh it would only make sense if there are multiple named arguments. With only one available it seems redundant.
Also `{% form_theme _form templates="template.html.twig" %}` is bad.
---------------------------------------------------------------------------
by vicb at 2012-03-14T07:59:08Z
I tend to agree with @Tobion but I'll have a closer look at assetic to see if we can make things more consistent.
---------------------------------------------------------------------------
by Seldaek at 2012-03-14T10:36:15Z
This would be more consistent with assetic, but assetic isn't really consistent with anything else in twig, although I see the benefits in that particular case for swapping and omitting parameters.
---------------------------------------------------------------------------
by schmittjoh at 2012-03-14T15:49:37Z
My goal was not really consistency, but I simply find it more obvious,
self-explanatory and easier to understand if you name things explicitly. We
are using the "with" keyword in several places and each time something
different is expected.
To me explicit naming is superior, but just my 2c
On Wed, Mar 14, 2012 at 4:36 AM, Jordi Boggiano <
reply@reply.github.com
> wrote:
> This would be more consistent with assetic, but assetic isn't really
> consistent with anything else in twig, although I see the benefits in that
> particular case for swapping and omitting parameters.
>
> ---
> Reply to this email directly or view it on GitHub:
> #3576 (comment)
>
---------------------------------------------------------------------------
by Tobion at 2012-03-14T16:48:01Z
When I first saw this tag I didn't understand the role of first parameter.
So if we use johannes suggestion it should rather be `{% form_theme form=myForm templates=[a, b, c] %}`
---------------------------------------------------------------------------
by mvrhov at 2012-03-14T18:09:09Z
Before we complicate this any further can I add another thing here.
Moving to dedicated issue: Inflexible form theming #3598
---------------------------------------------------------------------------
by vicb at 2012-03-14T18:20:54Z
@mvrhov that is not the good place to discuss this (both this particular issue and GH as this is a support request).
_Have you tried `{% form_theme form.subForm ... %}`_
---------------------------------------------------------------------------
by vicb at 2012-03-15T07:39:14Z
Where do you think we should go:
1. `{% form_theme form with [_self, "::base.html.twig"] %}`
2. `{% form_theme form=form src=[_self, "::base.html.twig"] %}`
Let's discuss the structure first & not the details (i.e. src vs templates).
---------------------------------------------------------------------------
by Baachi at 2012-03-15T07:52:51Z
I tend to ```{% form_theme form with [_self, "::base.html.twig"] %}```, because its more consistent to the twig syntax.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T13:10:56Z
@vicb: I like 1) more than 2) as this how the built-in tags work.
To keep BC even further, can we just remove the `with` keyword? To make it BC, we just need to have a look at extra parameters and add it to an array if they exist.
---------------------------------------------------------------------------
by Tobion at 2012-03-15T13:19:52Z
For newcomers 2) is definitely easier to understand. But it would also only make sense if you can change the parameter order, so `{% form_theme form=form src=[_self, "::base.html.twig"] %}` == ` {% form_theme src=[_self, "::base.html.twig"] form=form %}`. At the same time it reduces consistency. So for experienced developers option 1) [without "with"] is less redundant and preferable.
---------------------------------------------------------------------------
by vicb at 2012-03-15T13:53:49Z
@fabpot removing the `with` will make `Parser::parsePostfixException()` scream when providing an array of themes.
|
The "with" part of this is not documented on the cookbook form customisation page (http://symfony.com/doc/current/cookbook/form/form_customization.html) but is needed if you want to pass an array. Could a note about this be added somewhere? If I hadn't found this thread I was about to give up on passing an array! |
|
Please open an issue in the issue tracker of the symfony-docs repo for it |
before
{% form_theme form _self "::base.html.twig" %}after
{% form_theme form with "::base.html.twig" %}{% form_theme form with varTheme %}{% form_theme form with [_self, "::base.html.twig"] %}the former syntax is still supported