Commits
-------
b7b26af [DependencyInjection] Added, implemented and tested IntrospectableContainerInterface::initialized()
Discussion
----------
[DependencyInjection] IntrospectableContainerInterface::initialized()
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Added, implemented and tested `IntrospectableContainerInterface::initialized()`, which allows checking for whether or not a service id has actually been loaded, without forcing it to load. This could be implemented in several places to prevent loading a service when it's only needed if it has been previously loaded - for example the SwiftmailBundle's event listener for the `kernel.terminate` event, which currently forces Swiftmail to load on every request to check for messages to send, even if it was not previously loaded.
---------------------------------------------------------------------------
by fabpot at 2012-03-11T09:10:32Z
Do you have any other examples in mind where it would be useful?
---------------------------------------------------------------------------
by stof at 2012-03-11T12:39:22Z
@fabpot 2 exemples:
- the Swiftmailer listener to avoid loading the initialization code of Swiftmailer in each kernel.terminate event just to check that there is no pending message in the spool (if the spool has never been retrieved, it cannot have messages) (this is the use case we discussed on IRC last night)
- closing Doctrine connections and Monolog handlers in the shutdown method without creating them if they were not used
---------------------------------------------------------------------------
by stof at 2012-03-11T12:39:53Z
However, I don't like the name of the method
---------------------------------------------------------------------------
by lsmith77 at 2012-03-11T13:26:34Z
sounds very useful
---------------------------------------------------------------------------
by evillemez at 2012-03-11T17:00:39Z
@fabpot another example:
* Forcing a session to write early, if it was previously loaded - but not having to load the session to check, thus potentially forcing a database connection (if that's how the session is being handled) when it's not needed.
@stof Would more or less verbose be better? Like, `isServiceLoaded()` or just `loaded()`.... or a better word than "loaded" ? :)
---------------------------------------------------------------------------
by stof at 2012-03-11T17:03:11Z
@evillemez My issue is the word ``loaded``. I don't think it is clear about what it means here
---------------------------------------------------------------------------
by lsmith77 at 2012-03-11T17:04:24Z
one thing we should also keep in mind here is the scope of the service.
BTW: there are also a couple of CS violations in your PR
---------------------------------------------------------------------------
by fabpot at 2012-03-11T17:07:43Z
@stof: I agree that we should think of a better name: `initialized()` or `exists()` (to differentiate from `has()`)?
@evillemez: After choosing the name, can you work on actually using the new method for some of the use cases mentioned in this PR?
---------------------------------------------------------------------------
by lsmith77 at 2012-03-11T17:20:12Z
what i meant with "scope" was if we are only talking about services instantiated in the current scope? but i guess there is no way to handle anything else anyway.
as for name .. i think ``instantiated()`` is more fitting than ``initialized()``.
``exists()`` could be confused with ``has()`` imho
---------------------------------------------------------------------------
by stof at 2012-03-11T17:26:06Z
The current implementation only works for container-scoped services. It does not make sense for prototyped-scope services anyway (as the container does not keep them) but supporting scoped services will be tricky IMO
---------------------------------------------------------------------------
by mvrhov at 2012-03-11T17:34:21Z
hasBeen(Used|Called|Initialized),
---------------------------------------------------------------------------
by evillemez at 2012-03-11T17:54:43Z
The next day or two I'm only around for an hour or so here and there, I may be a little slow to respond.
@stof @lsmith77 I agree with either `instantiated()` or `initialized()`, I also think `exists()` might be easily confused with `has()`
@lsmith77 Besides the opening/closing brace placements, was there anything else?
@fabpot I would happily implement it in the Swiftmail bundle - as of now that's the only area where I know absolutely it would be useful. The Session example I mentioned was an issue I ran into working on another app in another framework, and I'm not familiar yet with the internals of the Monolog/Doctrine Bundles. But if people could point me in the right direction I'd be willing to check it out.
I'm still relatively new to some of the Symfony2 internals, so if there are obvious things I'm missing, please don't hesitate to point them out. :)
---------------------------------------------------------------------------
by evillemez at 2012-03-11T18:00:29Z
@lsmith77 Oh... I think there were some tab issues I didn't see in my editor, I'll fix those too.
---------------------------------------------------------------------------
by stof at 2012-03-11T18:13:03Z
The places where it should be used for Doctrine and Monolog are in separate repos anyway so it cannot be part of this PR
---------------------------------------------------------------------------
by evillemez at 2012-03-12T03:38:50Z
Any thoughts on `instantiated` vs `initialized`? I'm leaning towards `initialized`.
How should I proceed, close this request and submit another with the changed name and fixed CS violations?
---------------------------------------------------------------------------
by fabpot at 2012-03-12T07:41:11Z
`initialized()` looks fine to me. Make your changes, squash your commits and then force the push to your branch (the PR will be updated automatically).
---------------------------------------------------------------------------
by evillemez at 2012-03-12T20:49:17Z
I was about to squash my commits to update this, but it just occurred to me that I hadn't considered the interface. Does anyone feel this method `initialized()` should be defined in ContainerInterface as well? It seems like it's generic enough that it would make sense. But I'm not immediately aware if this would cause BC breaks.
---------------------------------------------------------------------------
by fabpot at 2012-03-13T11:34:33Z
We cannot break BC for `ContainerInterface` as this is marked with the `@api` tag.
---------------------------------------------------------------------------
by henrikbjorn at 2012-03-13T12:34:42Z
Is it a BC break if we add a method? i thought only changing the already written methods would be a BC break.
---------------------------------------------------------------------------
by ooflorent at 2012-03-13T12:39:44Z
@henrikbjorn It will raise a fatal error if the method isn't implemented in existing class.
---------------------------------------------------------------------------
by lsmith77 at 2012-03-13T13:06:26Z
we could however add a new interface that extends from the previous one.
---------------------------------------------------------------------------
by evillemez at 2012-03-13T15:40:39Z
Are the BC breaks we are worried about for compatibility just within the Symfony repo - or in general in case others have implemented the interface elsewhere? As far as Symfony is concerned, the only class I can find that implements `ContainerInterface` is `Container`, so adding the method shouldn't be an issue.
If it's an issue of principle, in case others may have implemented the `ContainerInterface`, then... yeah, it's a break, and maybe should be left out. I suppose this would mean that other places in code that type hint for `ContainerInterface` would have to change the type hint to `Container` if they want to implement the `initialized()` method.
Is this more or less acceptable than updating the interface?
---------------------------------------------------------------------------
by evillemez at 2012-03-14T19:17:27Z
Hadn't properly squashed commits, just fixed.
---------------------------------------------------------------------------
by evillemez at 2012-03-15T14:06:38Z
I'm done with this PR, unless there is a consensus that I should also implement `Container::initialized()` in `ContainerInterface`.
Anything else I need to do?
---------------------------------------------------------------------------
by Seldaek at 2012-03-15T15:41:44Z
@evillemez the common pattern for BC is to add a new interface, say IntrospectableContainerInterface or something, that extends ContainerInterface, then you can type-hint that without restricting use of competing implementations of the Container class.
---------------------------------------------------------------------------
by stof at 2012-04-03T22:30:51Z
@evillemez Please update this PR. It conflicts with master because of the move of tests. And the new interface suggested by @Seldaek is a good idea IMO
---------------------------------------------------------------------------
by evillemez at 2012-04-04T14:57:29Z
@Stof I may not be able to get to this until the end of the week, but I'll do it as soon as I can.
I'll rebase against the current master, and add/implement the interface. Is `IntrospectableContainerInterface ` as suggested by @Seldaek ok with everyone?
Are there other features that we can think of that would be useful for this new interface? I don't want to start a precedent of adding a new interface for every new method that comes up... I understand it makes sense for not breaking backwards compatibility for something previously marked as stable, but still, it's yet another file that will likely be included on every request.
---------------------------------------------------------------------------
by stof at 2012-04-04T15:35:15Z
@evillemez classes used on every requests can be added to some cached bootstrap files (which are loaded during the ``$kernel->loadClassCache()`` call in your front controller) to avoid including many files through the autoloader. And for even better performances in prod, the solution is to use APC with ``apc.stat = 0``, as advocated by @lsmith77
---------------------------------------------------------------------------
by evillemez at 2012-04-15T19:00:07Z
Ok, rebased against current master and implemented the interface. I didn't mark it as `@api` because I think we may want to consider if there's any other functionality that could be implemented there before declaring it stable.
Sorry for the delay. My wife and I are in the process of buying a house, and some unexpected things have come up.
---------------------------------------------------------------------------
by fabpot at 2012-04-18T10:59:01Z
Can you rebase before I merge? Thanks.
Commits
-------
6e4ed9e [Form] Fixed regression: bind(null) was not converted to an empty string anymore
fcb2227 [Form] Deprecated FieldType, which has been merged into FormType
bfa7ef2 [Form] Removed obsolete exceptions
2a49449 [Form] Simplified CSRF mechanism and removed "csrf" type
Discussion
----------
[Form] Merged FieldType into FormType
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #3878
Todo: update the documentation on theming
This PR is a preparatory PR for #3879. See also #3878.
---------------------------------------------------------------------------
by juliendidier at 2012-04-13T14:25:19Z
What's the benefit ?
---------------------------------------------------------------------------
by henrikbjorn at 2012-04-13T14:26:40Z
why `input_widget` ? and not just `widget`
---------------------------------------------------------------------------
by Burgov at 2012-04-13T14:27:49Z
@juliendidier dynamic inheritance is now obsolete which fixes some other issues
---------------------------------------------------------------------------
by stloyd at 2012-04-13T14:37:26Z
What about __not__ breaking API so *badly* and leaving `FieldType` which will be simple like (with marking as deprecated):
```php
<?php
class FieldType extends AbstractType
{
public function getParent(array $options)
{
return 'form';
}
public function getName()
{
return 'field';
}
}
---------------------------------------------------------------------------
by bschussek at 2012-04-13T14:43:41Z
@stloyd That's a very good idea.
---------------------------------------------------------------------------
by mvrhov at 2012-04-13T17:41:21Z
IMHO what @stloyd proposed sounds like a good idea, but removing FieldType class, if #3903 will come into life might ensure that more forms will broke and people will check them thoroughly.
---------------------------------------------------------------------------
by r1pp3rj4ck at 2012-04-13T18:46:08Z
@bschussek looks great, but I'm concerned about how quickly will the third-party bundles adapt to this BC break. I hope really quick, because if they don't the whole stuff will be useless :S of course it's not your problem to solve.
---------------------------------------------------------------------------
by stof at 2012-04-13T18:50:32Z
@r1pp3rj4ck there is already another BC break requiring to update custom types for Symfony master. So third party bundles already have to do some work.
---------------------------------------------------------------------------
by r1pp3rj4ck at 2012-04-13T18:59:37Z
@stof which one? I've looked into @bschussek 's RFC about these [foo].bar stuff, but it's not yet implemented. Are you refering to this or another one I've missed?
---------------------------------------------------------------------------
by stof at 2012-04-13T19:04:06Z
@r1pp3rj4ck the change regarding default options
---------------------------------------------------------------------------
by r1pp3rj4ck at 2012-04-13T19:06:10Z
@stof oh, I forgot that one. Weird thing is that I've already changed my default options today and still forgetting these stuff :D
---------------------------------------------------------------------------
by bschussek at 2012-04-14T08:58:29Z
I restored and deprecated FieldType now. I'd appreciate further reviews.
---------------------------------------------------------------------------
by stloyd at 2012-04-14T09:02:32Z
Maybe we should try to avoid this BC in templates ? What do you think about similar move like with `FieldType` ? (hold old, but inside just render new)
---------------------------------------------------------------------------
by bschussek at 2012-04-14T09:07:22Z
@stloyd You mean for those cases where people explicitely render the block "field_*"? We can do that.
---------------------------------------------------------------------------
by stloyd at 2012-04-14T09:09:45Z
@bschussek Yes I mean this case =) Sorry for not being explicit, I need some coffee I think =)
---------------------------------------------------------------------------
by bschussek at 2012-04-17T14:45:35Z
I added the field_* blocks again for BC. Could someone please review again? Otherwise this can be merged.
---------------------------------------------------------------------------
by Burgov at 2012-04-17T15:11:16Z
@bschussek I'm not sure what has changed to cause this, but if I try out your branch on our forms, if I leave the value of an input empty, eventually the reverseTransform method receives a null value, rather than a '' (empty string) value, as on the current symfony master.
DateTimeToLocalizedStringTransformer, for example, will throw an Exception if the value is not a string
```php
if (!is_string($value)) {
throw new UnexpectedTypeException($value, 'string');
}
```
Other than that, all forms render just the same as they do on symfony master
---------------------------------------------------------------------------
by bschussek at 2012-04-17T15:30:29Z
@Burgov Fixed.
Commits
-------
24bd8f4 Added missing dot to translation messages.
4bff221 Added missing dot to translation messages.
7454894 Added missing dot to translation messages.
6e90c50 Updated upgrade instructions.
7e21dd1 Added missing dot to translation messages.
Discussion
----------
Issue 3379
This should fix [issues 3379](https://github.com/symfony/symfony/issues/3379)
---------------------------------------------------------------------------
by stof at 2012-04-13T15:06:32Z
Your branch conflicts with master. Please rebase it
---------------------------------------------------------------------------
by umpirsky at 2012-04-13T19:11:54Z
@stof I tried to rebase, I'm not sure if I did everything right. Is it ok now?
---------------------------------------------------------------------------
by umpirsky at 2012-04-13T19:12:06Z
@stof I tried to rebase, I'm not sure if I did everything right. Is it ok now?
---------------------------------------------------------------------------
by mvrhov at 2012-04-13T19:19:34Z
IMHO no, because there are commits from other people. Did you follow the [instructions](http://symfony.com/doc/current/contributing/code/patches.html#id1)?
---------------------------------------------------------------------------
by stof at 2012-04-13T19:36:53Z
@mvrhov commits from others ?
---------------------------------------------------------------------------
by umpirsky at 2012-04-13T19:41:53Z
@stof There were some, so I reverted. Now I'm trying again following instructions from Symfony doc.
I come to this:
```
$ git push origin issue-3379
To git@github.com:umpirsky/symfony.git
! [rejected] issue-3379 -> issue-3379 (non-fast-forward)
error: failed to push some refs to 'git@github.com:umpirsky/symfony.git'
To prevent you from losing history, non-fast-forward updates were rejected
Merge the remote changes (e.g. 'git pull') before pushing again. See the
'Note about fast-forwards' section of 'git push --help' for details.
```
And I don't know how to fix this. Any idea?
---------------------------------------------------------------------------
by stof at 2012-04-13T19:43:45Z
@umpirsky when you rebase, it is logical to need to force the push
---------------------------------------------------------------------------
by umpirsky at 2012-04-13T19:44:38Z
@stof I did `git push -f origin issue-3379`. I hope it's fixed now.
---------------------------------------------------------------------------
by maoueh at 2012-04-13T20:39:34Z
@umpirsky seems better than last time I checked :)
---------------------------------------------------------------------------
by umpirsky at 2012-04-13T20:43:04Z
@maoueh Is it good enough? :)
---------------------------------------------------------------------------
by maoueh at 2012-04-13T20:51:27Z
@umpirsky At least, the rebase seems good enough :D As for the subject of the PR, I don't pronounce myself ;)
---------------------------------------------------------------------------
by vicb at 2012-04-13T20:53:23Z
you should probably squash the commits
Commits
-------
aa055df [Composer] Stwitch to composer vendors management
Discussion
----------
[Composer] Stwitch to composer vendors management
Bug fix: no
Feature addition: yes
Backwards compatibility break: No?
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
[](http://travis-ci.org/canni/symfony)
This speeds up Travis CI builds to `~2 min` also makes vendor management
a lot easier.
---------------------------------------------------------------------------
by fabpot at 2012-02-09T06:24:24Z
I'm -1 on this change. The `vendors.php` script is *only* for people working on the core so that we can run the unit tests. So, we need the flexibility to test on many different versions of the code and having the repository here is kind of mandatory.
---------------------------------------------------------------------------
by Seldaek at 2012-02-09T08:15:28Z
You can `composer install --dev` to get proper clones. I'm not really pro or against, just saying it's an option.
---------------------------------------------------------------------------
by canni at 2012-02-09T08:28:54Z
@fabpot I understand yours point, but from my view transferring the whole git structure of *vendors* is little pointless IMO (especially in Travis env)
but I think I can make this change optional, so Travis and anyone that prefer to, can use `composer` an with old functionality available.
(There will be almost no duplication, as anyway we're updating `composer.json`)
---------------------------------------------------------------------------
by canni at 2012-02-09T09:20:17Z
@fabpot I've enabled both behaviors, everything will work regardless of using `composer` or `vendors.php` this lets the developer decide what to use
---------------------------------------------------------------------------
by drak at 2012-02-16T12:05:28Z
Since there is a `--dev` option in Composer then I think this is a good idea. You could also add composer.phar to the repo bin directory.
---------------------------------------------------------------------------
by henrikbjorn at 2012-02-16T12:06:55Z
`--dev` have been renamed to `--prefer-source`
---------------------------------------------------------------------------
by canni at 2012-02-16T12:22:01Z
@fabpot any chance to consider this merge? If not, this PR can be closed.
---------------------------------------------------------------------------
by henrikbjorn at 2012-02-16T12:25:51Z
@canni This is the goal eventually. But i think we need composer to be a bit more stable in its solver.
---------------------------------------------------------------------------
by francoispluchino at 2012-02-16T12:39:24Z
👍
---------------------------------------------------------------------------
by jmikola at 2012-04-06T18:19:27Z
@fabpot: Is this PR still off the table, or are you reconsidering it with the `--prefer-source` option? I was just running symfony unit tests, and attempted to install deps with composer as I thought this PR or another like it had recently been merged to core. It wasn't :)
Admittedly, it's a downside that vendor libs, even if git repositories, will be nestled within the `.composer/` directory.
---------------------------------------------------------------------------
by drak at 2012-04-07T00:20:33Z
@canni This PR needs to be rebased and reviewed because of the changed tests directory (there is no longer a central `tests/` folder).
---------------------------------------------------------------------------
by canni at 2012-04-07T06:34:28Z
Hey,
will do after a weekend.
canni
Użytkownik Drak <reply@reply.github.com> napisał:
>@canni This PR needs to be rebased and reviewed because of the changed tests directory (there is no longer a central `tests/` folder).
>
>---
>Reply to this email directly or view it on GitHub:
>https://github.com/symfony/symfony/pull/3291#issuecomment-5004750
---------------------------------------------------------------------------
by canni at 2012-04-08T19:02:03Z
@drak done.
Commits
-------
98a0052 improved readability
b06537e refactored code to use get() when outputting a single route
Discussion
----------
Router debug refactoring
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/lsmith77/symfony)
Fixes the following tickets: -
refactored code to use get() when outputting a single route
this is useful for a CMS, where in most cases there will be too many routes to make it feasible to load all of them. here a router implementation will be used that will return an empty collection for ->all(). with this refactoring the given routes will not be listed via router:debug, but would still be shown when using router:debug [name]
Commits
-------
5208bbe [Validator] Fixed typo, updated CHANGELOG and UPGRADE
dc059ab [Validator] Added default validate() implementation to ConstraintValidator for BC
6336d93 [Validator] Renamed ConstraintValidatorInterface::isValid() to validate() because of the lack of a return value
46f0393 [Validator] Removed return value from ConstraintValidatorInterface::isValid()
Discussion
----------
[Validator] Renamed ConstraintValidatorInterface::isValid() to validate() and removed return value
Bug fix: no
Feature addition: no
Backwards compatibility break: **YES**
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: update the documentation
Before I begin, this PR is up for discussion.
I removed the return value of ConstraintValidator::isValid() because it wasn't used anymore within the framework. Removing it also means a simplification for userland implementations. Already now the validation component only depended on violation errors being present for deciding whether the validation was considered failed or not.
Because the name `isValid` does not make a lot of sense without a return value, I changed it to `validate`. Note that this affects an interface (ConstraintValidatorInterface) previously marked with `@api` by us!
What do you think about this change?
---------------------------------------------------------------------------
by stof at 2012-02-09T17:51:38Z
@bschussek IIRC, the Validator component was part of the components that are not considered as stable for 2.0 (there is 4 of them IIRC, including Config, Security and Form) so changing the interface should not be an issue here
---------------------------------------------------------------------------
by lsmith77 at 2012-02-09T17:54:55Z
No it was .. form wasn't:
http://symfony.com/doc/2.0/book/stable_api.html
---------------------------------------------------------------------------
by rdohms at 2012-02-10T13:23:32Z
I fail to see the value of the BC in this case.
Just because the framework does not use given functionality anymore is not reason to drop it, since all of this was clearly proposed as a "component" to be used in other projects. Other implementations of validator in other projects might actually depend on this.
Is it possible to just add a new value and have both functionalities available?
---------------------------------------------------------------------------
by stof at 2012-02-10T13:25:12Z
@rdohms the point is that the return value is confusing. Someone may return ``false`` by thinking it will mark the field as invalid (which is implied by the name ``isValid``) whereas it is not the case at all
---------------------------------------------------------------------------
by bschussek at 2012-02-10T13:30:13Z
Exactly. UniqueEntityValidator for example always returned `true` and nobody ever noticed.
---------------------------------------------------------------------------
by beberlei at 2012-02-10T13:53:03Z
@bschussek but its not a bug, setting the execution context failure is enough. returning false would lead to a second error message being evicted. This is the weird problem of the API imho
---------------------------------------------------------------------------
by bschussek at 2012-02-10T13:54:49Z
@beberlei This has already been fixed.
---------------------------------------------------------------------------
by stof at 2012-02-10T13:59:59Z
@beberlei in the master branch, errors are not duplicated anymore as the return value is simply ignored.
---------------------------------------------------------------------------
by Tobion at 2012-02-10T14:29:03Z
I'm +1. If people are concerned about this strong BC break we could maybe add a fallback for the majority.
Most people propably have extended the ConstraintValidator and not used the interface directly. So we can change the Interface and at the same time provide a default proxy method in ConstraintValidator for validate. I.e.
public function validate($value, Constraint $constraint)
{
$this->isValid($value, $constraint);
}
Thus all people who have extended ConstraintValidator won't notice a BC break.
---------------------------------------------------------------------------
by hades200082 at 2012-02-10T16:10:31Z
Could you not have both validate and isValid as separate methods with distinct purposes?
---------------------------------------------------------------------------
by stof at 2012-02-10T16:55:12Z
@hades200082 which distinct purposes ?
---------------------------------------------------------------------------
by hades200082 at 2012-02-10T17:02:57Z
One should actually validate. The other should return whether it is valid or not as a bool.
Even if isValid calls validate to determine this surely they are distinct purposes? doing it this way would not require a break to BC but the existing components in the framework could be switched to use validate.
---------------------------------------------------------------------------
by bschussek at 2012-02-10T17:10:50Z
@hades200082 Validators are stateless. They don't "remember" whether they validated successfully or not.
---------------------------------------------------------------------------
by hades200082 at 2012-02-10T17:13:25Z
Maybe they should? Would save revalidating every time
---------------------------------------------------------------------------
by stof at 2012-02-10T17:16:10Z
@hades200082 how could they be stateless ? you can use the same instance to validate several values. For instance, the UniqueEntityValidator is a service and so will be reused.
---------------------------------------------------------------------------
by fabpot at 2012-02-11T23:40:09Z
I would really like that we do not break BC in this case.
---------------------------------------------------------------------------
by stof at 2012-02-11T23:59:02Z
@fabpot there is also a BC break in the previous changes: the return value has no meaning at all now (it is not even considered by the GraphWalker.
Most 2.0 validator will continue working because of the new implementation of setMessage but I can provide the 2 broken cases:
```php
<?php
/**
* This validator always set the message, even when it is valid to keep things simple.
* This works fine in 2.0.x (as the return value is what makes the decision) but will
* add a violation in 2.1 (setMessage adds the violation to keep things working for
* cases setting the message only for invalid values, like the core used to do).
*/
public function isValid($value, Constraint $constraint)
{
$this->setMessage($constraint->message);
return true;
}
/**
* This validator never set the message, failing with an empty message.
* This works fine in 2.0.x (as the return value is what makes the decision) but will
* not add the violation in 2.1.
*/
public function isValid($value, Constraint $constraint)
{
return false;
}
```
The second one is clearly an edge case as it would absolutely not be user-friendly but the first one makes totally sense when using the 2.0.x API (with a boolean expression using the value of course)
---------------------------------------------------------------------------
by fabpot at 2012-02-12T00:11:19Z
I agree with you; I should probably have refused to merge the previous PR. And I think we need to reconsider this change. If not, why are we even bothering tagging stuff with the @api tag?
---------------------------------------------------------------------------
by bschussek at 2012-02-12T10:15:55Z
@stof I disagree with you. Setting an error message but not letting the validation fail is not how the API is supposed to work. Also the opposite was not meant to work, as it results in empty error messages. The third example is that a validator *had* to return true if it called `addViolation` directly. These cases show that the previous implementation was clearly buggy and needed to be fixed.
This PR is only a consequence that cleans the API up.
@fabpot IMHO validator was too young and not tried enough to be marked as stable. But as we can't change this anymore, I think the decision we have to make is
* BC/reliance on `@api` marks vs.
* API usability (also considering the coming LTR)
---------------------------------------------------------------------------
by bschussek at 2012-02-12T10:18:12Z
BTW @Tobion's suggestion could definitely make a transition easier.
---------------------------------------------------------------------------
by fabpot at 2012-02-15T10:26:10Z
@bschussek +1 for @Tobion's suggestion.
---------------------------------------------------------------------------
by Brouznouf at 2012-02-15T16:06:12Z
Could be nice to comment function as deprecated and/or trigger a E_USER_DEPRECATED error when using this method to prevent user calling this method.
---------------------------------------------------------------------------
by stof at 2012-02-15T16:09:37Z
trigger E_USER_DEPRECATED would be wrong as the kernel set the error reporting to ``-1`` and registers an error handler tuning all reported errors to exception in debug mode, so it would be a BC break.
Commenting the function as deprecated in indeed possible
---------------------------------------------------------------------------
by rdohms at 2012-02-29T11:15:01Z
Went back to working on validators and it really makes me disagree with these changes a little more. Let me explain.
In the isValid method, i like to work with return early checks, so straight up i check some stuff and return early either true/false.
From the frameworks perspective true/false does not make a difference, but from a reader's perspective it adds a lot of value:
if ($object->getId() === null) {
return true;
}
versus
if ($object->getId() === null) {
return;
}
having the return true make it clear that in this case the object is valid for anyone who is reading my validator. i think this is a good practice to push forward.
Anyway, my 2 cents on it.
---------------------------------------------------------------------------
by stof at 2012-04-04T00:05:09Z
@fabpot what do you think about this ?
---------------------------------------------------------------------------
by bschussek at 2012-04-05T16:37:38Z
@rdohms: Still, how do you want to deal with the fact that the return value is ignored anyway?
---------------------------------------------------------------------------
by rdohms at 2012-04-06T06:51:07Z
@bschussek Nobody has to know? I would keep it as it is, i have noticed that returning false without any error messages does not get me the expected results, so it seems there is no harm in keeping the parctice of true/false even if it is misleading.
Other then that.. i would alter the code to self create a error message if false is returned, thus making true/false still work, but i'm guessing that's not what your vision says, even if i find it les readable and understandable. So yeah, just my opinioin.
---------------------------------------------------------------------------
by bschussek at 2012-04-06T07:02:53Z
@rdohms: Your opinion is appreciated. Self-creation of error messages is what we did before, unfortunately it's very hacky then to suppress the self-creation if you want to return false and add (potentially more than one) error messages yourself.
---------------------------------------------------------------------------
by bschussek at 2012-04-17T14:58:07Z
I added @Tobion's suggestion now. Can you please review again? Otherwise this is ready for merge.
---------------------------------------------------------------------------
by Tobion at 2012-04-17T15:05:16Z
Statement in changelog and upgrade is missing, or?
Commits
-------
b7c2d3d [Propel1] Added tests for guessType() method
897a389 [Propel1] Added tests for the PropelTypeGuesser
cffcdc9 Improve the TypeGuesser to match the latest Sf2.1
Discussion
----------
[Form] Propel type guesser
Thanks to @vicb, the PropelTypeGuesser has been updated. I've added unit tests to prove his improvement, and everything is ok from my point of view.
---------------------------------------------------------------------------
by willdurand at 2012-04-15T16:38:09Z
Well, I made the changes, but I really don't care about fixing these comments.
To write `assertNull` doesn't improve readabilty, as I expect to read the returned value in the test. And, it's better to read `null` than to read the assertion method. Moreover, that makes the test suite inconsistent, as you are not able to read each tests the same way :)
---------------------------------------------------------------------------
by vicb at 2012-04-15T17:20:01Z
Great ! thanks @willdurand
CSRF fields are now only added when the view is built. For this reason we already know if
the form is the root form and avoid to create unnecessary CSRF fields for nested fields.
this is useful for a CMS, where in most cases there will be too many routes to make it feasible to load all of them. here a router implementation will be used that will return an empty collection for ->all(). with this refactoring the given routes will not be listed via router:debug, but would still be shown when using router:debug [name]
Commits
-------
b1ea552 [Locale] micro-optimization
663d218 [Locale] changed method name
bb61e09 [Locale] use the correct way for Intl error
Discussion
----------
[2.0][Locale] rebased PR 3765 plus few changes
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
#3765 was right but was made in master. Cherry-picked and rebased for 2.0.
Tests are passing.
Commits
-------
05b2238 [DependencyInjection] Fix for issue introduced in 3ae826a
Discussion
----------
[2.0][DependencyInjection] Fix for issue introduced in 3ae826a
Bug fix: yes
Tests pass: 
Fix for issue introduced in 3ae826a
---------------------------------------------------------------------------
by eriksencosta at 2012-04-14T21:08:40Z
@fabpot The 2.0 test suite is broken without this fix.
Commits
-------
80f96b7 [DependencyInjection] Add ability to clear tags by name.
Discussion
----------
[DependencyInjection] Add ability to clear tags by name.
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by jalliot at 2012-04-14T07:50:51Z
@drak Just for information, what is the use case?
---------------------------------------------------------------------------
by drak at 2012-04-14T08:40:13Z
I'm using this to filter (and prevent) some listeners from being loaded
into the dispatcher.
---------------------------------------------------------------------------
by lsmith77 at 2012-04-14T09:37:39Z
tags are used by compiler passes. bundles that set these tags expect some defined bundle to take some specific actions on the given services. as such this is already a fairly "fragile" relationship. once other compiler passes then start messing with these tags it can get even more tricky. then again, as long as you know what you are doing ..
that being said .. this method just adds convenience, since one could always just get all the tags, unset and reset them in bulk.
---------------------------------------------------------------------------
by drak at 2012-04-14T09:42:04Z
@lsmith77 - exactly.
Commits
-------
01fcb08 [HttpKernel] Fix the ProfilerListener (fix#3620)
Discussion
----------
[HttpKernel] Fix the ProfilerListener (fix#3620)
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/vicb/symfony)
Fixes the following tickets: #3620, PR #3618
Many thanks to @guilhermeblanco for helping with the tests.
---------------------------------------------------------------------------
by vicb at 2012-04-13T20:10:15Z
For ref: that's basically a backport of #3920 for 2.0
Commits
-------
41bdf26 [DoctrineBridge] Initialize proxies in UniqueEntityValidator
Discussion
----------
[DoctrineBridge] Initialize proxies in UniqueEntityValidator
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/jmikola/symfony)
Fixes the following tickets: #3163
---------------------------------------------------------------------------
by stof at 2012-04-13T18:17:57Z
I think you should use ``$em->initializeObject($value)`` instead (which is a no-op for other objects)
---------------------------------------------------------------------------
by jmikola at 2012-04-13T18:24:22Z
@stof: Thanks for the suggestion. I wasn't aware of that method.
Commits
-------
b611db8 [Profiler] Sub requests are not Main requests
2551270 [Profiler] Minimize the number of Profile writes
Discussion
----------
[HttpKernel] Profiler Listener tweaks
* `setParent()` is called in [`Profile::addChild()`](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Profiler/Profile.php#L180) in 2.1
* The profiles are now only saved once only in the listener (either at the end of the main request or on an exception)
* The profiles are now only saved once only in the TraceableEventDispatcher (twice for the root profile when there is a kernel.terminate' event
[](http://travis-ci.org/vicb/symfony)
---------------------------------------------------------------------------
by vicb at 2012-04-13T11:15:25Z
Not so sure for the save part... I'll double check
Commits
-------
c0e7ee9 [FrameworkBundle] Make session save path configurable
Discussion
----------
[FrameworkBundle] Make session save path configurable
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/asm89/symfony)
Makes it possible to configure the session save path. It still defaults to saving sessions in the kernel cache dir. This might not be appropriate if you want to keep your user sessions after deploying your application.
As promised to @fabpot I will also do a PR on the docs explaining why this might be useful.
---------------------------------------------------------------------------
by drak at 2012-04-13T10:16:17Z
It might be good to default this value to `%kernel.cache_dir%/sessions`.
---------------------------------------------------------------------------
by stloyd at 2012-04-13T10:30:58Z
@drak https://github.com/symfony/symfony/pull/3912/files#L0R184
---------------------------------------------------------------------------
by drak at 2012-04-13T10:31:57Z
@stloyd need my eyes checked :-P
Commits
-------
c331f4a HttpKernel test fix on windows
Discussion
----------
HttpKernel test fix on windows
The changes in `StopwatchEventTest` are only for consistency with the other tests in this file.
---------------------------------------------------------------------------
by drak at 2012-04-13T04:16:19Z
@fabpot - This seems to be an eternal problem with the these particular tests. I wonder if there is a better way to do this. How about a simple greater than condition to show time has elapsed?
---------------------------------------------------------------------------
by Tobion at 2012-04-13T04:33:04Z
The tests are fine. I didn't change them. Just made it more consistent.
---------------------------------------------------------------------------
by drak at 2012-04-13T04:49:20Z
Yes, but if you look at the history of these tests files, they are constantly being tweaked for whatever reason (and they often fail on windows builds randomly). This is a clear indication the tests are not robust and a different approach is probably warranted if it can be found.
---------------------------------------------------------------------------
by drak at 2012-04-13T04:52:53Z
@Tobion - regarding the commit message, what does "fix" refer to if the tests are fine?
---------------------------------------------------------------------------
by Tobion at 2012-04-13T04:56:39Z
The test in `KernelTest` did not pass for me. That's fixed.
Commits
-------
82bbf3b [HttpKernel] Allow override of ContainerBuilder instance used to build container
Discussion
----------
[HttpKernel] Allow override of ContainerBuilder class in Kernel
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
The ContainerBuilder class is hard coded into the `buildContainer()` method and is therefor not extensible.
---------------------------------------------------------------------------
by fabpot at 2012-04-13T04:54:28Z
I would definitely prefer a `getContainerBuilder()` that returns a container builder. But why would you want to do that?
---------------------------------------------------------------------------
by drak at 2012-04-13T05:12:13Z
@fabpot - The use case is to override the behaviour of the compilation that is performed by the kernel on the container. There is no way to override the compile() method called on the builder because the class is created hard in `Kernel::buildContainer()`. This was the simplest way to change it without other cascading changes.
If we had a method which returned a container builder instance, would it just be something that creates a container builder, or acts as a getter on a property? The reason I ask is there is no real need to have a container builder persisting in a property, so a method that just returns a `new ContainerBuilder()` would also work but then we have to deal with the constructor as there is no way to set a ParameterBag on a container and `Kernel::buildContainer()` needs that.
Basically, my reasoning is that since this particular container is just a throwaway, it seems ok to control the class name that was used to create the throwaway builder.
So are you suggesting doing this instead?
protected function getContainerBuilder()
{
return new ContainerBuilder(new ParameterBag($this->getKernelParameters()));
}
---------------------------------------------------------------------------
by fabpot at 2012-04-13T05:22:20Z
Yes this was my suggestion. But are you sure you cannot solve your problems with compiler passes?
---------------------------------------------------------------------------
by drak at 2012-04-13T05:38:29Z
@fabpot, yes, this particular issue can't be solved by compiler passes. I'll adapt the PR.
---------------------------------------------------------------------------
by drak at 2012-04-13T05:46:03Z
@fabpot, amended as per your suggestion.
Commits
-------
cb47b03 [Routing] small refactoring + language fixes
Discussion
----------
[Routing] small refactoring + language fixes
tests pass, no bc break
---------------------------------------------------------------------------
by vicb at 2012-04-12T06:42:19Z
@Tobion could you squash your commits ?
---------------------------------------------------------------------------
by Tobion at 2012-04-12T19:25:03Z
done
Commits
-------
be2456b [Form] [Tests] Used assertCount()
4120f13 [Form] Added all() method to the FormBuilder class
Discussion
----------
[Form] Added all() method to the FormBuilder class
In order to perform some introspection on a FormBuilder instance,
we need to be able to get its children. Almost everything is accessible
in this class, but the children are not.
This PR adds a all() method to respect the current API (get(), remove(),
...).
---------------------------------------------------------------------------
by bschussek at 2012-04-12T09:54:04Z
👍
Commits
-------
382b083 [Routing] improved generated class by PhpGeneratorDumper
27a05f4 [Routing] small optimization of PhpGeneratorDumper
Discussion
----------
[Routing] improved PhpGeneratorDumper
Test pass: yes
BC break: no
The first commit only replaces arrays with strings and makes some cosmetic changes, so that it's more readable. This makes the `PhpGeneratorDumper` consistent in style with `PhpMatcherDumper` that I fixed recently and should slightly improve performance of the generation of the class.
The second commit changes the output of the `PhpGeneratorDumper->dump` and tries to optimize the resulting class that is used to generate URLs. It's best explained with an example.
Before my changes:
```php
class ProjectUrlGenerator extends Symfony\Component\Routing\Generator\UrlGenerator
{
static private $declaredRouteNames = array(
'Test' => true,
'Test2' => true,
);
/**
* Constructor.
*/
public function __construct(RequestContext $context)
{
$this->context = $context;
}
public function generate($name, $parameters = array(), $absolute = false)
{
if (!isset(self::$declaredRouteNames[$name])) {
throw new RouteNotFoundException(sprintf('Route "%s" does not exist.', $name));
}
$escapedName = str_replace('.', '__', $name);
list($variables, $defaults, $requirements, $tokens) = $this->{'get'.$escapedName.'RouteInfo'}();
return $this->doGenerate($variables, $defaults, $requirements, $tokens, $parameters, $name, $absolute);
}
private function getTestRouteInfo()
{
return array(array ( 0 => 'foo',), array (), array (), array ( 0 => array ( 0 => 'variable', 1 => '/', 2 => '[^/]+?', 3 => 'foo', ), 1 => array ( 0 => 'text', 1 => '/testing', ),));
}
private function getTest2RouteInfo()
{
return array(array (), array (), array (), array ( 0 => array ( 0 => 'text', 1 => '/testing2', ),));
}
}
```
After my changes in second commit:
```php
class ProjectUrlGenerator extends Symfony\Component\Routing\Generator\UrlGenerator
{
static private $declaredRoutes = array(
'Test' => array ( 0 => array ( 0 => 'foo', ), 1 => array ( ), 2 => array ( ), 3 => array ( 0 => array ( 0 => 'variable', 1 => '/', 2 => '[^/]+?', 3 => 'foo', ), 1 => array ( 0 => 'text', 1 => '/testing', ), ),),
'Test2' => array ( 0 => array ( ), 1 => array ( ), 2 => array ( ), 3 => array ( 0 => array ( 0 => 'text', 1 => '/testing2', ), ),),
);
/**
* Constructor.
*/
public function __construct(RequestContext $context)
{
$this->context = $context;
}
public function generate($name, $parameters = array(), $absolute = false)
{
if (!isset(self::$declaredRoutes[$name])) {
throw new RouteNotFoundException(sprintf('Route "%s" does not exist.', $name));
}
list($variables, $defaults, $requirements, $tokens) = self::$declaredRoutes[$name];
return $this->doGenerate($variables, $defaults, $requirements, $tokens, $parameters, $name, $absolute);
}
}
```
As you can see, there is no need to escape the route name and invoke a special method anymore. Instead the route properties are included in the static route array directly, that existed anyway. Is also easier to read as defined routes and their properties are in the same place.
Commits
-------
03d30fd [Routing] remove duplicated cache of compiled routes
Discussion
----------
[Routing] remove duplicated cache of compiled routes
The UrlGenerator caches compiled routes for generating URLs. But the Route class caches it's compiled route itself as long as it does not get modified. So compiled routes are cached twice which makes no sense in my opinion.
Test pass: yes
BC break: no
Commits
-------
89a5c1a [process] Added destructor to process to make sure handles are always closed in the right order.
Discussion
----------
[process] Added destructor to process to make sure handles are always cl...
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Commits
-------
6f56dfc [Form] Fixed DateType default options
779d3bb [Form] Fixed documentation, and the DateType (default options)
Discussion
----------
[Form] Fixed documentation, and the DateType (default options)
---------------------------------------------------------------------------
by fabpot at 2012-04-11T16:48:04Z
That breaks the tests.
---------------------------------------------------------------------------
by willdurand at 2012-04-11T16:50:35Z
I got an error with the Form test suite before to write this patch..
---------------------------------------------------------------------------
by willdurand at 2012-04-11T16:53:30Z
Nevermind, I can see broken tests.. I'm on, sorry
---------------------------------------------------------------------------
by willdurand at 2012-04-11T16:57:52Z
@fabpot fixed.
```
OK, but incomplete or skipped tests!
Tests: 945, Assertions: 1439, Incomplete: 11.
```
In order to perform some introspection on a FormBuilder instance,
we need to be able to get its children. Almost everything is accessible
in this class, but the children are not.
This PR adds a all() method to respect the current API (get(), remove(),
...).
Commits
-------
8329087 [Form] Moved calculation of ChoiceType options to closures
5adec19 [Form] Fixed typos
cb87ccb [Form] Failing test for empty_data option BC break
b733045 [Form] Fixed option support in Form component
Discussion
----------
[Form] Fixed option support in Form component
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #3354, #3512, #3685, #3694
Todo: -
This PR also introduces a new helper `DefaultOptions` for solving option graphs. It accepts default options to be defined on various layers of your class hierarchy. These options can then be merged with the options passed by the user. This is called *resolving*.
The important feature of this utility is that it lets you define *lazy options*. Lazy options are specified using closures that are evaluated when resolving and thus have access to the resolved values of other (potentially lazy) options. The class detects cyclic option dependencies and fails with an exception in this case.
For more information, check the inline documentation of the `DefaultOptions` class and the UPGRADE file.
@fabpot: Might this be worth a separate component? (in total the utility consists of five classes with two associated tests)
---------------------------------------------------------------------------
by beberlei at 2012-04-05T08:54:10Z
"The important feature of this utility is that it lets you define lazy options. Lazy options are specified using closures"
What about options that are closures? are those differentiated?
---------------------------------------------------------------------------
by bschussek at 2012-04-05T08:57:35Z
@beberlei Yes. Closures for lazy options receive a Symfony\Component\Form\Options instance as first argument. All other closures are interpreted as normal values.
---------------------------------------------------------------------------
by stof at 2012-04-05T11:09:49Z
I'm wondering if these classes should go in the Config component. My issue with it is that it would add a required dependency to the Config component and that the Config component mixes many different things in it already (the loader part, the resource part, the definition part...)
---------------------------------------------------------------------------
by sstok at 2012-04-06T13:36:36Z
Sharing the Options class would be great, and its more then one class so why not give it its own Component folder?
Filesystem is just one class, and that has its own folder.
Great job on the class bschussek 👏
---------------------------------------------------------------------------
by bschussek at 2012-04-10T12:32:34Z
@fabpot Any input?
---------------------------------------------------------------------------
by bschussek at 2012-04-10T13:54:13Z
@fabpot Apart from the decision about the final location of DefaultOptions et al., could you merge this soon? This would make my work a bit easier since this one is a blocker.
---------------------------------------------------------------------------
by fabpot at 2012-04-10T18:08:18Z
@bschussek: Can you rebase on master? I will merge afterwards. Thanks.
This demonstrates the issue described in symfony/symfony#3354. FieldType no longer has access to the child type's data_class option, which makes it unable to create the default closure for empty_data.
Commits
-------
f9a486e [Validator] Added support for pluralization of the SizeLengthValidator
c0715f1 [FrameworkBundle], [TwigBundle] added support for form error message pluralization
7a6376e [Form] added support for error message pluralization
345981f [Validator] added support for plural messages
Discussion
----------
[Validator] Added support for plural error messages
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Todo: create translations for en and update others (FrameworkBundle)
[](http://travis-ci.org/hason/symfony)
---------------------------------------------------------------------------
by fabpot at 2011-05-14T20:41:01Z
@bschussek: What's your opinion?
---------------------------------------------------------------------------
by stof at 2011-09-04T13:14:29Z
@hason could you rebase your branch on top of master and update the PR ?
You also need to change the messages in the constraint that uses the pluralization to a pluralized format.
---------------------------------------------------------------------------
by stof at 2011-10-16T18:06:22Z
@hason ping
---------------------------------------------------------------------------
by stof at 2011-11-11T14:58:19Z
@hason ping again
---------------------------------------------------------------------------
by stof at 2011-12-12T20:39:10Z
@hason ping again. Can you update your PR ?
---------------------------------------------------------------------------
by hason at 2011-12-12T21:29:14Z
@stof I hope that I will update PR this week.
---------------------------------------------------------------------------
by bschussek at 2012-01-15T19:07:32Z
Looks good to me.
---------------------------------------------------------------------------
by canni at 2012-02-02T17:28:54Z
@hason can you update this PR and squash commits, it conflicts with current master
---------------------------------------------------------------------------
by hason at 2012-02-09T07:21:41Z
@stof, @canni Rebased.
What is the best solution for the translation of messages?
1. Change messages in the classes and all xliff files?
2. Keep messages in the classes and change all xliff files?
---------------------------------------------------------------------------
by stof at 2012-02-09T08:19:41Z
The constraints contain the en message so you will need to modify them to update the message
---------------------------------------------------------------------------
by hason at 2012-02-09T08:55:55Z
I prefer second option. The Validator component should be decoupled from the Translation component. The constraints contain the en message which is also the key for Translation component. We should create validators.en.xlf in the FrameworkBundle for en message. I think that this is better solution. What do you think?
---------------------------------------------------------------------------
by stof at 2012-04-04T02:22:02Z
@hason Please rebase your branch. It conflicts with master because of the move of the tests
@fabpot ping
Commits
-------
efad5d5 [Filesystem] Prevented infiite loop on windows while calling mirror on symlink. Added test for mirroring symlinks.
Discussion
----------
[Filesystem] Prevented infinite loop on windows while mirrorring symlinks
First check for filetype in *mirror()* method is:
if (is_link($file)) {
$this->symlink($file, $target);
later we see:
} elseif (is_file($file) || ($copyOnWindows && is_link($file))) {
$this->copy($file, $target, isset($options['override']) ? $options['override'] : false);
The later check for links on windows (*$copyOnWindows && is_link($file)*) won't ever get called. Calling *symlink()* in *mirror()* on windows would lead to calling *mirror()* again.
Note that I didn't actually try running it on windows platform. I added a test for mirroring symlinks (non-windows test). I think it'd be good if someone added some windows specific tests to this class.
I also modified the target path:
$target = $targetDir.'/'.str_replace($originDir.DIRECTORY_SEPARATOR, '', $file->getPathname());
It didn't use DIRECTORY_SEPARATOR and is equivalent to:
$target = str_replace($originDir, $targetDir, $file->getPathname());
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: ~
Commits
-------
9307f5b [Routing] Implement bug fixes and enhancements
Discussion
----------
[Routing] Implement bug fixes and enhancements (from @Tobion)
This is mainly #3754 with some minor formatting changes.
Original PR message from @Tobion:
Here a list of what is fixed. Tests pass.
1. The `RouteCollection` states
> it overrides existing routes with the same name defined in the instance or its children and parents.
But this is not true for `->addCollection()` but only for `->add()`. addCollection does not remove routes with the same name in its parents (only in its children). I don't think this is on purpose.
So I fixed it by making sure there can only be one route per name in all connected collections. This way we can also simplify `->get()` and `->remove()` and improve performance since we can stop iterating recursively when we found the first and only route with a given name.
See `testUniqueRouteWithGivenName` that fails in the old code but works now.
2. There was an bug with `$collection->addPrefix('0');` that didn't apply the starting slash. Fixed and test case added.
3. There is an issue with `->get()` that I also think is not intended. Currently it allows to access a sub-RouteCollection by specifing $name as array integer index. But according to the PHPdoc you should only be allowed to receive a Route instance and not a RouteCollection.
See `testGet` that has a failing test case. I fixed this behavior.
4. Then I recognized that `->addCollection` depended on the order of applying them. So
$collection1->addCollection($collection2, '/b');
$collection2->addCollection($collection3, '/c');
$rootCollection->addCollection($collection1, '/a');
had a different pattern result from
$collection2->addCollection($collection3, '/c');
$collection1->addCollection($collection2, '/b');
$rootCollection->addCollection($collection1, '/a');
Fixed and test case added. See `testPatternDoesNotChangeWhenDefinitionOrderChanges`.
5. PHP could have ended in an infinite loop when one tried to add an existing RouteCollection to the tree. Fixed by throwing an exception when this situation is detected. See tests `testCannotSelfJoinCollection` and `testCannotAddExistingCollectionToTree`.
6. I made `setParent()` private because its not useful outside the class itself. And `remove()` also removes the route from its parents. Added public `getRoot()` method.
7. The `Route` class throwed a PHP warning when trying to set an empty requirement.
8. Fixed issue #3777. See discussion there for more info. I fixed it by removing the over-optimization that was introduced in 91f4097a09 but didn't work properly. One cannot reorder the route definitions, as is was done, because then the wrong route might me matched before the correct one. If one really wanted to do that, it would require to calculate the intersection of two regular expressions to determine if they can be grouped together (a tool that would also be useful to check whether a route is unreachable, see discussion in #3678) We can only safely optimize routes with a static prefix within a RouteCollection, not across multiple RouteCollections with different parents. This is especially true when using variables and regular expressions requirements.
I could however apply an optimization that was missing yet: Collections with a single route were missing the static prefix optimization with `0 === strpos()`.
9. Fixed an issue where the `PhpMatcherDumper` would not apply the optimization if the root collection to be dumped has a prefix itself. For this I had to rewrite `compileRoutes`. It is also much easier to understand now. Addionally I added many comments and PHPdoc because complex recursive methods like this are still hard to grasp.
I added a test case for this (`url_matcher3.php`).
10. Fix that `Route::compile` needs to recompile a route once it is modified. Otherwise we have a wrong result. Test case added.
Commits
-------
f617e02 [Validator] added less-strict email host verification
Discussion
----------
[Validator] added less-strict email host verification
uhhhhh, my first pull request :>. uhm... tell me if i did something wrong :)
#### Request info
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes... well,not really (i guess master branch is not passing - at least i didnt broke the email test)
Fixes the following tickets: #2827
#### Description
New checkHost attribute in email constraint will make the validator check for only one of MX, A or AAAA DNS resource records to verify it as a valid email address.
Commits
-------
039ff6f allow more control on GetSetMethodNormalizer by using callback functions and an ignoreAttributes list
Discussion
----------
allow more control on GetSetMethodNormalizer
Here is an other attempt. You would use this as follows:
$serializer = new \Symfony\Component\Serializer\Serializer(
array(new \Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer()),
array('json' => new \Symfony\Component\Serializer\Encoder\JsonEncoder())
);
$callbacks = array('books' => function ($books) { return NULL; });
return new Response(
$serializer->serialize($paginator->getRows(), 'json', $callbacks),
200,
array('Content-Type' => 'application/json')
);
Besides of returning NULL, you could also do things like:
$callbacks = array(
'books' => function ($books) {
$ids = array();
foreach ($books as $book) {
$ids[] = $book->getId();
}
return $ids;
},
'author' => function ($author) {
return $author->getId();
},
'creationDate' => function ($creationDate) {
return $creationDate->format('d/m/Y');
},
);
The commit is not complete yet. But at this point I am interested in your opinions.
---------------------------------------------------------------------------
by lsmith77 at 2012-03-12T22:53:18Z
in general i agree that using a callback is a good solution to provide more power without complicating the API or implementation in this case.
please add a test case, this should also help illustrate how this can be used in practice.
---------------------------------------------------------------------------
by schmittjoh at 2012-03-13T04:54:33Z
Note that your change breaks the API defined by the interface, i.e. someone using this method needs to type-hint the serializer implementation, not the interface.
It also adds a parameter to the public API of the serializer which will only work with one specific normalizer. What if another normalizer needs additional information, should another parameter be added to the serialize method? What about deserialization?
Bottom line is, the serializer component was simply not designed for this kind of thing. I've tried to make it more flexible before creating the bundle, but some things simply cannot be fixed in a sane way.
---------------------------------------------------------------------------
by tvlooy at 2012-03-13T06:07:45Z
Would just adding a setCallbacks() to the GetSetMethodNormalizer be a better solution? That doesn't touch the API. I will try to write some tests this evening.
---------------------------------------------------------------------------
by schmittjoh at 2012-03-13T16:22:50Z
That would definitely be better.
You would then need to retrieve the normalizer instance before calling ``serialize`` on the serializer which also leaves a stale taste, but I have no other solution for now.
---------------------------------------------------------------------------
by tvlooy at 2012-03-13T21:32:26Z
So, this should be it then. Yet an other usage example:
$normalizer = new \Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer();
$normalizer->setCallbacks(
array(
'books' => function ($books) {
$ids = array();
foreach ($books as $book) {
$ids[] = $book->getId();
}
return $ids;
},
)
);
$serializer = new \Symfony\Component\Serializer\Serializer(
array($normalizer),
array('json' => new \Symfony\Component\Serializer\Encoder\JsonEncoder())
);
return new Response(
$serializer->serialize($paginator->getRows(), 'json'),
200,
array('Content-Type' => 'application/json')
);
---------------------------------------------------------------------------
by tvlooy at 2012-03-18T21:16:48Z
Anything else needed for this to get pulled in?
---------------------------------------------------------------------------
by tvlooy at 2012-03-19T18:33:58Z
Hm, I like to keep it that way because I like the fact that not passing a callable will result in a warning instead of silently skipping it. You don't get that behaviour by treating it as null.
---------------------------------------------------------------------------
by vicb at 2012-03-19T23:15:37Z
I was unclear: the code should throw an exception when an element is not callable, this is why `null` will not be supported any more (it is not a callback as the `setCallbacks` indicate).
They are several way to support the former behavior:
* the cb can return a defined interface,
* the cb can throw a defines exc,
* by adding a `setIgnoredAttributes` method
Please also squash your commits.
---------------------------------------------------------------------------
by tvlooy at 2012-03-20T21:02:06Z
Yes, I like the setIgnoredAttributes solution. I changed it and squashed the commits.
---------------------------------------------------------------------------
by tvlooy at 2012-03-26T20:07:36Z
some improvements and squashed the commits
---------------------------------------------------------------------------
by stof at 2012-04-03T22:36:15Z
@tvlooy Please rebase your branch. It conflicts with master because of the move of tests.
---------------------------------------------------------------------------
by tvlooy at 2012-04-04T07:43:47Z
@stof I will do it on saturday, if that is ok with you.
---------------------------------------------------------------------------
by fabpot at 2012-04-10T18:29:30Z
Is it mergeable now? ping @Seldaek, @schmittjoh.
---------------------------------------------------------------------------
by tvlooy at 2012-04-10T18:55:04Z
yes, it should be
Commits
-------
c36651b Fixed spelling error
f123684 Removed leftover from c/p
b74a5d4 Updated to new cache loader pattern.
7e66908 Added XCache class loader
Discussion
----------
[ClassLoader] Added XCache class loader
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
There is no tests, as it seems there is no way to use xcache storage functions from CLI.
---------------------------------------------------------------------------
by stof at 2012-04-06T20:12:09Z
Please implement a XcacheClassLoader following the same pattern than the new ApcClassLoader instead
---------------------------------------------------------------------------
by cordoval at 2012-04-07T14:20:47Z
- should include tests
- should include documentation (will you also update the component documentation for this new class?)
---------------------------------------------------------------------------
by stof at 2012-04-07T14:25:00Z
@cordoval the PR explains why there is no tests: xcache canot be used in the CLI.
---------------------------------------------------------------------------
by cordoval at 2012-04-07T14:26:43Z
ok @stof sorry it said it seemed not to be possible, i thought it was possible but I am wrong.
---------------------------------------------------------------------------
by kimhemsoe at 2012-04-07T15:01:24Z
@cordoval My english is really horrible. I would not mind if someone else could do that task for me. We also need to add doc for the new ApcClassLoader.
---------------------------------------------------------------------------
by cordoval at 2012-04-07T15:03:57Z
I wish you can explain me more then about this class and how to use it in code so then I can write easily the documentation :D deal?
---------------------------------------------------------------------------
by kimhemsoe at 2012-04-07T15:21:25Z
Deal :P
The XcacheClassLoader and ApcClassLoader replaces the old ApcUniversalClassLoader.
They giving us support for using another loader then UniversalClassLoader, without duplicating the cache layer.
Aslong it have a public function findFile($class) method.
$loader = new ClassLoader();
// register classes with namespaces
$loader->add('Symfony\Component', __DIR__.'/component');
$loader->add('Symfony', __DIR__.'/framework');
$cachedLoader = new XcacheClassLoader('my_prefix', $loader);
// activate the cached autoloader
$cachedLoader->register();
Think that is more or less the essence of this.
---------------------------------------------------------------------------
by cordoval at 2012-04-09T08:28:53Z
it is not add but registerNamespace right?
so the main idea is to get rid of the restriction to use Apc with Universal loader
what is the comparative advantage between APC and Xcache?
---------------------------------------------------------------------------
by kimhemsoe at 2012-04-09T08:55:23Z
Yes if the $loader (class finder) were to be a instance UniversalClassLoader.
Yes the main idea is to be able to reuse the cache layer with any class loader there obey to the one restriction.
Difference between apc and xcache and why to use what is coming down to taste and your setup. We use xcache as APC have some issues in fastcgi setups. when we upgrade to php54 at somepoint we get to chance to move to php-fpm wich solves these issues. Short story: Slightly out of scope for any documentation in here :-P
Commits
-------
65aa387 [Form] Fixed index generation in EntityChoiceList if ID is not an integer
Discussion
----------
[Form] Fixed index generation in EntityChoiceList if ID is not an integer
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3635
Todo: -
Commits
-------
77185e0 [Routing] Allow spaces in the script name for the apache dumper
6465a69 [Routing] Fixes to handle spaces in route pattern
Discussion
----------
[Routing] Handling of space characters in the dumpers
The compiler was using the 'x' modifier in order to ignore extra spaces and line feeds but the code was flawed:
- it was actually ignoring all the spaces, not only the extra ones added by the compiler,
- all the spaces were stripped in the php and apache matchers.
The proposed fix:
- do not use the 'x' modifier any more (and then do no add extra spaces / line feeds),
- do not strip the spaces in the matchers,
- escapes the spaces (both in regexs and script name) for the apache matcher.
It also include [a small optimization](https://github.com/vicb/symfony/pull/new#L9L89) when the only token of a route is an optional variable token - the idea is to make the regex easier to read.
---------------------------------------------------------------------------
by vicb at 2012-04-10T13:59:45Z
@Baachi fixed now. Thanks.
---------------------------------------------------------------------------
by Tobion at 2012-04-10T16:01:31Z
+1, I saw no reason for pretty printing the regex in the first place (just for debugging I guess).
@vicb since you want to make the regex easier to read, I propose the remove the `P` from the variable regex `?P<bar>`, which is not needed anymore in PHP 5.3 (and we only support PHP 5.3+ anyway).
---------------------------------------------------------------------------
by vicb at 2012-04-10T16:08:36Z
@Tobion could you make a PR to this branch for the named parameters ?
---------------------------------------------------------------------------
by Tobion at 2012-04-10T16:12:34Z
I can include it in #3754 because I'm about the add 2 more fixes to it anyway.
But when I proposed to apply these fixes to 2.0 Fabien rejected it. So not sure what branch you want me to apply this.
---------------------------------------------------------------------------
by vicb at 2012-04-10T16:25:38Z
May be the best is to put it on hold while I am reviewing your PRs. There are already enough changes, we'll make an other PR after all have been sorted out.
What's the difference between 3754 and 3810 ? (3810 + 3763 = 3754 ?)
---------------------------------------------------------------------------
by Tobion at 2012-04-10T16:39:32Z
Lol you forget to link the PR numbers. At first sight I thought it's some sort of mathematical riddle. Haha
#3810 is for 2.0 = #3763 (already merged) + #3754 for master
---------------------------------------------------------------------------
by vicb at 2012-04-10T16:52:18Z
I didn't link on purpose... the question is if '=' means strictly or loosely equal (any diffs - beside master vs 2.0) ?
---------------------------------------------------------------------------
by Tobion at 2012-04-10T17:06:04Z
It just applies my changes to 2.0. Nothing more. So master still differs from 2.0 by the addional features that were already implemented (e.g. `RouteCollection->addCollection` with optional requirements and options). But since my changes are bug fixes (except the performance improvement in #3763 but that doesn't break anything and makes 2.0 easier to maintain) I thought they should go into 2.0 as well.
---------------------------------------------------------------------------
by vicb at 2012-04-10T17:14:27Z
@Tobion only bug fixes mean "only bug fixes". You should re-open a PR for 2.0 with "only bug fixes", you might want to wait for me to review 3754.
---------------------------------------------------------------------------
by Tobion at 2012-04-10T17:21:00Z
Without #3763 it's much harder to apply the bug fixes. And now that I found 2 more bugs which requiresome rewriting of the PhpMatcherDumper, I don't want to apply all the commits by hand again for 2.0...
Commits
-------
0024ddc Fix for using route name as check_path.
Discussion
----------
Security Bundle route as check_path
In the current 2.0 branch you can't use a route as
firewalls:
admin_area:
login_path:
you will get a InvalidConfigurationException.
In the 2.1 version this is fixed. Since 2.1 isn't released i think this fix should be merged into the 2.0 branch too. Many people have this problem (https://github.com/schmittjoh/JMSI18nRoutingBundle/issues/7) for example which effectively blocks internationalisation in combination with the firewall.
---------------------------------------------------------------------------
by stof at 2012-04-10T13:35:13Z
@fabpot ping
Commits
-------
c4e68a3 [Form] Moved logic of addXxx()/removeXxx() methods to the PropertyPath class
Discussion
----------
[Form] Moved logic of addXxx()/removeXxx() methods to the PropertyPath class
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3732
Todo: -
The addXxx()/removeXxx() methods should now be called correctly in ChoiceType and CollectionType.
PropertyPath now favors addXxx()/removeXxx() over setXxx() for collections. For example:
```
$propertyPath = new PropertyPath('article.tags');
// Tries to use addTag()/removeTag() and only uses setTags() (et al.)
// if not found
$propertyPath->setValue($article, $tags);
```
For other languages than English or very irregular plurals, a custom singular can be set by separating it with a pipe:
```
$propertyPath = new PropertyPath('article.genera|genus');
```
---------------------------------------------------------------------------
by bschussek at 2012-04-07T12:40:39Z
Again, the failing build is not my fault.
