Commits
-------
5b0f1da [HttpKernel] made WebTestCase methods static
Discussion
----------
[HttpKernel] made WebTestCase methods static
This makes it possible to load fixture data in `::setUpBeforeClass()` which makes tests run much faster.
Also, `createClient()` is not protected instead of public; I'm not sure why it was public in the first place.
Commits
-------
edf4b87 Add missing "tearDown" functions, and some missing variable declaration (this saves for me almost 20MB when run all tests) Force AsseticBundle tests to use TestCase Fix test for DoctrineBundle to use TestCase
2b0c352 Increase code coverage for: YamlParser, Validators, PhpEngine + Helpers, HttpFoundation
b88a0a0 Remove tabs
99f9337 Additional tests for PhpEngine + Helpers More tests for UrlValidator
450ed85 Additional tests for DateTimeValidator, EmailValidator and UrlValidator
Discussion
----------
[Tests] Cleanup + make code coverage more happy
Hey,
this PR is a bit bigger than usually ;-) few infos what's inside:
- Fix `DoctrineBundle` test to use `TestCase`
- Mark tests as "incomplete" instead of commenting them out
- Increase code coverage for: `Validators`, `PhpEngine` + `Helpers`, `HttpFoundation` (`Session`, `Response` etc.)
- And my favourite ;-) added missing variables definition (also removed non-used) and `tearDown()` function (if needed) to tests which allowed me saved __~15MB__ when running all tests
---------------------------------------------------------------------------
by stloyd at 2011/06/16 05:58:21 -0700
@fabpot & @marcw It was rebased and cleanup up (I split up `AsseticBundle` symfony/AsseticBundle#1 change to new repo), and added few new tests.
Commits
-------
6436da2 added quote
7dc4e24 [Tests][Locale] locale supported INTL_ICU_VERSION, use ReflectionExtension to find ICU version
Discussion
----------
[Tests][Locale] locale supported INTL_ICU_VERSION, use ReflectionExtension to find ICU version
---------------------------------------------------------------------------
by igorw at 2011/06/16 03:44:25 -0700
Nice.
`INTL_ICU_VERSION` is PHP 5.4 only, right?
Also, it may be possible to use:
$output = \ReflectionExtension::export('intl', true);
That way we can get rid of the output buffering.
---------------------------------------------------------------------------
by stloyd at 2011/06/16 03:49:22 -0700
@igorw It will be added in PHP 5.3.7 to.
---------------------------------------------------------------------------
by stealth35 at 2011/06/16 04:04:09 -0700
@igorw no export don't return the info table
Commits
-------
7d09695 [FrameworkBundle] Simplified TemplateReference::getPath() method and added a unit test.
2616efb [FrameworkBundle] fixed TemplateRefence::getPath() when using namespaced controllers (i.e: AcmeBlogBundle\\Controller\\Admin\\PostController)
Discussion
----------
[FrameworkBundle] fixed TemplateRefence::getPath() when using namespaced
[FrameworkBundle] fixed TemplateRefence::getPath() when using namespaced controllers (i.e: AcmeBlogBundle\\Controller\\Admin\\PostController)
Commits
-------
72c074a [Session] Used \Locale::setDefault() when the locale is setted
Discussion
----------
[Session] Used \Locale::setDefault() when the locale is setted
For `DateType` in form component (by example), `\Locale::getDefault()` is used to displayed the name of months.
If `\Locale` class is not used when the locale is setted in the session, the name of months is not in a good language.
This PR solves this problem.
---------------------------------------------------------------------------
by pborreli at 2011/05/29 09:13:44 -0700
what if user doesn't have intl extension ?
---------------------------------------------------------------------------
by stof at 2011/05/29 09:24:04 -0700
You should wrap the calls to ``\Locale::setDefault`` in a ``class_exist`` check to avoid issue when using the stub implementation (for which calling ``setDefault`` is forbidden).
---------------------------------------------------------------------------
by francisbesset at 2011/05/29 09:26:40 -0700
@pborreli: Symfony have a fake Locale class and this class is used only if the server haven't intl enabled.
---------------------------------------------------------------------------
by stof at 2011/05/29 09:33:16 -0700
@francisbesset Yeah, but ``setDefault`` throw a ``BadMethodCall`` exception.
and so the check has to use ``extension_loaded`` instead of ``class_exists``.
---------------------------------------------------------------------------
by fabpot at 2011/06/13 10:12:15 -0700
Ticket #1121 is related to this PR.
---------------------------------------------------------------------------
by fabpot at 2011/06/15 06:18:28 -0700
I have just tried another implementation where the locale is passed as an argument to the built-in types and some data transformers (via a `LocaleAwareInterface` interface). That works fine as forms are immutable now, but the solution is obviously more "complex" as we need to pass the locale to many different classes. Also, using `Locale::setDefault()` has an advantage over my method: you can change the locale whenever you want within a PHP process (which can be useful even if this is an edge case). Last, but not the least, if make sense to update the PHP Locale to the user locale.
So, to sum up, this patch is probably the best solution (easy and flexible enough).
Commits
-------
e8326aa Renamed attributes to attr to be consistent with templating.
c707467 Added support for additional attributes in Form types that list field as their parent.
Discussion
----------
[Form] Added support for additional attributes
Added support for additional attributes in Form types that list field as their parent.
This is needed particularity for html5 data and data- attributes support, unfortunately $options['data'] is already taken, so adding a general $options['attributes'] is the easiest solution without breaking BC.
Now I know that this will be tempting for some to stuck style and class attributes here also, but I'd rather not restrict the keys that are passed in.
---------------------------------------------------------------------------
by Seldaek at 2011/05/22 14:51:02 -0700
Maybe it should be called attr for consistency with the template stuff, or the other should be renamed attributes. Other than that, I'm +1, data-* attributes are awesome, and abusers will find ways to abuse things either way.
---------------------------------------------------------------------------
by mvrhov at 2011/05/22 21:48:01 -0700
Naming it attr also crossed my mind when I was signing off yesterday.
Along with the possibility to go the way xml attributes are handled when node is converted to/from array.
So every option with @ prefix would automatically become html attribute. However going the latter path, it'd be harder to implement.
---------------------------------------------------------------------------
by fabpot at 2011/06/13 07:43:52 -0700
Can you give an example of a real-world use case? Thanks.
---------------------------------------------------------------------------
by Seldaek at 2011/06/13 07:54:51 -0700
You can use `array('attr' => array('data-foo' => 'bar'))`, which will output `data-foo="bar"`, which can be read easily by jQuery for example as `$('el').data('foo')`. It's a standard compliant and elegant way to pass extra data needed by the JS code along with DOM nodes, without polluting everything with script tags and all.
---------------------------------------------------------------------------
by fabpot at 2011/06/13 08:01:08 -0700
@Seldaek: I understand that. But why not doing this in the template?
---------------------------------------------------------------------------
by Seldaek at 2011/06/13 08:04:10 -0700
Well, I agree it most likely belongs in the template, but it's kind of data stuff that is not directly impacting the display rules of the element, so in some cases having the possibility to set that from the php code might be useful. Anyway I'll let @mvrhov answer maybe he had a more concrete use case. I just think it's nice to leave the door open, but I don't really need it.
---------------------------------------------------------------------------
by mvrhov at 2011/06/13 10:27:03 -0700
A bit late to the party. Ok, here is my use-case.
I have a pretty large form where part of the data is of tabular form. The number of rows is almost every time a lot larger than the number of columns
So I can either output a lot of text inputs filled with data and make already a large form intimidating. Or I can use a grid that supports editing. So I serialize that tabular data as json and put it as a value into one hidden field. Somehow I also have to get the column definitions to that grid. I decided to I serialize it and put it inside data-* attribute. Putting it into another hidden field doesn't make sense as when data is submitted back I don't need the column definitions as only the number of rows changes.
---------------------------------------------------------------------------
by fabpot at 2011/06/13 10:44:58 -0700
@mvrhov: ok, but what prevents you from doing this in the template directly?
---------------------------------------------------------------------------
by mvrhov at 2011/06/13 11:22:53 -0700
I have to get it into the view somehow. What I'd really not like to do is, iterate through that data in a controller and then pass it as another template variable.
---------------------------------------------------------------------------
by fabpot at 2011/06/14 00:10:22 -0700
But the controller is where you prepare the data that you want to send to the view. Without any concrete example, I'm going to close this PR.
---------------------------------------------------------------------------
by mvrhov at 2011/06/14 01:21:10 -0700
IMHO this has to go out through form as this is the part of the form also I do have a very slim controllers in this case 10 LOC, where half of them is just setting up an view data..
Nonetheless I went looking again very closely at the AbstractType and I do have buildView function available which I can override and set the column data I need there, and then provide custom view for that, so at least from this part this is an non issue.
With this PR all default form attributes can be set from outside and when searching for a good use-case I have found out that @henrikbjorn has implemented this via extensions [1], maybe he has a good use-case for this.
[1] - https://github.com/Comways/ComwaysFormExtraBundle/blob/master/Form/Extension/FieldTypeExtension.php
---------------------------------------------------------------------------
by henrikbjorn at 2011/06/14 01:48:53 -0700
Convenience is the only use case
---------------------------------------------------------------------------
by stof at 2011/06/14 02:08:09 -0700
@fabpot The issue is that passing it from the controller as another template variable makes it really hard when you use the type twice with different values. Passing them from the form would be the easiest way
---------------------------------------------------------------------------
by shuogawa at 2011/06/14 19:37:50 -0700
hello. thanks for great form library.
I want to support two ways to display additional attributes for form elements.
1) control in template like
{{ form_row(form.name, { 'width': '30' }) }}
2) control from php
$builder->add('name', 'text', array('attr'=>array('width'=>'30')));
If form elements configure by end user like cms,
and form elements dynamically change.
The second method is useful.
template designer can write {{form_row(form)}}
but
template designer can not write {{form_row(form.name), { 'width': '30' }}}
Thank you
---------------------------------------------------------------------------
by fabpot at 2011/06/14 23:01:18 -0700
@shuogawa: That's what I fear. Setting the `width` or any other attribute in PHP is wrong. This belongs to the templates.
---------------------------------------------------------------------------
by stloyd at 2011/06/15 00:09:05 -0700
@fabpot Then maybe just restrict allowed tags to `data-*` and don't use `attr` but some other not confusing name ? Just like we do with i.e. `maxlength`.
---------------------------------------------------------------------------
by fabpot at 2011/06/15 04:44:25 -0700
I'm going to merge it as a "convenience" tool, but the documentation should clearly state that the only usage should be for `data-*` attributes.
Commits
-------
cdf4b6a Checked log levels
a45d3ee Reverted last commit
529381b ControllerNotFound: Changed log level from info to error. Also moved throw exception code block up, to prevent the message from beeing logged multiple times.
7c29e88 Changed log level of "Matched route ..." message from info to debug
dca09fd Changed log level of "Using Controller ..." message from info to debug
Discussion
----------
Log levels
Just wanted to ask if the log level INFO is still correct for these messages?
As there are only four log levels left (DEBUG, INFO, WARNING, ERROR), DEBUG might be the more appropriate level for these messages now.
Let me give an example: An application is logging user actions (maybe to database) in order to assure comprehensibility, e. g. "User %s deleted post %d", "User %s written a message to user %s". These are not warnings of course, so the only suitable log level is INFO.
But they will be thrown together with these very common (at least two per request?) "Using controller..." and "Matched route..." messages when choosing INFO as log level.
---------------------------------------------------------------------------
by Seldaek at 2011/05/24 07:13:18 -0700
Agreed, this stuff is framework debug information.
---------------------------------------------------------------------------
by fabpot at 2011/05/24 08:53:24 -0700
Why do you want to change these two specific ones? The framework uses the INFO level at other places too. Is it a good idea to say that the framework only logs with DEBUG?
---------------------------------------------------------------------------
by stof at 2011/05/24 09:12:53 -0700
Doctrine logs at the INFO level too and I think it is useful to keep it as INFO. Being able to see the queries without having all DEBUG messages of the event dispatcher and security components is useful IMO.
---------------------------------------------------------------------------
by Seldaek at 2011/05/25 02:30:24 -0700
Yeah, that's true, maybe we just need to reintroduce (again, meh:) NOTICE between INFO and WARNING.
@kaiwa Of course the other way could be that you just add your DB handler to the app logger stack. That could be done in a onCoreRequest listener or such, basically you'd have to call `->pushHandler($yourDBHandler)` on the `monolog.logger.app` service. That way your messages will flow to it, but it won't receive noise from the framework stuff since those log on monolog.logger.request and other log channels.
---------------------------------------------------------------------------
by fabpot at 2011/05/25 02:48:26 -0700
@Seldaek: I don't think we need another level. We just need to come up with a standard rules about the usage of each level. Adapted from log4j:
* ERROR: Other runtime errors or unexpected conditions.
* WARN: Use of deprecated APIs, poor use of API, 'almost' errors, other runtime that are undesirable or unexpected, but not necessarily "wrong" (unable to write to the profiler DB, ).
* INFO: Interesting runtime events (security infos like the fact the user is logged-in or not, SQL logs, ...).
* DEBUG: Detailed information on the flow through the system (route match, security flow infos like the fact that a token was found or that remember-me cookie is found, ...).
What do you think?
---------------------------------------------------------------------------
by stloyd at 2011/05/25 02:53:38 -0700
+1 for this standard (also this PR can be merged then), but we should review code for other "wrong" log levels usage (if everyone accept this standard)
---------------------------------------------------------------------------
by fabpot at 2011/05/25 02:55:07 -0700
I won't merge this PR before all occurrences of the logger calls have been reviewed carefully and changed to the right level.
---------------------------------------------------------------------------
by kaiwa at 2011/05/25 02:58:44 -0700
@fabpot: Just noticed these two occurring for every request in my log file. You are right, there are other places where this changes must be applied if we will change the log level.
@stof: Hmm, i see. It is not possible to set the logger separately for each bundle, is it? That maybe would solve the problem. If somebody is interested in seeing the queries, he could set the log handler level to DEBUG for doctrine bundle, but still use INFO for the framwork itself. Plus he could even define a different output file or a completely different handler.
I'm not sure if something like that is possible already (?) or realizable at all... just came into my mind.
---------------------------------------------------------------------------
by Seldaek at 2011/05/25 03:01:07 -0700
Just FYI, from Monolog\Logger (which has CRITICAL and ALERT):
* Debug messages
const DEBUG = 100;
* Messages you usually don't want to see
const INFO = 200;
* Exceptional occurences that are not errors
* This is typically the logging level you want to use
const WARNING = 300;
* Errors
const ERROR = 400;
* Critical conditions (component unavailable, etc.)
const CRITICAL = 500;
* Action must be taken immediately (entire service down)
* Should trigger alert by sms, email, etc.
const ALERT = 550;
The values kind of match http error codes too, 4xx are expected errors that are not really important (404s etc) and 5xx are server errors that you'd better fix ASAP. I'm ok with the descriptions, but I think alert and critical should be included too. I'll probably update Monolog docblocks to match whatever ends up in the docs.
---------------------------------------------------------------------------
by Seldaek at 2011/05/25 03:03:21 -0700
@kaiwa you can do a lot, but not from the default monolog configuration entry, I'm not sure if we can really make that fully configurable without having a giant config mess. Please refer to my [comment above](https://github.com/symfony/symfony/pull/1073#issuecomment-1234316) to see how you could solve it. Maybe @fabpot has an idea how to make this more usable though.
---------------------------------------------------------------------------
by stof at 2011/05/25 03:19:43 -0700
@Seldaek the issue is that the different logging channels are only know in the compiler pass, not in the DI extension. So changing the level in the extension is really hard IMO.
Thus, the handlers are shared between the different logging channels (needed to open the log file only once for instance, or to send a single mail instead of one per channel) and the level is handled in the handlers, not the logger.
I'm +1 for the standard, by adding the distinction between 400 and 500 status calls using ERROR and CRITICAL (which is already the case in the code).
@kaiwa do you have time to review the calls to the logger between DEBUG and INFO or do you prefer I do it ? For instance, the Security component currently logs all message at DEBUG level and some of them should be INFO.
---------------------------------------------------------------------------
by kaiwa at 2011/05/25 04:31:04 -0700
@stof ok i'll do that
---------------------------------------------------------------------------
by kaiwa at 2011/05/25 12:22:51 -0700
Need some help :) I came across `ControllerNameParser::handleControllerNotFoundException()` which leads to redundant log messages currently:
>[2011-05-25 20:53:16] request.INFO: Unable to find controller "AppBaseBundle:Blog" - class "App\BaseBundle\Controller\BlogController" does not exist.
>[2011-05-25 20:53:16] request.ERROR: InvalidArgumentException: Unable to find controller "AppBaseBundle:Blog" - class "App\BaseBundle\Controller\BlogController" does not exist. (uncaught exception) at /home/ruth/symfony3/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php line 87
Is it necessary to call `$this->logger->info($log);` if the InvalidArgumentException will be logged anyway?
---------------------------------------------------------------------------
by stof at 2011/05/25 12:39:22 -0700
Well, the issue is that the ControllerNameParser logs messages and then uses them to throw an exception. I guess the logging call should be removed as it is redundant with the one of the ExceptionListener. @fabpot thoughts ?
---------------------------------------------------------------------------
by kaiwa at 2011/05/27 11:39:25 -0700
I checked all debug, info and log calls. Sometimes it is hard to distinguish between the levels, so it would be great if someone reviews @cdf4b6a. @stof, maybe you want to take a look?
---------------------------------------------------------------------------
by kaiwa at 2011/05/31 12:52:07 -0700
@stof, thanks for your comments. I added some replies above, please let me know your suggestions.
---------------------------------------------------------------------------
by stof at 2011/05/31 14:04:22 -0700
@kaiwa As I said before, all the security logging calls should be DEBUG (most of them) or INFO (the one syaing that authentication succeeded for instance), but not WARN or ERROR as the exception don't go outside the firewall.
The blackhole plugin that was used previously stop the propagation
of events, which means that the behavior can be slightly different
depending on the order of plugin registrations.
Instead, we now use the null transport to avoid this issue.
Commits
-------
07fa82d [Form] Revert changes impacting perfomance and readability
b709551 [Order] Make Form::types and FormView::types use the same order (Parent > Child)
e56dad6 [Form] simplify the code
bdd755e [Form] Fix the exception message when no block is found
c68c511 [Form] Make theming form prototypes consistent (start by looking for a '_<id>_<section>' block)
9ec9960 [Form] Simplify the code
4e3e276 [Form] Make the prototype view child of the collection view
Discussion
----------
[Form] Make the prototype view child of the collection view
This PR should be a base for discussion.
The [current implementation](https://github.com/symfony/symfony/pull/1188) has some drawbacks because the prototype view is not a child of the collection view:
* The 'multipart' attribute is not propagated from the prototype to the collection,
* The prototype view do not use the theme from the collection.
Those 2 points are fixed by the proposed implementation and one more benefit is that the template markup might be easier to work with:
before:
```html
<div id="form_emails">
<div>
<label for="form_emails_0">0</label>
<input type="email" id="form_emails_0" name="form[emails][0]" value="a@b.com">
</div>
<script type="text/html" id="form_emails_prototype">
<div>
<label for="$$name$$">$$name$$</label>
<input type="email" id="$$name$$" name="$$name$$" value="" />
</div>
</script>
</div>
```
after:
```html
<div id="form_emails">
<div>
<label for="form_emails_0">0</label>
<input type="email" id="form_emails_0" name="form[emails][0]" value="a@b.com">
</div>
<script type="text/html" id="form_emails_prototype">
<div>
<label for="form_emails_$$name$$">$$name$$</label>
<input type="email" id="form_emails_$$name$$" name="form[emails][$$name$$]" value="" />
</div>
</script>
</div>
```
@kriswallsmith I'd like to get your feedback on this PR. thanks.
---------------------------------------------------------------------------
by stof at 2011/06/14 07:01:01 -0700
@fabpot any ETA about merging it ? Using the prototype currently is a pain to build the name. The change makes it far easier
---------------------------------------------------------------------------
by fabpot at 2011/06/14 07:09:46 -0700
The templates are much better but I'm a bit concerned that we need to add the logic into the Form class directly. That looks quite ugly. If there is no other way, I will merge it.
---------------------------------------------------------------------------
by vicb at 2011/06/14 07:14:32 -0700
I have found no better way... I am testing some minor tweaks I want to submit.
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 07:34:25 -0700
I'm not happy with the code in Form.php either... would creating a PrototypeType accomplish the same thing?
---------------------------------------------------------------------------
by vicb at 2011/06/14 07:42:07 -0700
@kriswallsmith tried and dismissed, the id and name are bad & you have to go for `render_widget(form.get('proto'))` in the template. That should be fixeable but not any better.
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 07:45:21 -0700
What do you mean the id and name are bad? If we have a distinct type for the prototype, can't we do whatever we want using buildView() and the template?
---------------------------------------------------------------------------
by vicb at 2011/06/14 07:53:31 -0700
@kriswallsmith the id would be smthg like `form_emails_$$name$$_prototype` but yes we should be able to do whatever we want but the code might end up being more complex.
I am done with the tweaks but still open to feedback on this PR.
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 08:08:21 -0700
Yes, that is the type of name I would expect.
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 08:08:33 -0700
Oops -- I mean id.
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 08:09:42 -0700
Maybe I'm confused what id you're referring to. I'll try to spend some time on this today.
---------------------------------------------------------------------------
by vicb at 2011/06/14 08:23:56 -0700
That should be the id of the `<input>`, the id of the script would be `form_emails_$$name$$_prototype_prototype` (if prototype is the name of the nested node).
I am trying to setup a branch with my code (playing with git & netbeans local history)
---------------------------------------------------------------------------
by vicb at 2011/06/14 08:46:25 -0700
@kriswallsmith https://github.com/vicb/symfony/tree/kris/proto if that can help (there are still changes in Form.php)
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 08:47:08 -0700
Thanks, I'll take a look.
---------------------------------------------------------------------------
by vicb at 2011/06/15 00:48:38 -0700
I would have expected it to be faster however `array_map` is about twice slower... reverted !
