Commits
-------
09562df Update CHANGELOG for 2.1, describe new auth events
cf09c2d added authentication success/failure events
Discussion
----------
[Security] Implementation of a "failed login" event, replaces: PR #1307
As I have to use this feature I have completed its implementation.
Bugfix: no
Feature addition: yes
Symfopny2 tests pass: yes
Replaces/closes PR: #1307
---------------------------------------------------------------------------
by schmittjoh at 2011/11/18 23:57:56 -0800
Usually, this event is used for the wrong reasons (to customize what happens on authentication failure). Can you move your implementation to the AuthenticationProviderManager instead?
see https://github.com/schmittjoh/symfony/blob/master/src/Symfony/Component/Security/Core/Authentication/AuthenticationProviderManager.php#L103
---------------------------------------------------------------------------
by canni at 2011/11/19 06:00:36 -0800
Good point :) I'll not rewrite yours work, I've cherry-picked yours commits. (BTW you added call to `setEventDispatcher` on `security.authentication.manager` to commit related to some different work ;)
---------------------------------------------------------------------------
by fabpot at 2011/11/22 00:12:19 -0800
The new files are missing the LICENSE header. As far as I can see, @schmittjoh fork has a different license from the Symfony one. This needs to be clarified before I can merge this PR.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/22 01:53:09 -0800
No biggy, MIT is fine here.
---------------------------------------------------------------------------
by canni at 2011/11/22 01:57:51 -0800
@fabpot done
---------------------------------------------------------------------------
by fabpot at 2011/11/22 02:22:47 -0800
@canni: Can you update the CHANGELOG file (to reference the changes and the BC breaks -- like the move of KernelEvents for instance).
---------------------------------------------------------------------------
by canni at 2011/11/22 02:40:33 -0800
@fabpot: no problem & done
PS I haven't realized that namespace change of `SecurityEvents` is actually a BC Break, thx for pointing this.
---------------------------------------------------------------------------
by fabpot at 2011/11/22 03:06:17 -0800
@canni: What about keeping a `SecurityEvents` class in the `Http` namespace that just extends the new one. That way, we don't break BC.
---------------------------------------------------------------------------
by canni at 2011/11/22 03:53:01 -0800
@fabpot: that will force us to remove `final` keyword form one of classes.
Maybe we can add new, not extending class e.g.: `GeneralSecurityEvents` or `AuthenticationEvents`, that way we dont break BC and dont introduce confusion in naming?
---------------------------------------------------------------------------
by canni at 2011/11/22 05:53:15 -0800
@fabpot: I've removed the BC break, and squashed schmittjoh commits, to keep things nice and clear.
Commits
-------
e7215ae Fix composer.json
Discussion
----------
Fix composer.json files
>=2.1 does not include the 2.1.0-dev versions, since those are lesser than 2.1.0 stable, so nothing is really working right now.
Commits
-------
11b6156 updated unittest
a931e21 get correct client IP from X-forwarded-for header
Discussion
----------
[HttpFoundation] Get correct client IP when using trusted proxy (Varnish)
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Note: This is reopened PR #2686 for 2.0 branch.
If using trusted proxy (Varnish, ...) the client IP must be identified from X-Forwarded-For header. The header has de-facto standard format:
X-Forwarded-For : client1, proxy1, proxy2,
where the value is a comma+space separated list of IP addresses, the left-most being the farthest downstream client, and each successive proxy that passed the request adding the IP address where it received the request from. See: http://en.wikipedia.org/wiki/X-Forwarded-For
Function getClientIp should return only one client IP, not a list of all nonimportant IPs as it's now. Similar example can be seen in Cake framework: http://api.cakephp.org/view_source/request-handler-component/#line-477
There are many ways how to chose the first IP from X-Forwarded-For header. Any other faster and more reliable way is welcome.
Commits
-------
b6bf018 tweaked error handling for the forward compatibility
dd606b5 added note about the purpose of this class
c1426ba added locale handling forward compatibility
10eed30 added MessageDataCollector forward compatibility
Discussion
----------
Forward compat
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2522
Commits
-------
78e9b2f [Form] Fixed textarea_widget (W3C standards)
Discussion
----------
[Form] Fixed textarea_widget (W3C standards)
Textarea widget included the "pattern" attribute but is not valid by W3C standards.
(See PR 2666 - New PR because rebase inside the 2.0 branch)
---------------------------------------------------------------------------
by fabpot at 2011/11/18 09:01:41 -0800
@hlecorche: Thanks for your work on this issue. Can you update the unit tests to be sure that this case is covered? If you're not comfortable with this, just tell me and I will do it myself
---------------------------------------------------------------------------
by hlecorche at 2011/11/19 02:51:06 -0800
@fabpot: I did'nt commited because I am not sure. I changed the "tests/Symfony/Tests/Component/Form/AbstractLayoutTest.php" file :
public function testTextarea()
{
$form = $this->factory->createNamed('textarea', 'na&me', 'foo&bar', array(
'property_path' => 'name',
'pattern' => 'foo',
));
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/textarea
[@name="na&me"]
[not(@pattern)]
[.="foo&bar"]
'
);
}
Is it correct?
Commits
-------
36cebf0 Fix infinite loop on circullar reference in form factory
Discussion
----------
[BugFix][Form]Throw exception on form name circulal ref
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Closes: #2673
When FormType method `getName()` returns the same value as `getParent()` we're asking about trouble, and land into infinite loop.
Commits
-------
5b30812 See this issue : https://github.com/symfony/symfony/issues/2433
Discussion
----------
See this issue : https://github.com/symfony/symfony/issues/2433
I changed the access speficiers to `protected`, which makes easier to extend this class if one needs to like I did.
---------------------------------------------------------------------------
by greg0ire at 2011/11/10 06:55:12 -0800
Precision on the problem I had : I wanted to use a `CollectionType` and display a collection element attribute as the label for this element. I had no choice but to extend `ResizeFormListener` and `CollectionType`.
Commits
-------
57e1aeb Fixed undefined index notice in readProperty() method (PropertyPath)
Discussion
----------
Fixed undefined index notice in readProperty() method (PropertyPath)
Hi,
For some reasons, I get `notice` errors on `readProperty()` with Propel:
Notice: Undefined index: 0 in /Users/william/projects/Propel/testProjects/symfony2/vendor/symfony/src/Symfony/Component/Form/Util/PropertyPath.php line 284
The `PropelObjectCollection` implements `ArrayAccess`, the `readProperty()` method does not check if the given `index` exists so the `notice` error is thrown. I suppose to check whether the index exists or not has to be added.
Regards,
William
---------------------------------------------------------------------------
by fabpot at 2011/09/27 23:42:07 -0700
The patch is probably not what we want to do. First, I suppose that you are not creating the propertyPath by hand. If that is the case, we need to understand why the property path does not exist. Then, even if we might want to check the existence of the index, if it does not exist, we should probably throw an exception instead of just ignoring the problem.
---------------------------------------------------------------------------
by willdurand at 2011/09/28 01:14:49 -0700
My bad. This is a Propel bug due to `ArrayObject`. It throws a notice error if the index is not found in `offsetGet()` which is wrong according to the `ArrayAccess` interface. If the index is not found, we have to return `null`.
@fabpot Are you agree with that (for the `null` value) ?
---------------------------------------------------------------------------
by fabpot at 2011/09/28 01:17:09 -0700
My point is that it should never happen under normal circumstances.
---------------------------------------------------------------------------
by willdurand at 2011/09/28 01:23:55 -0700
@fabpot Not sure to get it.
The fact is that it tries to get the value (`getValue()`) of a fresh object, just added to the `collection` when I'm submitting a form with a `CollectionType` and a new entry in it.
I mean it tries to get this new object (not yet persisted, not yet in the collection) in the collection (`getValue()` -> `readProperty()`) which implements `ArrayAccess` but this object cannot be in the collection at this time.
Am I wrong ?
And, without this notice error thrown by Propel, I probably never opened this issue...
---------------------------------------------------------------------------
by willdurand at 2011/09/29 06:40:34 -0700
@fabpot: you can try this example: http://www.propelorm.org/cookbook/symfony2/mastering-symfony2-forms-with-propel.html#manytomany_relations in order to make your own tests. Will it be enough?
As I said, it throws a weird notice for the reasons above.
---------------------------------------------------------------------------
by jaugustin at 2011/10/04 12:58:10 -0700
any news on this ?
@fabpot did you have time to look at the test case ?
---------------------------------------------------------------------------
by cedriclombardot at 2011/11/09 14:29:42 -0800
@fabpot: can we have news about this ?
Commits
-------
fc4e628 [Config] added append to the node builder
Discussion
----------
[Config] added append to the node builder
Bug fix: no
Feature addition: yes
BC break: no
test pass: true
the problem is that i can only append a node if i have an array node, but if the current node is the node builder i can not append a node (and i think the node builder is like a array node).
---------------------------------------------------------------------------
by tecbot at 2011/11/16 23:23:16 -0800
@fabpot: any chance to merge this?
Commits
-------
fabe818 [EventDispatcher] Add reference to the EventDispatcher on the Event
Discussion
----------
[EventDispatcher] Add reference to the EventDispatcher on the Event
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
I don't like registering event listeners unless they are really used, it seems wasteful. So I tend to register listeners for the response event in other listeners, only when they will be required. @stof has [brought to my attention](fb243ace83 (commitcomment-696467)) that this may cause issues in Silex or any other situation where event listeners are not lazy loaded, since it creates a circular reference in that case.
With this PR, avoiding the circular reference is possible, without bloating the response listener with unnecessary "do I need to do anything?" code.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/06 05:28:39 -0800
Did you do any benchmarks? It's just a feeling, but registering a listener at runtime might be more expensive than just having it always executed.
Also, I find these dynamic listeners a bit of a code smell. They are not easily testable, and the control flow is harder to track. Besides, you do not take into account subrequests which might happen in between.
---------------------------------------------------------------------------
by Seldaek at 2011/11/06 09:34:27 -0800
I don't see why it would be slower, if it's a commonly fired event yes you blast away the `sorted` listener cache every time you add one, but most of the time those optional listeners are for the response, which is typically not sorted yet when you add the listener, so I don't think there is any overhead.
As for the code smell, of course it's a matter of preference, but I have the opposite view on control flow, I find it weird that listeners are registered when they are not used in the end, while doing it my way I think it's more clear what happens.
For sub-requests, I'm not sure what you mean. In this instance, and in most response listeners I have seen, the sub-requests are always ignored anyway by the listener.
---------------------------------------------------------------------------
by drak at 2011/11/10 06:07:45 -0800
I don't see how loading up the dispatcher with a bunch of callables can be expensive - it's just loading an array basically.
Wouldn't it be better to have a separate `DispatcherAwareEvent extends Event`
class DispatcherAwareEvent extends Event
{
protected $dispatcher;
public function setDispatcher(EventDispatcher $dispatcher)
{
$this->dispatcher = $dispatcher;
}
public function getDispatcher()
{
return $this->dispatcher;
}
This can then be used as a base class for what you need `MyEvent extends DispatcherAwareEvent`
$event = new MyEvent($dispatcher, $foo);
$dispatcher->dispatch($event);
---------------------------------------------------------------------------
by Seldaek at 2011/11/10 06:18:57 -0800
@drak: Every event is part the event dispatching system and therefore should be aware of the dispatcher imo. It's not like the ContainerAwareInterface which is gluing things that do not especially have to know about the DIC together.
If we do that, then we have to start arguing every time we need the dispatcher in a given event, because the original author did not think it was necessary, and then that will only make it into the next minor version, etc. Not fun at all.
---------------------------------------------------------------------------
by drak at 2011/11/10 06:36:26 -0800
By the way, the event dispatchers looks to be pretty well optimized given the fact that it only sorts listeners if they are called, and then only once.
---------------------------------------------------------------------------
by drak at 2011/11/10 12:33:28 -0800
It just seems weird. I mean, following on, why isn't the event name a compulsory parameter also? - again, you can say both ways, if you need it, make it part of your custom Event class, or since it's a required param to be able to dispatch an event in the first place, make it part of the base Event class. All I'm saying it it seems suspicious when it could be achieved a different way.
For example, you could inject the dispatcher into the listener itself and then the event handler could access the dispatcher if it needs:
class MyListener
{
public function __construct(EventDispatcher $eventDispatcher)
{
//...
}
public function someListener(Event $event)
{
//...
}
}
---------------------------------------------------------------------------
by stof at 2011/11/10 15:20:07 -0800
@drak The issue when injecting the dispatcher in the listener is described in the issue: circular dependency: you need to create the dispatcher before the listener (as it is injected in it) and when the listener is not lazy-loaded (in Silex for instance), you need to create it before the dispatcher.
---------------------------------------------------------------------------
by drak at 2011/11/10 21:15:45 -0800
Indeed, although it might not unreasonable to expect to create the dispatcher first... but anyway I'm convinced!
Injecting the dispatcher could lead to some __very interesting possibilities__ as standard. While we are at it though, we should have a getter and setter for `$name` in the `Event` class and `$event->setName($eventname)` in the `dispatch()` method. Allowing an event to know it's name is very useful. It allows a single listener to be registered for multiple names, and even makes the event object reusable. I don't know why $name was removed, it was in the Symfony 1 dispatcher and while the new dispatcher is brilliant from an OO point of view, missing the name as standard is a big shame.
+1 from me.
The Firewall is now executed after the Router. This was needed to have access
to the locale and other request attributes that are set by the Router. This
change implies that all Firewall specific URLs have proper (empty) routes like
`/login_check` and `/logout`.
Commits
-------
f9a65ba Redirect to default_target_path if use_referer is true and the referer is the login_path.
Discussion
----------
Login redirect
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Redirect to default_target_path if use_referer is true and the referer is the login_path.
---------------------------------------------------------------------------
by Seldaek at 2011/10/30 10:52:38 -0700
👍
---------------------------------------------------------------------------
by stealth35 at 2011/10/30 11:04:16 -0700
@snc BC break ?
---------------------------------------------------------------------------
by snc at 2011/10/30 12:11:39 -0700
Well I'm sure it is never intended by a developer to be redirected to the login page after logging in but it could be possible that the controller which displays the login form handles this case, so my change would break it.
Commits
-------
79ae3fc [Form] fixed radio and checkbox when data is not bool
Discussion
----------
[Form] fixed checkbox view
The checkbox view was being built based on app data, not client data. This fixes it.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by fabpot at 2011/11/16 13:31:09 -0800
`RadioType` suffers from the same problem, no?
---------------------------------------------------------------------------
by kriswallsmith at 2011/11/16 13:32:50 -0800
Yeah, I'll fix that too.
---------------------------------------------------------------------------
by kriswallsmith at 2011/11/16 13:43:29 -0800
Updated to include `RadioType`.
Commits
-------
8399574 Fixes a small php doc issue of Symfony\Component\Console\Command\Command::setDefinition()
Discussion
----------
Fixes a small php doc issue of Symfony\Component\Console\Command\Command::setDefinition()
Have setDefinition() accept InputDefinition instead of Definition.
* 2.0:
[HttpKernel] fixed Content-Length header when using ESI tags (closes#2623)
[HttpFoundation] added an exception to MimeTypeGuesser::guess() when no guesser are available (closes#2636)
[Security] fixed HttpUtils::checkRequestPath() to not catch all exceptions (closes#2637)
[DoctrineBundle] added missing default parameters, needed to setup and use DBAL without ORM
[Transation] Fix grammar.
[TwigBundle] Fix trace to not show 'in at line' when file/line are empty.
