This PR was merged into the master branch.
Commits
-------
1858b96 [Form] Adapted FormValidator to latest changes in the Validator
1f752e8 [DoctrineBridge] Adapted UniqueValidator to latest changes in the Validator
efe42cb [Validator] Refactored the GraphWalker into an implementation of the Visitor design pattern.
Discussion
----------
[Validator] Refactored the Validator for use in Drupal
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: TODO
Drupal wants to use the Symfony Validator component in their next version. I was talking to @fago recently about the changes that we'd need to make and implemented these changes in this PR. I don't want to rush this, but the deadline is tight, since Drupal feature freeze is on December 1st and @fago needs at least a couple of days to integrate the Validator into Drupal.
This PR introduces two significant changes:
* Interfaces were created for all classes that constitute the Validator's API. This is were the PR breaks BC, because `ConstraintValidatorInterface::initialize()` is now type hinted against `ExecutionContextInterface` instead of `ExecutionContext`.
* The graph walker was refactored into an implementation of the Visitor pattern. This way, the validator was decoupled from the structure of the metadata (class → properties and getter methods) and makes it possible to implement a different metadata structure, as is required by the Drupal Entity API.
As a consequence of the API change, custom validation code is now much easier to write, because `ValidatorInterface` and `ExecutionContextInterface` share the following set of methods:
```php
interface ValidatorInterface
{
public function validate($value, $groups = null, $traverse = false, $deep = false);
public function validateValue($value, $constraints, $groups = null);
public function getMetadataFor($value);
}
interface ExecutionContextInterface
{
public function validate($value, $subPath = '', $groups = null, $traverse = false, $deep = false);
public function validateValue($value, $constraints, $subPath = '', $groups = null);
public function getMetadataFor($value);
}
```
No more juggling with property paths, no more fiddling with the graph walker. Just call on the execution context what you'd call on the validator and you're done.
There are two controversial things to discuss and decide (cc @fabpot):
* I moved the `@api` tags of all implementations to the respective interfaces. Is this ok?
* I would like to deprecate `ValidatorInterface::getMetadataFactory()` (tagged as `@api`) in favor of the added `ValidatorInterface::getMetadataFor()`, which offers the exact same functionality, but with a different API and better encapsulation, which makes it easier to maintain for us. We can tag `getMetadataFor()` as `@api`, as I don't expect it to change. Can we do this or should we leave the old method in?
I would like to decide the major issues of this PR until **Sunday November 25th** in order to give @fago enough room for his implementation.
Let me hear your thoughts.
This PR was merged into the master branch.
Commits
-------
d1b5093 Try to make sure cookies get deleted from the TokenProvider when no longer in use
Discussion
----------
Delete cookies from the TokenProvider that is no longer in use
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Todo: -
License of the code: MIT
When the user logs in, or login fails for some reason, the old "remember me" cookie should be deleted from the TokenProvider if you are using the PersistentTokenBasedRememberMeServices.
As the code is now, the token is only deleted on logout.
---------------------------------------------------------------------------
by TerjeBr at 2012-11-20T13:45:54Z
So, anything else that needs to be done before this is merged?
---------------------------------------------------------------------------
by TerjeBr at 2012-11-21T10:30:53Z
Ok, I have corrected the typo in the comment and squashed the commit.
---------------------------------------------------------------------------
by schmittjoh at 2012-11-21T10:36:29Z
btw, ``canceled`` (more American) and ``cancelled`` (more British) are both
correct English forms.
On Wed, Nov 21, 2012 at 11:30 AM, Terje Bråten <notifications@github.com>wrote:
> Ok, I have corrected the typo in the comment and squashed the commit.
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/symfony/symfony/pull/6055#issuecomment-10592112>.
>
>
---------------------------------------------------------------------------
by schmittjoh at 2012-11-21T10:40:24Z
As a side-note have you verified that this does not break the cookie theft protection?
---------------------------------------------------------------------------
by TerjeBr at 2012-11-21T10:51:10Z
Yes, cookie theft protection is still there and is functioning well.
---------------------------------------------------------------------------
by TerjeBr at 2012-11-21T11:14:04Z
I am using this together with the DoctrineTokenProvider in issue #6057 in my own project and done some extensive testing on it.
---------------------------------------------------------------------------
by TerjeBr at 2012-11-23T10:30:34Z
Is this ready to be merged now?
This PR was squashed before being merged into the master branch (closes#6080).
Commits
-------
e477a2e Handle case of static controller method and controllers using magic __call() method
Discussion
----------
Handle case of static controller method and controllers using magic __call() method
Improve collecting controller details for edge cases where:
- controller is array, but contains class name and static method
- method doesn't exist, but is handled by magic __call() method
---------------------------------------------------------------------------
by fabpot at 2012-11-21T08:12:08Z
Can you add some unit tests?
---------------------------------------------------------------------------
by sli-systems at 2012-11-21T22:19:17Z
@pierredup
I disagree with the your comment about is_callable() only working with objects. The PHP docs state that the first argument is a callable, so it can be a string, array, closure, and perhaps more.
The test I added also shows that the code works as is :)
I've thought about your suggestion of adding reflection to look up the location of __call(). However, I think this doesn't really add a lot and only complicates matters. Also, as you can see in the new test, there is also __callStatic() to consider.
The fact that file/line are n/a is correct, because the most typical case will be that __call() and __callStatic() will delegate to some other method that might not even be in the same class/file (a subclass I would expect), IMHO.
@fabpot
Good catch about the '/'. I hope the test is complete enough. Looks more like an exercise on PHP callables than anything else, tho ;)
---------------------------------------------------------------------------
by pierredup at 2012-11-22T04:56:18Z
True that ````is_callable```` takes any callable argument, except in the one specific case where you have a ````__call()```` method, and pass an array with the first paramater as a string.
Take the following example:
class Controller {
public function __call($method, $arguments) {}
}
$controller = array('Controller', 'action');
var_dump(is_callable($controller));
Here ````is_callable($controller)```` will actually return ````false````, where if you have ````$controller = array(new Controller, 'action');```` it would return true.
Of course if you have a ````__callStatic```` method, then it would always return true.
Your tests doesn't seem to cover this use case
---------------------------------------------------------------------------
by sli-systems at 2012-11-22T20:27:05Z
Hmm, maybe. I have to admin that I do not know about this case. OTOH, if is_callable returns false is it really callable then? I would think this more of a PHP bug then?
I think I might have come across this case during coding, but then dismissed it because in that case FilterControllerEvent failed already before the data collector code is reached.
In FilterControllerEvent there is a check on is_callable and a LogicException is thrown if $controller is not callable.
So, is FilterControllerEvent wrong too then?
---------------------------------------------------------------------------
by pierredup at 2012-11-22T20:41:14Z
One would think that if is_callable returns false, then the controller isn't callable, but in the case I mentioned above, the controller is in fact callable. I also thought it was a bug with php, but the php-internals don't seem to think so.
The problem is, if you specify the class as a string, php looks for a static method, even if you have a __call method, it won't be registered.
I will have a look at the FilterControllerEvent to see if this use case applies there as well.
---------------------------------------------------------------------------
by sli-systems at 2012-11-22T20:50:32Z
Rather strange - if that is the case then using is_callable seems pretty pointless and the only way would be to try to execute the controller to find out if it is, in fact, callable...
---------------------------------------------------------------------------
by pierredup at 2012-11-22T20:51:07Z
Okay so it actually seems that the case above isn't callable after all. If the controller is specified as a string, then a static method need to exist. Hence why it works with __callStatic. Only when an instance of the class is specified, will it handle the __call method.
---------------------------------------------------------------------------
by sli-systems at 2012-11-22T20:57:55Z
So the tests are sufficient then?
---------------------------------------------------------------------------
by pierredup at 2012-11-22T20:59:22Z
Yes it is.
This happens when you just assume something without actually testing it :)
Sorry for the hassle
With this refactoring comes a decoupling of the validator from the structure of
the underlying metadata. This way it is possible for Drupal to use the validator
for validating their Entity API by using their own metadata layer, which is not
modeled as classes and properties/getter methods.
This PR was merged into the master branch.
Commits
-------
e2a50ef [OptionsResolver] fix normalizer without corresponding option
5a53821 [OptionsResolver] fix removing normalizers
Discussion
----------
OptionsResolver: normalizer fix
setNormalizer() -> replace() -> all() would generate an error.
---------------------------------------------------------------------------
by bschussek at 2012-07-29T16:09:20Z
Thank you for the fix! Could you please add a test case?
---------------------------------------------------------------------------
by Tobion at 2012-07-30T15:42:26Z
There is another problem: setNormalizer() (without setting an option) -> all()
I suggest to simply ignore normalizers that have no corresponding option. Do you agree?
---------------------------------------------------------------------------
by Tobion at 2012-07-30T16:19:24Z
On the other hand, one could argue that a normalizer without option should also work like this:
```
$this->options->setNormalizer('foo', function (Options $options) {
return '';
});
$this->assertEquals(array('foo' => ''), $this->options->all());
```
But when having a normalizer that wants a previous value as param, it does not work (because there is none).
---------------------------------------------------------------------------
by stof at 2012-07-30T16:30:34Z
@Tobion according to github, this need to be rebased
---------------------------------------------------------------------------
by bschussek at 2012-07-30T19:16:48Z
I guess setNormalizer() should check whether the option is set and fail otherwise. The second possibility, as you say, is to ignore them in all(). I'd prefer whatever is more efficient.
---------------------------------------------------------------------------
by bschussek at 2012-07-30T19:17:27Z
But setting a normalizer without setting an option, and having that option appear in the final options, does not make sense if you ask me.
---------------------------------------------------------------------------
by Tobion at 2012-07-30T21:23:46Z
Well it could make sense. If you want to override/normalize an option to a given value however it has been overloaded by others or just not overloaded at all. This is what normalizers do. I think its more consistent than the other solutions.
Raising exception in setNormalizer would make the Class dependent on the order you call the methods, e.g. `setNormalizer(); set()` would not work. But the other way round would be ok.
Ignoring some normalizers in `all` would be strange because they are there but not applied under some circumstances.
---------------------------------------------------------------------------
by Tobion at 2012-07-30T21:42:40Z
Added the fix. If you disagree tell me.
---------------------------------------------------------------------------
by bschussek at 2012-08-04T09:30:18Z
> Raising exception in setNormalizer would make the Class dependent on the order you call the methods, e.g. `setNormalizer(); set()` would not work. But the other way round would be ok.
I think this would be a better solution. I dislike if the normalizer magically adds an option that does not exist. This could hide implementation error, e.g. when a refactoring removes an option, but the normalizer is forgotten. Can you throw an exception in this case?
Should we find use cases that rely on this to work, we can soften the behavior and remove the exception.
---------------------------------------------------------------------------
by Tobion at 2012-08-04T15:02:51Z
Well, that would also make it impossible to set a normalizer for on optional option in OptionsResolver.
So `setOptional` + `setNormalizers` would throw an exception which sounds counter-intuitive. Are you sure about that?
---------------------------------------------------------------------------
by Tobion at 2012-08-17T11:47:58Z
ping @bschussek
---------------------------------------------------------------------------
by Tobion at 2012-10-07T22:31:44Z
@bschussek ping
---------------------------------------------------------------------------
by stof at 2012-10-13T18:04:30Z
@bschussek ping
---------------------------------------------------------------------------
by Tobion at 2012-11-08T09:55:15Z
@bschussek please let's get this finished.
This PR was merged into the master branch.
Commits
-------
b930066 Add Route::hasOption() and Route::hasRequirement() methods.
Discussion
----------
Add Route::hasOption() and Route::hasRequirement() methods.
It seems odd that there's a hasDefault() method but not for options and requirements. Drupal has logic that depends on the existence of a given requirement. So let's add that.
I think I got the coding standards right the first time for once... :-)
This PR was submitted for the 2.1 branch but it was merged into the master branch instead (closes#5885).
Commits
-------
efc22a8 [HttpKernel] Added ability to set controller result in the kernel.view event
Discussion
----------
[HttpKernel] Added ability to set controller result in the kernel.view event
Added the method `GetResponseForControllerResultEvent:setControllerResult(array $controllerResult)` to allow kernel.view events to inject data into the result.
---------------------------------------------------------------------------
by stof at 2012-11-01T13:48:51Z
new features canot be added in the 2.1 branch, which is a maintenance branch and so receives only bugfixes.
---------------------------------------------------------------------------
by fabpot at 2012-11-02T07:13:56Z
I'm not opposed to the change but can you share your use case with us?
---------------------------------------------------------------------------
by ltouroumov at 2012-11-02T07:55:55Z
I have custom annotations that allow controller methods to be invoked before the actions is run (`@BeforeFilter`) and after the action is run (`@ViewFilter`) and after the response is sent (`@ResponseFilter`).
The view filter allows to alter the controller result for example to inject parent entities into the template.
If you need more details I suppose I could post the code but I would have to ask the boss first.
---------------------------------------------------------------------------
by ltouroumov at 2012-11-02T07:57:53Z
Also I fixed the missing dollar sign in the [`6ffea1dc1cfa23844c6d90a1bfa9a282d1807a61`](6ffea1dc1c) commit. But I can't seem to attach it to the pull request.
---------------------------------------------------------------------------
by henrikbjorn at 2012-11-02T08:12:20Z
Needs tests.
---------------------------------------------------------------------------
by ltouroumov at 2012-11-02T08:38:03Z
Is it really necessary to write a test for a simple setter ? I'm not opposed to testing, just looking for a justification. And should I create a new test case or is there one ?
---------------------------------------------------------------------------
by henrikbjorn at 2012-11-02T08:42:49Z
Since your code had a clear syntax error i think that a test is in order ;)
---------------------------------------------------------------------------
by ltouroumov at 2012-11-02T08:45:03Z
Can't argue with that !
Would putting it in `Symfony\Component\HttpKernel\Tests\EventListener\ControllerResponseTest` do the trick ?
---------------------------------------------------------------------------
by henrikbjorn at 2012-11-02T09:16:55Z
Sounds fair enough.
---------------------------------------------------------------------------
by ltouroumov at 2012-11-02T09:31:09Z
This tests covers my use case(s), but there might be others I didn't think of.
---------------------------------------------------------------------------
by ludekstepan at 2012-11-20T17:15:45Z
+1
Please merge into master, I have a use case for it, now I'm relying on a Reflection to set the private property, but that's really a dirty solution. The setter would be perfect!
This PR was merged into the master branch.
Commits
-------
e39b709 [Routing] initialize the Route properties
Discussion
----------
[Routing] initialize the Route properties
They should normally be initialized anyway in the constructor. But when extending the Route (like in CMF) and using an ORM/ODM to persist them in the DB, the constructor is not called. Then a new property that is not saved like hostnamePattern stays null which in turn makes the RouteCompiler fails as it expects '' instead of null.
This PR was squashed before being merged into the master branch (closes#6063).
Commits
-------
d3d8ff4 [travis-ci] Zend Garbage Collection only for PHP5.4
Discussion
----------
[travis-ci] Zend Garbage Collection only for PHP5.4
They should normally be initialized anyway in the constructor. But when extending the Route (like in CMF) and using an ORM/ODM to persist them in the DB, the constructor is not called. Then a new property that is not saved like hostnamePattern stays null which in turn makes the RouteCompiler fails as it expects '' instead of null.
This PR was squashed before being merged into the master branch (closes#5888).
Commits
-------
2379d86 CS Fixes - Replaced "array of type" by "Type[]" in PHPDoc block
Discussion
----------
CS Fixes - Replaced "array of type" by "Type[]" in PHPDoc block
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no (but tests doesn't pass on master too). See Travis.
License of the code: MIT
Documentation PR: Not Applicable
Status: Finished
To improve support of the eclipse PDT pluggin (for autocompletion), I propose to change the array notation in PHPDoc blocks to match the phpDocumentor notation for "array of type".
Modifications are made for the following components:
- BrowserKit
- ClassLoader
- Config
- Console
- CssSelector
- DependencyInjection
- DomCrawler
- EventDispatcher (no changes)
- Filesystem (no changes)
- Finder
- Form
- HttpFoundation
- HttpKernel
- Locale
- OptionResolver (no changes)
- Process (no changes)
- Routing (no changes)
- Serializer (no changes)
- Templating
- Translation
- Validator
- Yaml (no changes)
- Security
- Stopwatch (no changes)
See Proposal https://github.com/symfony/symfony/pull/5852
---------------------------------------------------------------------------
by pborreli at 2012-11-01T15:19:27Z
will you make a PR for each component ? why not only one PR with one commit for each component instead ?
---------------------------------------------------------------------------
by raziel057 at 2012-11-01T15:32:39Z
Ok, I'm going try to do it.
---------------------------------------------------------------------------
by raziel057 at 2012-11-01T16:12:56Z
I would like to rename my branch from COMPONENT_Form to changes-phpdoc (as all modifications would be commited in only one branch), so I tried to execute the following command but I have an error.
git remote rename COMPONENT_Form changes-phpdoc
error: Could not rename config section 'remote.COMPONENT_Form' to 'remote.changes-phpdoc'
Do you know how to do it?
---------------------------------------------------------------------------
by pborreli at 2012-11-01T16:14:26Z
don't rename it, you will have to close and make another PR which is useless here, just edit the title.
---------------------------------------------------------------------------
by stof at 2012-11-01T16:16:17Z
and ``git remote rename`` is about renaming a remote repo, not a branch
---------------------------------------------------------------------------
by raziel057 at 2012-11-03T11:36:02Z
Is it normal that all my commit are duplicated? I would like just update my master and merge with my branch.
---------------------------------------------------------------------------
by fabpot at 2012-11-06T10:22:55Z
@raziel057 Can you rebase on master? That should fix your problem.
---------------------------------------------------------------------------
by fabpot at 2012-11-09T13:28:53Z
@raziel057 Can you finish this PR?
---------------------------------------------------------------------------
by Tobion at 2012-11-09T13:34:45Z
I'll do it for the routing component this evening because I know it by heart. ^^
---------------------------------------------------------------------------
by raziel057 at 2012-11-09T15:06:26Z
@Tobion ok Thanks!
@fabpot Yes, I will try to finish it this week end.
---------------------------------------------------------------------------
by raziel057 at 2012-11-11T13:04:07Z
@Tobion Did you already change PHPDoc in the Routing component?
---------------------------------------------------------------------------
by Tobion at 2012-11-11T15:21:18Z
@raziel057 Yes I'm working on it.
---------------------------------------------------------------------------
by Tobion at 2012-11-12T15:16:31Z
@raziel057 Done. See #5994
This PR was merged into the master branch.
Commits
-------
824a0f3 [Routing] compatibility with older PCRE (pre 8)
Discussion
----------
[Routing] compatibility with older PCRE (pre 8)
#6062 for master
This PR was merged into the 2.1 branch.
Commits
-------
1daefa5 [Routing] made it compatible with older PCRE version (pre 8)
Discussion
----------
[Routing] compatibility with older PCRE version (pre 8)
fixes#4093
Ok I changed my mind about this issue.
1. I figured more people are affected than I thought and CentOS is stubborn.
2. Symfony still uses the old regex style `?P<param>` in several other components. So also doing so in the routing makes it more consistent.
3. Even if it's definitely not good to use an over 6 year old PCRE version with a recent PHP version, we can still try to provide the best experience. It doesn't mean we support outdated software stacks of custom PHP compilations as we won't and cannot specifically test against it.
@fabpot: I will do a seperate PR on master when you merged this because the code changed alot in master so it cannot easily be merged I guess. I will also convert the symfony requirement for PCRE in the requirements check to a recommendation.
This PR was merged into the master branch.
Commits
-------
acbb393 Renamed variable for consistency
Discussion
----------
[SecurityBundle] Renamed variable for consistency
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -
This PR was merged into the 2.1 branch.
Commits
-------
ea2bb09 tiny refactoring for consistency
Discussion
----------
tiny refactoring for consistency
no need to use the iterator within the class. not done anywhere else.
This PR was merged into the master branch.
Commits
-------
966e7d6 [DI] removed unneeded is_object() calls
Discussion
----------
[DI] removed unneeded is_object() calls
I searched through all of symfony for occurences of the coding style `(is_object($value) && $value instanceof Object)` with a regex like `is_object\(\$[a-zA-z0-9]+\) && \$[a-zA-z0-9]+ instanceof `.
The `is_object` calls are not needed in this case. Only the DI component made such duplicate checks.
This PR was merged into the master branch.
Commits
-------
acf8a70 [Routing] fix Route recompilation when hostname changed
Discussion
----------
[Routing] fix Route recompilation when hostname changed
This PR was squashed before being merged into the master branch (closes#6030).
Commits
-------
749dac1 Improve docBlock
Discussion
----------
Improve docBlock
This is just a minor change documenting the return type of `SerializerInterface::deserialize()`.
This PR was merged into the master branch.
Commits
-------
644de74 Fix docblock in Doctrine Bridge
Discussion
----------
Fix docblocks in Doctrine Bridge
This PR was merged into the master branch.
Commits
-------
a146156 Merge pull request #2 from Tobion/patch-2
38802ea remove logic that could not be triggered anyway
f7ea68f [Routing] Fixed undefined variable + typo
Discussion
----------
[Routing] Fixed typo + removed dead code
---------------------------------------------------------------------------
by Tobion at 2012-11-17T16:00:04Z
@pborreli: pborreli/symfony#2
---------------------------------------------------------------------------
by pborreli at 2012-11-17T16:02:08Z
@Tobion totally agree, tried to setup a phpunit test which could trigger this exception but couldn't ..
