Commits
-------
abd60ac [WebProfilerBundle] Do not display toolbar loading result if it's not a valid toolbar
406c8d8 [WebProfilerBundle] Make toolbar loading non-blocking
Discussion
----------
Non-blocking WDT & prevents garbage to slip in the page
I made the loading non-blocking so that it's not preventing normal operation of the page when the WDT takes a bit long to come up (happens sometimes when the machine is busy).
The second commit also checks that the response looks correct, to prevent stack traces and such to appear in the page if there was a problem. The main issue is not really stack traces though it's mostly with security and intercept_redirect enabled, if you look at a fully secured site you get twice the redirect intercept message to the login page.
Tested in IE7/9/FF4/Opera11
Commits
-------
f315ad9 [WebProfilerBundle] Make sure the toolbar closes properly
Discussion
----------
[WebProfilerBundle] Make sure the toolbar closes properly
Due to the whitespace element between the div which clears and the toolbar div, in some browsers it was left over after you close the toolbar, this doesn't happen anymore.
Tested in IE7/9/FF4/Opera11
Commits
-------
6c46a3b [FrameworkBundle] Prevent breakage when an array callback is not callable
Discussion
----------
[FrameworkBundle] Prevent breakage when an array callback is not callable
W/o this you get warnings that objects can't be converted to strings.
Commits
-------
f400124 [HttpFoundation] Small optimization of Session flash handling
Discussion
----------
[HttpFoundation] Small optimization of Session flash handling
Due to copy-on-write, this is faster and takes less memory unless the flashes are changed during the request, but that's not very likely as typically you set flashes, redirect, then show them, and at that point you do not modify them again.
Commits
-------
72d0ebe9 [WebProfilerBundle] Added the support of the the logging context in the template
410b3e0 [HttpKernel] Added the context in the LoggerInterface
Discussion
----------
context in the LoggerInterface
This adds the context in the LoggerInterface. The change is totally BC for people using the logger. However this affects people implementing the interface.
Note that this require Seldaek/monolog#33 for the implementation
---------------------------------------------------------------------------
by Seldaek at 2011/06/17 04:24:18 -0700
@fabpot: just ping me when you are merging this one, so I can merge in monolog and we avoid out-of-sync issues.
---------------------------------------------------------------------------
by stof at 2011/06/17 04:49:05 -0700
@Seldaek you can merge in Monolog when you want. Monolog is BC so merging it before the PR in Symfony2 does not break things.
---------------------------------------------------------------------------
by Seldaek at 2011/06/17 05:08:34 -0700
Ah right, I thought the interfaces wouldn't match, but PHP allows extra args it seems so I'll merge right now.
---------------------------------------------------------------------------
by stof at 2011/06/17 05:32:58 -0700
PHP allows extra *optionnal* args and it is the case here :)
---------------------------------------------------------------------------
by Seldaek at 2011/06/17 05:35:00 -0700
Well yes otherwise you break the interface. Anyway it's merged so @fabpot, anytime :)
Commits
-------
98b6f9b Fixed tests
ad1b690 [DoctrineBundle] Updated the XSD schema
41347bc [DoctrineBundle] Added the support of custom types for the platform
Discussion
----------
Doctrine mapping types
This adds the full support of Doctrine mapping type, allowing to use them in the SchemaTool. It closes#1349
---------------------------------------------------------------------------
by stof at 2011/06/17 02:05:45 -0700
See also symfony/symfony-docs#414 for the doc
---------------------------------------------------------------------------
by fabpot at 2011/06/17 02:18:03 -0700
Tests do not pass.
---------------------------------------------------------------------------
by stof at 2011/06/17 02:25:25 -0700
Fixed
---------------------------------------------------------------------------
by beberlei at 2011/06/17 03:27:58 -0700
+1
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
-------
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 !
Commits
-------
49dd558 Couple more CS fixes
5a986bf Add $keysCount & minor CS fix
91f4097 [Routing] Better nesting for RouteCollections in dumped URL matcher classes With this change, a route prefixed with '/blogger' will be nested inside '/blog' (for example)
Discussion
----------
[Routing] Better nesting for RouteCollections in dumped URL matcher classes
Consider the following routing file:
_blog:
resource: "@AcmeDemoBundle/Resources/config/routing/BlogController.yml"
prefix: /blog
_blogger:
resource: "@AcmeDemoBundle/Resources/config/routing/BloggerController.yml"
prefix: /blogger
_bloggeroo:
resource: "@AcmeDemoBundle/Resources/config/routing/BloggerooController.yml"
prefix: /bloggeroo
_blogtest:
pattern: /blogtest
defaults: { _controller: AcmeDemoBundle:Blog:test }
login:
pattern: /login
defaults: { _controller: AcmeDemoBundle:Security:login }
Note: Each imported file contains two simple blog/blogger/bloggeroo routes (e.g. blog_index & blog_view)
With this change, the cached URL matcher looks something like this:
```php
<?php
class appprodUrlMatcher
{
public function match($pathinfo)
{
$allow = array();
if (0 === strpos($pathinfo, '/blog')) {
// blog_index
if (preg_match('#^/blog/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...}
// blog_view
if (preg_match('#^/blog/(?P<year>\d{4})/(?P<month>\d{2})/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...}
if (0 === strpos($pathinfo, '/blogger')) {
// blogger_index
if (preg_match('#^/blogger/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...}
// blogger_view
if (preg_match('#^/blogger/(?P<year>\d{4})/(?P<month>\d{2})/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...}
if (0 === strpos($pathinfo, '/bloggeroo')) {
// bloggeroo_index
if (preg_match('#^/bloggeroo/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...}
// bloggeroo_view
if (preg_match('#^/bloggeroo/(?P<year>\d{4})/(?P<month>\d{2})/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...}
throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
}
throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
}
// _blogtest
if ($pathinfo === '/blogtest') {...}
throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
}
// login
if ($pathinfo === '/login') {...}
throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
}
}
```
As far as I can see, you can throw the 404 earlier (as I've done), as by nesting all these possibilities, there is no chance another route will be matched outside the parent if statement.
---------------------------------------------------------------------------
by stloyd at 2011/06/11 08:37:15 -0700
You should follow Symfony [CS rules] (http://symfony.com/doc/current/contributing/code/standards.html), also you should modify tests.
---------------------------------------------------------------------------
by lmcd at 2011/06/11 08:46:32 -0700
Besides the inline `continue` statement, I can't see spot any other CS violations. I'm on to the updated tests.
---------------------------------------------------------------------------
by fabpot at 2011/06/13 02:19:14 -0700
What if you have this:
_blog:
resource: "@AcmeDemoBundle/Resources/config/routing/BlogController.yml"
prefix: /blog
login:
pattern: /login
defaults: { _controller: AcmeDemoBundle:Security:login }
_blogger:
resource: "@AcmeDemoBundle/Resources/config/routing/BloggerController.yml"
prefix: /blogger
You cannot send 404 early because the same `blog` prefix is used in two different places. I can see many things that will break with this patch. I'm pretty sure we can enhance the performance of the existing code but let's do that after 2.0.
---------------------------------------------------------------------------
by lmcd at 2011/06/13 05:04:03 -0700
@fabpot: The code was designed so it would work in these kind of circumstances. I ran the dumper against the routing file you provided and this is the output below.
I see no reason why you can't throw the 404 earlier, as by nesting everything with similar prefixes, we're eliminating the possibility that a route will ever be matched outside of that if statement.
```php
public function match($pathinfo)
{
$allow = array();
if (0 === strpos($pathinfo, '/blog')) {
// blog_index
if (preg_match('#^/blog/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {
...
}
// blog_view
if (preg_match('#^/blog/(?P<year>\d{4})/(?P<month>\d{2})/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {
...
}
if (0 === strpos($pathinfo, '/blogger')) {
// blog_index
if (preg_match('#^/blogger/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {
...
}
// blog_view
if (preg_match('#^/blogger/(?P<year>\d{4})/(?P<month>\d{2})/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {
...
}
throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
}
throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
}
// login
if ($pathinfo === '/login') {
...
}
throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
}
```
---------------------------------------------------------------------------
by fabpot at 2011/06/13 07:48:49 -0700
Can you add some more tests to ensure that everything works fine and that we won't have regressions later on? Thanks.
Commits
-------
e718474 fixed CS and unsude variable (PR #1319)
b552354 Executable Finder will now try to match if a file is already in the open_basedir list before searching it thru that list
Discussion
----------
open_basedir exhancements for Executable Finder
I've made the some changed to the Executable Finder in order to handle the open_basedir ability to specify files in the list not just directories.
This should fixsymfony/symfony#1319
Hope I didn't made any CS violations.
Commits
-------
9d6357c [HttpFoundation] Document the changes to the File classes
136b80a [HttFoundation] Add File::getExtension() as \SplFileInfo::getExtension() was introduced in PHP 5.3.6
38b3b74 [HttpKernel] Fix and test previous commit
ac0c00c [HttpFoundation] Make File extends \SplFileInfo
Discussion
----------
[HttpFoundation] Make File extends \SplFileInfo
This is a rebased version of [PR 674](https://github.com/symfony/symfony/pull/674).
* File: The API has changed (now extends \SplFileInfo),
* File: move() creates the target directory when it does not exist
* UploadedFile: introduction of getClientXXX() methods (for Size, OriginalName, MimeType)
If this PR does not get merged UploadedFile should at least be fixed: [Client.php](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Client.php#L124) relies on a last parameter which is no more defined and which is used to bypass [move_uploaded_file()](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/File/UploadedFile.php#L155) in test mode.
If this could be merged, I'll detail the changes in UPDATE.md
---------------------------------------------------------------------------
by fabpot at 2011/06/14 08:20:59 -0700
I'll merge it. Can you update the UPDATE file?
---------------------------------------------------------------------------
by vicb at 2011/06/14 09:24:01 -0700
done
Commits
-------
4016dfb [AsseticBundle] moved ExecutableFinder back into a closure so it's only called if needed
Discussion
----------
[AsseticBundle] moved ExecutableFinder back into a closure
[AsseticBundle] moved ExecutableFinder back into a closure so it's only called if needed
---------------------------------------------------------------------------
by fabpot at 2011/06/14 05:46:14 -0700
Does it relate to #1319?
---------------------------------------------------------------------------
by kriswallsmith at 2011/06/14 05:47:25 -0700
It's related, yes, but I don't think it's a solution for the issue.
---------------------------------------------------------------------------
by dlsniper at 2011/06/14 06:00:55 -0700
The issue in #1319 has been fixed with this: 0d54f5c21e
It's in the latest master tree but finding that file only when needed is also a good idea.
# Commits
ca52a04 [Validator] Allow DateTime objects as valid Times
# Discussion
## [Validator] Allow DateTime objects as valid Times
Also added tests for `DateTime` objects as valid on `Date` and `Time` constraints.
I didn't include the test for the `DateTime` constraint, as it's already included in this PR:
https://github.com/symfony/symfony/pull/1085
---------------------------------------------------------------------------
## fabpot @ 2011/06/09 09:07:21 -0700
I don't think it makes sense to use a \DateTime instance to represent a Time.
---------------------------------------------------------------------------
## ajessu @ 2011/06/09 09:33:20 -0700
If I have an entity with a doctrine type `Time`:
Time (DateTime instance where only H:i:s get persisted)
```php
<?php
/**
* @ORM\Column(type="time")
* @Assert\Time()
*/
protected $startTime;
```
and I create a form out of this Entity, a `DateTime` object is passed when the form is submitted.
This generates an `UnexpectedTypeException`.
I just made this change to match the `Date` validator with the doctrine type `Date`, which also shares this behavior:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/DateValidator.php#L28
Date (DateTime instance where only Y-m-d get persisted)
Rules are as follows:
* If multiple is true, then the empty_value is ignored
* If not, and if the field is not required, the empty_value is set to the empty string by default and displayed
* If the field is required, and if the user explicitely set the empty_value, then it is displayed
* kriswallsmith/form/collection-proto:
added script[type="text/html"] collection prototype to form themes
[Form] removed collection prototype from form tree
* vicb/form-render-fix:
[Form][TwigBridge] Improve the cache layer by caching blocks instead of templates
[Form][TwigBridge] Make the template cache more efficient
[Form][TwigBridge] Fix rendering
This reverts commit d044498cde.
The reason for reverting is that the name is actually used to customize
the template on a per field basis:
{% block _post_excerpt_widget %}
***{{ block('text_widget') }}***
{% endblock %}
Here, post is the name of the Type.
The definition "assetic.helper.static" references the service "templating.helper.assets" which belongs to a narrower scope. Generally, it is safer to either move "assetic.helper.static" to scope "request" or alternatively rely on the provider pattern by injecting the container itself, and requesting the service "templating.helper.assets" each time it is needed. In rare, special cases however that might not be necessary, then you can set the reference to strict=false to get rid of this error.
* theinterned/patch-1:
made logoutPath localizable as well
storing localized targetPath in a string as opposed to updating the attribute
In the spirit of 882a8e3f09 allow for localized logout target url
* kriswallsmith/form/lazier-csrf-token:
[Form] fixed xpath
[Form] moved csrf listener to its own class
fix issue with csrf token not present on collection fields because of resize listener
* lmcd/master:
$code referenced but not defined in compileRoute()
[Routing] Optimised the PHP URL matcher dumper The cached URL matcher classes contain some unneeded logic. Consider the following example:
The cached URL matcher classes contain some unneeded logic. Consider the following example:
if (0 === strpos($pathinfo, '/Blog')) {
// blog_index
if (0 === strpos($pathinfo, '/Blog') && preg_match('#^/Blog/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {
return array_merge($this->mergeDefaults($matches, array ( '_action' => 'index',)), array('_route' => 'blog_index'));
}
}
The 2nd strpos is not required, as we have already satisfied this condition in the parent if statement.
My change will produce the following code for the same routing setup::
if (0 === strpos($pathinfo, '/Blog')) {
// blog_index
if (preg_match('#^/Blog/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {
return array_merge($this->mergeDefaults($matches, array ( '_action' => 'index',)), array('_route' => 'blog_index'));
}
}
* lsmith77/serializer_tweaks: (22 commits)
clarified that BC is broken in the Serializer component
added UPDATE notes for Serializer component changes
fix tests
marked public api
more encoder lazy loading tweaks
always use getEncoder() to enable lazy loading
cosmetic tweak
removed setEncoder/removeEncoder/addNormalizer/removeNormalizer
SerializerAwareInterface and DecoderInterface do not implement EncoderInterface anymore
handle non objects
moved the methods that can later be moved to a Builder to the bottom
use getEncoder inside encode/decode
made serialize/deserialize/encode/decode final
added Constructor
added Exception's from SerializerBundle
made (de)normalizeObject() private
renamed hasEncoder/hasDecoder to supportsSerialization/supportsDeserialization
notice fixes
typo fixes
all encoders implement EncoderInterface
...
* lmcd/master:
Added an optimisation for PHP-FPM (FastCGI Process Manager). As soon as a full Response is dispatched to the browser, the HTTP connection is closed, but the script stays alive on FPM servers.
* hhamon/doctrine_metadatafactory_fix:
[DoctrineBundle] fixed missing backslashe.
[DoctrineBundle] simplified getClassMetadataFactoryClass() method in both DisconnectedMetadataFactory and MetadataFactory classes.
[DoctrineBundle] added new DisconnectedMetadataFactory class that is now used in the doctrine:generate:entities command instead of the MetadataFactory class.
anything else is a total edge case that doesnt break with this change. it just means that for that edge case it will not be possible to "statically" determine if the encoder doesnt actually support encoding.
actually the main methods I am looking for is hasDecoder() and getEncoder() to be able to check if there is a Decoder to decode the Request body as well as if the encoder implements the TemplatingAwareInterface
The current implementation is not ready for inclusion in 2.0. It has several
known problems (security, not possible to disable it, not "cloud-compatible",
...) and it's not a must have feature anyway.
Some references:
* Security issue in FileType: https://github.com/symfony/symfony/issues/1001
* Validation fails on file, still stored in TemporaryStorage: https://github.com/symfony/symfony/issues/908
* Add a size argument & ability to configure TemporaryStorage: https://github.com/symfony/symfony/pull/748
This feature should be reworked and discussed for inclusion in 2.1.
* yethee/doctrine_bridge:
[DoctrineBridge] Removed all options of the parent type
[DoctrineBridge] Removed unneeded use statement
[DoctrineBridge] Fixed typo
[DoctrineBridge] Removed duplicating options in EntityType
* added a RegistryInterface
* changed all classes to depend on the Registry instead of a specific EntityManager
This is more consistent as the validator already took the registry and this allows
to use any entity manager in Forms.
* beberlei/AsseticDirectoryResource:
[Assetic] Make AsseticExtension::createDirectoryResourceDefinition public, so that third party bundels can easilycreate definitions to search for assetic formulae in additional directories. LiipThemeBundle needs this to make the app/Resources/themes and Bundle/Resources/themes folder searchable. Without this change the whole method would need to be copy pasted. Since the method has no side-effects at all (only factory, not pushing into the container) this change is justifyable imho.
This fixes choices when all the keys are strings (but integers really).
That's because in PHP, if you have the following array:
array('1' => 'foo', '2' => 'bar');
PHP "converts" it automatically to the following array:
array(1 => 'foo', 2 => 'bar');
* fivestar/single-choice-expanded:
[Form] Fixed FixRadioInputListener to not ignore 0.
[Form] Fixed single expanded choice type to set checked attribute when passed boolean value
* vicb/form-rendering-fix:
[Form] Fix accessibility for file inputs
[FrameworkBundle] Fix the FormHelper phpDoc
[FrameworkBundle][Form] Add some phpDoc for the FormHelper class
[FrameworkBundle][Form] Fix label rendering
[FrameworkBundle][Form] Fix rendering search inputs in PHP
[Form] FormType labels should never have a for attribute
[Form] Never render a view again
* The command names have now full support for nested namespaces. It means
that abbreviations work for each sub-namespace:
./app/console doctrine:mapping:info
# worked before
./app/console doctrine:map:in
# works now
./app/console doc:map:in
* Aliases are now first class citizen. They can have their own namespace,
like the main name. So, now, there is no difference between an alias and a
name.
* As names and aliases can be namespaced, the Command::getFullName() and
Command::getNamespace() method have been removed.
Here are the new simplified rules:
* Required cache warmers are *always* executed when the Kernel boots for the first time;
* Optional cache warmers are *only* executed from the CLI via cache:warmup
These new rules means that all the configuration settings for the cache
warmers have been removed. So, if you want the best performance, remember to
warmup the cache when going to production.
This also fixed quite a few bugs.
* gordonslondon/http-foundation/response:
[HttpFoundation] merge Response::isRedirected() with Response::isRedirect() - Response::isRedirected() has been removed
If some of the nested views are rendered individually they should not be rendered again when calling form_rest.
A typical would be when some nested file views are rendered, form_rest should not render them again.
It is still possible to render a label once the widget has been rendered. This is for checkboxes and radios
where the widget is typically rendered before the label.
* jwage/master:
Revert back to using gmmktime and use day 15 instead of 1 to avoid edge case
Use mktime instead of gmmaketime. On the 1st of every month the following options are generated:
