Commits
-------
0f86a33 micro-optim: replace for with foreach
4efb9fe [Form] Remove unneeded FormUtil constants
Discussion
----------
[Form] Remove FormUtil constants
The constants are not useful from outside the class as the $pluralMap is also private. So no need to expose these veriables in the API when they cannot be used in any way. Unfortunately there are not private constants, so use private static. Then I realized the variables can be removed altogether, as they are only used once anyway and the index meaning is already documented in pluralMap.
---------------------------------------------------------------------------
by empire at 2012-08-18T12:58:22Z
FormUtils is abstract class, and maybe subclass (in future) will use this constants, I think changing access modifier to `protected` is better option.
---------------------------------------------------------------------------
by Tobion at 2012-08-18T12:59:40Z
They cannot, as pluralMap is private...
---------------------------------------------------------------------------
by Partugal at 2012-08-18T14:11:17Z
extract self::$pluralMap into local variable add small speed up
4.5499801635742 ms vs 5.7430267333984 ms on 100 iterations
---------------------------------------------------------------------------
by Tobion at 2012-08-18T14:16:47Z
This is not about performance.
---------------------------------------------------------------------------
by Partugal at 2012-08-18T14:21:11Z
yes but adds
your changes vs current is
5.7430267333984 ms vs 6.4971446990967 ms on 100 iterations
---------------------------------------------------------------------------
by Tobion at 2012-08-18T14:29:48Z
How about `$map =& self::$pluralMap[$i]`?
---------------------------------------------------------------------------
by Partugal at 2012-08-18T14:59:57Z
I mean https://gist.github.com/3387253
---------------------------------------------------------------------------
by Partugal at 2012-08-18T15:01:45Z
foreach is event faster :)
(4.1971206665039 ms on my hw)
---------------------------------------------------------------------------
by Tobion at 2012-08-18T15:04:51Z
I see.
---------------------------------------------------------------------------
by Partugal at 2012-08-18T15:06:41Z
in first comment i mean code like this:
```php
$pluralMap = self::$pluralMap; // do this because access to static property is to slow
```
on my machine & is slower `$map =& $pluralMap[$i]` vs `$map = $pluralMap[$i]`
5.0 vs 4.8 ms
imho & not needed in read only code
---------------------------------------------------------------------------
by Tobion at 2012-08-18T15:15:03Z
Well, you'd need to benchmark memory too. `=&` should reduce memory primarily in this case.
---------------------------------------------------------------------------
by Partugal at 2012-08-18T15:19:35Z
```php
echo memory_get_usage() . "\n"; // 711536
$a = array_fill(5, 6000, 'banana');
echo memory_get_usage() . "\n"; // 1497632
$b = $a;
echo memory_get_usage() . "\n"; // 1497760
$b[1] = 2;
echo memory_get_usage() . "\n"; // 2283832
```
1497760 - 1497632 = 128 it is size for variable structure not for its value:
```php
echo memory_get_usage() . "\n"; // 711536
$a = 1;
echo memory_get_usage() . "\n"; // 1497632
$b = &$a;
echo memory_get_usage() . "\n"; // 1497760
```
---------------------------------------------------------------------------
by Seldaek at 2012-08-18T17:52:32Z
@Tobion http://schlueters.de/blog/archives/125-Do-not-use-PHP-references.html - search for "copy-on-write" if you don't want to read it all.
---------------------------------------------------------------------------
by Tobion at 2012-08-18T19:37:44Z
Yeah I know about copy on write. I thought there might be a difference what you assign a sub-element of an array to a variable. But apparently not.
Interestingly `$a =& $b` takes a little more memory than `$a = $b` according to `memory_get_usage ()` but not when using `memory_get_usage (true)`.
---------------------------------------------------------------------------
by bschussek at 2012-08-30T08:15:01Z
I don't like the removal of the constants. They introduce meaning into the integers and improve code clarity. The rest looks good.
---------------------------------------------------------------------------
by Tobion at 2012-08-30T13:18:19Z
My opinion of the constants:
- They are part of the public API (as const are alwalys public) but cannot be used at all, as everything else is private...
- They are each only used once.
- The meaning of the indices is already documented in `$pluralMap`
- They are not used when building `$pluralMap` so they dont imprivate code clarity and consistence either. But doing so would on the other hand make it probably more ugly. So removing them is IMO best solution.
---------------------------------------------------------------------------
by bschussek at 2012-08-30T15:21:03Z
If you really need to remove the constants, then please comment the code where they are used accordingly.
---------------------------------------------------------------------------
by Tobion at 2012-08-31T00:58:51Z
I dont see what I should comment to make it more understandable, as the the map is already assigned to a named variable like `$suffixLength = $map[1];`.
---------------------------------------------------------------------------
by bschussek at 2012-08-31T09:12:18Z
> I dont see what I should comment to make it more understandable, as the the map is already assigned to a named variable
`$map[2]` and `$map[3]` is not self-describing.
---------------------------------------------------------------------------
by Tobion at 2012-08-31T17:23:15Z
@bschussek Done.
---------------------------------------------------------------------------
by bschussek at 2012-08-31T22:13:41Z
Could you please squash your commits?
Commits
-------
1b5ad17 Revert "Removed MySQL-exclusive usage of unsigned integer from table creation"
Discussion
----------
[Security][DBAL] Revert MySQL unsigned removal
Revert "Removed MySQL-exclusive usage of unsigned integer from table creation"
This reverts commit 57694aaa94.
The problem is underlying in Doctrine DBAL change tracking and should
either be fixed or ignored there.
I opened a ticket on Doctrine Jira http://doctrine-project.org/jira/browse/DBAL-322
---------------------------------------------------------------------------
by fabpot at 2012-08-14T06:40:47Z
I will merge this PR after we have a release of DBAL that includes the fix for DBAL-322.
---------------------------------------------------------------------------
by acasademont at 2012-08-20T08:01:48Z
This was already fixed 2 weeks ago in doctrine/dbal#183 so i guess this can be closed
---------------------------------------------------------------------------
by acasademont at 2012-08-20T08:02:06Z
merged i mean
Commits
-------
3036b00 JsonResponseTest
Discussion
----------
JsonResponseTest
Hi,
This patch adds some tests for JsonResponse.
Best regards,
Michal
---------------------------------------------------------------------------
by eventhorizonpl at 2012-09-01T07:09:12Z
Done. Thanks for the review!
Commits
-------
8a3c8c9 load test
Discussion
----------
load test
Hi,
This patch add test that covers this situation
public static function load($classes, $cacheDir, $name, $autoReload, $adaptive = false, $extension = '.php')
{
// each $name can only be loaded once per PHP process
if (isset(self::$loaded[$name])) {
return;
}
Best regards,
Michal
Commits
-------
0af8778 Response tests
Discussion
----------
Response tests
Hi,
This patch adds some tests to ResponseTest.
Best regards,
Michal
---------------------------------------------------------------------------
by eventhorizonpl at 2012-09-01T09:45:16Z
Fixed, thanks for the review.
---------------------------------------------------------------------------
by eventhorizonpl at 2012-09-02T19:39:26Z
CS fixed. Thanks for the review :)
Commits
-------
c74d9a9 ResponseHeaderBag tests
Discussion
----------
ResponseHeaderBag tests
Hi,
This patch adds some ResponseHeaderBag tests. Now ResponseHeaderBag got 100% test coverage :)
Best regards,
Michal
Commits
-------
b89d4ee StreamedResponseTest
Discussion
----------
StreamedResponseTest
Hi,
This patch adds one test to StreamedResponseTest and fixes another. StreamedResponse has 100% test coverage.
Best regards,
Michal
Commits
-------
21a5841 RedirectResponse tests
Discussion
----------
RedirectResponse tests
Hi,
This patch adds 100% test coverage for RedirectResponse class.
Best regards,
Michal
Commits
-------
04fd5f1 [Form] Fixed PropertyPath to not modify Collection instances (not even their clones)
Discussion
----------
[Form] Fixed PropertyPath to not modify Collection instances
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4670
Todo: -
---------------------------------------------------------------------------
by pocallaghan at 2012-08-31T14:20:52Z
As far as I can see the pull request does fix the issue, which makes sense based on the code change (I didn't know iterator_to_array existed, good call). One thing I would say, I'm not sure on the use in the change to the test case. It's not clear to me what additional protection this extra assertion gives, as both the old and new code seem to pass.
---------------------------------------------------------------------------
by bschussek at 2012-08-31T14:21:46Z
The new assertion is there because not even the old code (`clone`) was tested.
---------------------------------------------------------------------------
by stof at 2012-08-31T14:37:38Z
@bschussek but was it failing without the code change ?
---------------------------------------------------------------------------
by bschussek at 2012-08-31T22:12:00Z
@stof It was not, but I was unable to write a good test for the change within reasonable time. I added an explanatory comment instead.
They are part of the public API (as const are always public) but cannot be used at all from outside the class as the$pluralMap is private. The meaning of the indices is already documented in the array.
Commits
-------
cb7e3f5 [Routing] added route compile check to identify a default value of a required variable that does not match the requirement
Discussion
----------
[Routing] added route compile check to identify a bad default value
BC break: yes but only for strange route definitions
See the exception message in code for the reasoning.
An exception is thrown for a __required__ variable that __has a default__ that __doesn't match__ the requirement.
So optional variables can of course still have a default that don't meet the requirement, which is useful.
This helps to identify useless route definitions at compile time instead of when generating or matching a URL.
Commits
-------
890aea2 FileLocatorInterface used in typehint instead of FileLocator
Discussion
----------
FileLocatorInterface used in typehint instead of FileLocator
---------------------------------------------------------------------------
by stof at 2012-08-30T22:09:39Z
@fabpot this makes sense (and it is BC)
---------------------------------------------------------------------------
by mvrhov at 2012-08-31T08:34:17Z
What's wrong with Interface hint? I always hint interface when available as this means that I can inject whatever class implementing that interface.
Commits
-------
7503ec9 Issue #5307: HTML regexp when match is false
Discussion
----------
Issue #5307: HTML regexp when match is false
When match is false the html5 validation regexp should be either inverted or not added.
Since we are in RC added a fix where this is not added, but marked a @todo so that this
can be revisited and we try to inverse the regexp instead.
Discussed in #5307.
---------------------------------------------------------------------------
by bschussek at 2012-08-30T08:40:06Z
👍 once the CS issue is fixed.
---------------------------------------------------------------------------
by rdohms at 2012-08-30T09:23:57Z
Could swear that was the CS in PSR-1 or 2, anyway, fixed.
---------------------------------------------------------------------------
by fabpot at 2012-08-30T09:26:07Z
Can you squash your commits before I merge? Thanks.
---------------------------------------------------------------------------
by rdohms at 2012-08-30T09:54:26Z
@fabpot done.
When match is false the html5 validation regexp should be either inverted or not added.
Since we are in RC added a fix where this is not added, but marked a @todo so that this
can be revisited and we try to inverse the regexp instead.
Commits
-------
58ebd1b [Form] Fixed error bubbling from DateTime widget - Issue #52708ea1607 Update src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php
Discussion
----------
[Form] Fixed error bubbling from DateTime widget - Issue #5270
This is related to https://github.com/symfony/symfony/issues/5270
---------------------------------------------------------------------------
by mpiecko at 2012-08-16T19:37:45Z
Travisbot shows something like this in it's log:
[Composer\Downloader\TransportException] The "http://nodeload.github.com/phingofficial/phing/zipball/2.4.12" file could not be downloaded (HTTP/1.1 500 Internal Server Error)
So is it my PR ot Travis CI who fails ... ? I saw this error in some other PR's ...
---------------------------------------------------------------------------
by stloyd at 2012-08-16T20:40:39Z
It's GitHub =)
---------------------------------------------------------------------------
by mpiecko at 2012-08-17T09:36:31Z
Bad GitHub :)
---------------------------------------------------------------------------
by bschussek at 2012-08-17T11:21:39Z
Could you please add a test to DateTimeTypeTest?
---------------------------------------------------------------------------
by mpiecko at 2012-08-17T12:23:40Z
Sure!
---------------------------------------------------------------------------
by bschussek at 2012-08-30T08:20:08Z
👍
Commits
-------
0706d18 [Routing] fixed 4 bugs in the UrlGenerator
Discussion
----------
[Routing] UrlGenerator: fixed missing query param and some ignored requirements
This was pretty hard to figure out. I could fix 4 bugs and refactor the code to safe 2 variables and several assignments. Sorry for doing this in one commit, but they were highly interdependent.
See the added tests for what was fixed. The most obvious bug was that a query param was ignored if it had by accident the same name as a default param (but wasn't used in the path).
In 3 cases it generated the wrong URL that wouldn't match this route. The generator wrongly ignored either the requirements or the passed parameter. I had to adjust one test that was asserting something wrong (see comments).
---------------------------------------------------------------------------
by Tobion at 2012-08-13T14:22:35Z
ping @fabpot
---------------------------------------------------------------------------
by Tobion at 2012-08-29T17:53:07Z
@fabpot I think it's important to merge this before 2.1 final.
Commits
-------
f2d8a8a Refactor the unit test for the "MongoDbSessionHandler"
Discussion
----------
[HttpFoundation] Refactor the unit test for the "MongoDbSessionHandler"
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -
---------------------------------------------------------------------------
by drak at 2012-08-29T19:49:49Z
Big +1 from me. Exactly how these kind of tests should be written.
Commits
-------
eb2eba1 [Form] don't allow users to force exceptions by submitting unexpected data
Discussion
----------
[Form] don't allow users to force exceptions by submitting unexpected data
fix#5334
This makes it more fault-tolerant by simply ignoring wrong stuff from hackers.
@bschussek: I didn't find any other UnexpectedTypeExceptions that could be invoked by simply submitting unexpected data. But I'm not 100% sure that there aren't any indirectly invokeable, e.g. in some listeners.
---------------------------------------------------------------------------
by stof at 2012-08-24T22:34:52Z
a test is missing for this.
---------------------------------------------------------------------------
by Tobion at 2012-08-24T23:02:26Z
@stof true, I will add one
---------------------------------------------------------------------------
by Tobion at 2012-08-25T13:51:23Z
Added test.
---------------------------------------------------------------------------
by bschussek at 2012-08-29T11:07:37Z
👍
Could you please squash the commits?
---------------------------------------------------------------------------
by Tobion at 2012-08-29T13:43:52Z
Done.
Commits
-------
7e8ab54 [Form] raise OutOfBoundsException instead of InvalidArgumentException for inexistent form childs to be in line with PropertyPath
Discussion
----------
[Form] raise OutOfBoundsException instead of InvalidArgumentException in Form::get
BC break: yes
Raise OutOfBoundsException instead of InvalidArgumentException in Form::get for inexistent form childs to be in line with PropertyPath, which also uses OutOfBoundsException for invalid indexes. OutOfBoundsException fits much better as it extends RuntimeException instead of LogicException and this error can typically not be detected at compile time.
---------------------------------------------------------------------------
by bschussek at 2012-08-29T11:01:01Z
👍
---------------------------------------------------------------------------
by stloyd at 2012-08-29T11:07:51Z
Shouldn't this change be noted in upgrade file ?
---------------------------------------------------------------------------
by stof at 2012-08-29T11:23:04Z
it should (and in the changelog of the component)
Commits
-------
e7e39e0 [Form] refactor Guess
dcbeeb1 [Form] replaced UnexpectedValueException by InvalidArgumentException in Guess
Discussion
----------
[Form] replaced UnexpectedValueException by InvalidArgumentException in Guess
BC break: yes
this is a better fit because the error is a logic exception (that can be detected at compile time, i.e. when writing the code) instead of a runtime exception
---------------------------------------------------------------------------
by bschussek at 2012-08-29T10:51:54Z
👍
Commits
-------
0186731 [Form] removed hasParent from FormInterface and deprecated its use
Discussion
----------
[Form] removed hasParent from FormInterface and deprecated its use
There are already 2 alternatives with getParent() and isRoot(), so a third one with similar semantics is confusing and unneeded.
---------------------------------------------------------------------------
by bschussek at 2012-08-29T11:11:11Z
👍
Commits
-------
492c990 [Form] optimized PropertyPathMapper to invoke the expensive property path less often
47a8bbd [Form] optimized the binding of child forms and calculation of extra data
8d45539 [Form] refactor Form::bind to save 7 assignments
Discussion
----------
[Form] refactor Form::bind to save 7 assignments and a complete loop
---------------------------------------------------------------------------
by stof at 2012-08-24T23:45:18Z
the new code is not equivalent. See travis for the proof.
---------------------------------------------------------------------------
by Tobion at 2012-08-25T01:50:41Z
@stof fixed, I had to reduce the refactoring a little
---------------------------------------------------------------------------
by bschussek at 2012-08-29T11:05:52Z
👍
Commits
-------
d5eb4f7 [Form] fix phpdoc of Form::hasErrors
5cb8264 [Form] deprecated Form::hasErrors that isn't part of the Interface
Discussion
----------
[Form] deprecated Form::hasErrors that isn't part of the Interface
This method is not part of FormInterface, so I deprecated it as it cannot be used reliably. This is consistent with other hassers that were deprecated like `hasChildren` where one should use `count` instead.
---------------------------------------------------------------------------
by stof at 2012-08-26T19:11:19Z
You should deprecate it, not remove it
---------------------------------------------------------------------------
by Tobion at 2012-08-26T19:17:35Z
oh right. I thought it was added in 2.1 and thus can be removed but it's also in 2.0.
Done.
---------------------------------------------------------------------------
by bschussek at 2012-08-29T11:00:32Z
👍
* 2.0:
updated VERSION for 2.0.17
updated CHANGELOG for 2.0.17
updated vendors for 2.0.17
fixed XML decoding attack vector through external entities
prevents injection of malicious doc types
disabled network access when loading XML documents
refined previous commit
prevents injection of malicious doc types
standardized the way we handle XML errors
Redirects are now absolute
Conflicts:
CHANGELOG-2.0.md
src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
src/Symfony/Component/DomCrawler/Crawler.php
src/Symfony/Component/HttpKernel/Kernel.php
tests/Symfony/Tests/Component/DependencyInjection/Loader/XmlFileLoaderTest.php
tests/Symfony/Tests/Component/Routing/Loader/XmlFileLoaderTest.php
tests/Symfony/Tests/Component/Serializer/Encoder/XmlEncoderTest.php
tests/Symfony/Tests/Component/Translation/Loader/XliffFileLoaderTest.php
tests/Symfony/Tests/Component/Validator/Mapping/Loader/XmlFileLoaderTest.php
vendors.php
Commits
-------
f1970fa dump test
Discussion
----------
Symfony\Component\ClassLoader\ClassMapGenerator dump test
Hi,
100 percent test coverage for ClassMapGenerator :)
Best regards,
Michal
---------------------------------------------------------------------------
by eventhorizonpl at 2012-08-24T07:47:24Z
Fixed, thanks for the review!
Commits
-------
83dc966 [Form] Fixed some PHPDoc
596bbb1 [Form] fixed FormConfigBuilder to use PropertyPathInterface
a523823 [Form] fixed and added phpDoc
Discussion
----------
[Form] fixed and added phpDoc
[ci skip]
---------------------------------------------------------------------------
by sstok at 2012-08-26T08:11:01Z
Some descriptions don''t seem to be properly aligned, use the CS-fixer.
---------------------------------------------------------------------------
by Tobion at 2012-08-26T17:02:25Z
@sstok This is more about manual fixes concerning forgotten exceptions or wrong data type. The cs fixer gives many false positives and can be applied later.
Commits
-------
9e5d5a4 [Form] fix static method call
Discussion
----------
[Form] fix static method call
`allowDataWalking` was called statically, but wasnt defined as such.
Commits
-------
933e821 Add minimum-stability (dev) in each component
Discussion
----------
Add minimum-stability (dev) in each component
This fixes the ability to run the test suite in each component if a `composer install` is needed.
---------------------------------------------------------------------------
by stof at 2012-08-22T13:57:14Z
If you really want to run the testsuite standalone, some dev requirements are missing (SecurityBundle needs the FrameworkBundle for its functional tests for instance). If you have some time to check the missing dev requirement, it would be great.
Anyway, 👍 for this
---------------------------------------------------------------------------
by willdurand at 2012-08-22T13:59:15Z
Yes I already did that once. I'll try to fix more components later.
On Wed, Aug 22, 2012 at 3:57 PM, Christophe Coevoet <
notifications@github.com> wrote:
> If you really want to run the testsuite standalone, some dev requirements
> are missing (SecurityBundle needs the FrameworkBundle for its functional
> tests for instance). If you have some time to check the missing dev
> requirement, it would be great.
> Anyway, [image: 👍] for this
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/symfony/symfony/pull/5318#issuecomment-7934886>.
>
>
---------------------------------------------------------------------------
by stof at 2012-08-22T14:02:23Z
Well, I think most components should be good now (as some work has been done on them). But the bridges and bundles may need some work (bundles were not having any dev requirements until yesterday when @guilhermeblanco added some on FrameworkBundle)
---------------------------------------------------------------------------
by pborreli at 2012-08-22T14:14:00Z
what about having for each READ-ONLY repo his own .travis.yml and travisci hook activated ?
---------------------------------------------------------------------------
by fabpot at 2012-08-22T14:30:13Z
please, don't add more travis files. The main already tests everything, and that's all we need.
---------------------------------------------------------------------------
by stof at 2012-08-22T14:33:46Z
@pborreli tests should not be different for subtree split repos as the code is the same and the tests are the same (except that more tests could be skipped because of missing deps).
Note that for the bundles, it is likely to be different currently as I think some skip tests are missing (just like dev requirements are). But fixing this does not require enablign travis.
---------------------------------------------------------------------------
by pborreli at 2012-08-22T14:42:30Z
ok, i was just thinking about a way to be sure each component is usable individually but yeah that would require to relaunch each tests and add a bunch of travis files + hook
---------------------------------------------------------------------------
by hason at 2012-08-24T13:12:04Z
@stof, @eriksencosta, @fabpot: Tests are different for Locale component, see #5235
---------------------------------------------------------------------------
by stof at 2012-08-24T13:35:07Z
@hason no. You also need to do it when running the tests of the Locale component as part of the full run.
Commits
-------
8c74b55 getNamespaces test
Discussion
----------
100pc symfony component class loader
Hi,
I added some tests for Symfony\Component\ClassLoader\UniversalClassLoader and Symfony\Component\ClassLoader\ClassLoader.
Best regards,
Michal
---------------------------------------------------------------------------
by eventhorizonpl at 2012-08-23T20:05:02Z
Fixed. Thanks for the review!
---------------------------------------------------------------------------
by pborreli at 2012-08-24T05:11:00Z
👍
---------------------------------------------------------------------------
by fabpot at 2012-08-24T05:46:36Z
Can you squash your commits before I merge the PR? Thanks.
Commits
-------
1ff081d added tests for ValidatorBuilder fluent interface
fec11ae updated docblocks for ValidatorBuilderInterface
b5aaf53 added fluent interface to validatorbuilder
Discussion
----------
[Validator] Added missing fluent interface to ValidatorBuilder
The new ValidatorBuilder class seems to be intended to have a fluent interface, reasoning:
- Static Validation::createValidatorBuilder() method exists
- Consistency with other builders in the framework
- Component README actually uses fluent interface for examples.
This was not implemented though. This PR adds the fluent interface.
BC Break: No
Symfony2 Tests Pass: Yes
---------------------------------------------------------------------------
by henrikbjorn at 2012-08-23T09:47:35Z
Could you add a test for this? :)
---------------------------------------------------------------------------
by bschussek at 2012-08-23T12:04:12Z
Great, thanks! 👍
---------------------------------------------------------------------------
by meandmymonkey at 2012-08-23T12:30:40Z
@henrikbjorn Yes, will do.
---------------------------------------------------------------------------
by meandmymonkey at 2012-08-25T16:21:37Z
@henrikbjorn done
Commits
-------
a38232a [Form] Fixed: FormTypeInterface::getParent() supports returning FormTypeInterface instances again
Discussion
----------
[Form] Fixed: FormTypeInterface::getParent() supports returning FormTypeInterface instances again
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #5221
Todo: -
---------------------------------------------------------------------------
by stof at 2012-08-22T14:14:55Z
the return value of the getParent method should be updated in the phpdoc of the FormTypeInterface to mention the FormTypeInterface .And the description of the method should be updated to explain than returning an instance is discouraged as it implies a performance penalty and does not support using type extensions (if the comment in the factory also applies to the unregistered parent)
---------------------------------------------------------------------------
by henrikbjorn at 2012-08-22T14:22:00Z
Wasn't TypeExtensions supported before? This means that Csrf will not be applied?
---------------------------------------------------------------------------
by stof at 2012-08-22T14:23:50Z
@henrikbjorn the csrf extension is targeting the FormType, which is registered in the form registry. What is not supported is having a type extension targeting an unregistered type
---------------------------------------------------------------------------
by bschussek at 2012-08-22T14:39:53Z
@stof Exactly. I find it a bit unlogical to register an extension for something that is not registered.
---------------------------------------------------------------------------
by henrikbjorn at 2012-08-22T14:39:57Z
Okay. That wasn't what i got from reading the comment :)
---------------------------------------------------------------------------
by bschussek at 2012-08-22T14:44:27Z
@stof Updated.
Commits
-------
bca68ca Fixed a typo
Discussion
----------
Fixed a typo
The CSRF error message won't be translated due to this typo even if the translator is enabled.
Commits
-------
47b8538 [Filesystem] missing realpath breaking FilesystemTest class on Windows (one line of code change)
Discussion
----------
[Filesystem] missing readlink breaking FilesystemTest class on Windows
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #5233
Todo:
License of the code: MIT
Documentation PR:
fix for windows plateform
$file == 'C:\Users\USERNA~1\...' before touch
$file == 'C:\Users\Username\... after the touch and readlink so it can pass following assertEquals
---------------------------------------------------------------------------
by bdmu at 2012-08-13T05:35:07Z
Hello,
Another solution may be (need to test it) to add
$this->workspace = realpath($this->workspace);
line 36, after the
mkdir($this->workspace, 0777, true);
in the setup method
Regards,
Christophe
Commits
-------
0ad00f8 [EventDispatcher] Adding IteratorAggregate to GenericEvent
Discussion
----------
[EventDispatcher] Adding IteratorAggregate to GenericEvent
---------------------------------------------------------------------------
by drak at 2012-08-16T07:43:29Z
What is the use case for this that it should be part of the Generic event?
---------------------------------------------------------------------------
by mtdowling at 2012-08-16T17:12:28Z
This allows for the GenericEvent to be even more generic. Now listeners don't need to know an exact key from the arguments, but rather can iterate over the arguments to find what they are looking for. This makes the GenericEvent more like an array.
---------------------------------------------------------------------------
by mtdowling at 2012-08-17T19:31:04Z
How would this be a nasty break? It's just giving the GenericEvent more capabilities with IteratorAggregate.
This is a completely separate PR from the one that flipped the constructor args.
---------------------------------------------------------------------------
by schmittjoh at 2012-08-17T19:34:47Z
Why are you not just doing ``foreach ($event->getArguments() as $arg) { /** ... */ }``?
If you just have ``foreach ($event)``, to me at least it would not be so clear what we are actually iterating over.
---------------------------------------------------------------------------
by mtdowling at 2012-08-17T19:39:23Z
This class already has ArrayAccess. If you're already using this class like an array, then I think you should expect to be able to iterate it like an array. I'm just finishing that concept off by implementing IteratorAggregate.
---------------------------------------------------------------------------
by schmittjoh at 2012-08-17T19:47:43Z
Indeed, if we already have ArrayAccess which we probably don't want to remove again, then that seems reasonable.
Commits
-------
ccb6dad [HttpFoundation] fixed undefined offset for assoc arrays in HeaderBag
Discussion
----------
[HttpFoundation] fixed undefined offset for assoc arrays in HeaderBag
`get` is assuming the headers are zero-indexed. So something like this would otherwise create a php warning.
```
$bag->set('foo', array('bad-assoc-index' => 'value'));
$this->assertSame('value', $bag->get('foo'));
```
Commits
-------
bdaa877 [HttpFoundation] fix#5271 (duplicated header in JsonResponse)
Discussion
----------
fix JsonResponse: duplicate header
fix#5271
---------------------------------------------------------------------------
by Tobion at 2012-08-16T16:50:04Z
Will look into the failing test later.
---------------------------------------------------------------------------
by Tobion at 2012-08-16T23:55:45Z
Finished.
Commits
-------
ac0fb48 0x02 -> \MongoBinData::BYTE_ARRAY
70a64bd MongoBinData constructor now require "type" parameter
Discussion
----------
MongoBinData constructor now requires "type" parameter
From mongo driver version 1.2.11 E_DEPRECATED is emitted when the 2nd argument (type) is not used for MongoBinData constructor. At the moment the default value is 0x02 ("byte array").
See: http://php.net/manual/en/mongobindata.construct.php
From mongo driver version 1.2.11 E_DEPRECATED is emitted when the 2nd argument (type) is not used for MongoBinData constructor. At the moment the default value is 0x02 ("byte array").
See: http://php.net/manual/en/mongobindata.construct.php
Commits
-------
57694aa Removed MySQL-exclusive usage of unsigned integer from table creation
Discussion
----------
[Security] Removed MySQL-exclusive usage of unsigned integer from table creation
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #5108
This patch removes the setting of unsigned integer fields which are MySQL-only.
---------------------------------------------------------------------------
by fabpot at 2012-08-13T12:25:23Z
@beberlei Does it look good?
---------------------------------------------------------------------------
by travisbot at 2012-08-13T12:30:27Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2107738) (merged 57694aaa into 31536c36).
---------------------------------------------------------------------------
by lsmith77 at 2012-08-13T20:04:20Z
indeed afaik unsigned isnt part of the sql standard. in other RDBMS you need to use a constraint if you want to prevent negative values and of course there is no way to then also get the additional values per bit that you get with MySQL, but so it goes. that being said .. i dont know how Doctrine schema handles unsigned for other RDBMS .. ie if it just emulates them by trying to double the size with a constraint.
---------------------------------------------------------------------------
by beberlei at 2012-08-13T20:10:44Z
Looks good.
Doctrine ignores the 'unsigned' => true for all others vendors.
Commits
-------
1a4a4ee [DependencyInjection] Fixed a frozen constructor of a container with no parameters
2a124bc [DependencyInjection] Added a test for a frozen constructor of a container with no parameters
Discussion
----------
[DependencyInjection] Fix PHP Dumper for a constructor of a frozen container with no parameters
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets:
Todo: -
License of the code: MIT
---------------------------------------------------------------------------
by travisbot at 2012-08-06T16:51:20Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2049206) (merged 1a4a4ee9 into 3d32a0bc).
Commits
-------
039264d [Console] Fixed tests about message exception when command is not available
a4d2d31 [Console] Added tests for message exception when command is not available
Discussion
----------
[Console] Fixed message exception when command is not avaible
Bug fix: yes
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
License of the code: MIT
Fixed singular / plural
`Did you mean this?` VS `Did you mean one of these`
---------------------------------------------------------------------------
by travisbot at 2012-08-07T14:40:55Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2057647) (merged 039264d2 into b91a4a83).
---------------------------------------------------------------------------
by lyrixx at 2012-08-07T15:11:47Z
@travisbot You failed, not me !
Commits
-------
5ad75c7 updated method name in a comment
Discussion
----------
updated method name in a comment
---------------------------------------------------------------------------
by travisbot at 2012-08-07T14:35:39Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2057604) (merged 5ad75c71 into b91a4a83).
Commits
-------
fa93324 [HttpFoundation] Fixed test under Windows environment
Discussion
----------
[HttpFoundation] Fixed test under Windows environment
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #5226
Todo: -
License of the code: MIT
Documentation PR: -
---------------------------------------------------------------------------
by travisbot at 2012-08-10T09:54:20Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2084750) (merged fa933247 into dd9afddf).
Commits
-------
22be499 [HttpFoundation][Session] Cleaned up constructor invokation
Discussion
----------
[HttpFoundation][Session] Cleaned up constructor invokation
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: ~
License of the code: MIT
Documentation PR: ~
This simply cleans up a mismatch between the class instantiation and the actual class constructor. Evidently something left over from the various iterations.
---------------------------------------------------------------------------
by travisbot at 2012-08-07T19:16:14Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2060028) (merged 22be4993 into 7dbadbf2).
---------------------------------------------------------------------------
by drak at 2012-08-09T08:25:43Z
ping @fabpot
Commits
-------
3e1a1ab Force the value of Dumper::setIndentation($num) to be of type integer.
5be7237 Added Yaml\Dumper::setIndentation() method to allow a custom indentation level of nested nodes.
Discussion
----------
[Yaml] Allow custom indentation level for nested nodes
YAML does not specify an absolute indentation level, but a consistent indentation of nested nodes only: http://yaml.org/spec/current.html#indentation%20space/
Projects that are generally using 2 spaces for indentation should be able to retain consistency with their coding standards by supplying a custom value for the new $indent parameter for Yaml::dump() and Dumper::dump().
The new parameter is a backwards-compatible API addition and defaults to the previous default of 4 (which was changed from 2 via PR #2242 only recently).
The old $indent parameter is renamed to $level, and remains to be used internally only.
---------------------------------------------------------------------------
by travisbot at 2012-08-03T00:24:22Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2024289) (merged 56331202 into b1618d21).
---------------------------------------------------------------------------
by travisbot at 2012-08-03T00:29:02Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2024315) (merged eeae28a4 into b1618d21).
---------------------------------------------------------------------------
by travisbot at 2012-08-03T02:41:42Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2024905) (merged 9a82c438 into b1618d21).
---------------------------------------------------------------------------
by fabpot at 2012-08-03T07:53:50Z
This is indeed a BC break as the method signature changed in a non-BC way.
---------------------------------------------------------------------------
by fabpot at 2012-08-03T08:35:51Z
I think I would prefer to have a static method to be able to change the number of spaces to use on a global basis. It makes more sense and would prevent the BC break. What do you think?
---------------------------------------------------------------------------
by sun at 2012-08-03T13:23:23Z
Thanks for your feedback! — Took some time to think through the static proposal. Here's what I think:
1. The call from DependencyInjection\Dumper\YamlDumper truly was a unexpected/nasty surprise for me. That is, because it passes a parameter to Yaml\Dumper that is documented for internal use only, and the surrounding YamlDumper code generally hard-codes lots of assumptions of where exactly the nodes/collections will be output in the dumped YAML structure. (I think that code can be simplified, cleaned up, and made faster at the same time (hence the todo), but indeed, that probably belongs into a separate issue.)
1. The essential problem with a static is closely bound to that though; a static property value for the indentation would "stick" and thus hi-jack the DependencyInjection\Dumper\YamlDumper, as it hard-codes and thus expects 4 spaces (contrary to any custom indentation).
1. Most code uses Yaml\Yaml as utility/helper service directly, so there'd be no clean way to prime the static with a custom value, aside from subclassing the entire thing - in which case this entire issue would sorta become moot, because at the point you're subclassing, you can as well go the extra mile and replace the entire Yaml\Dumper::dump() to implant the custom indentation level...
1. Another option would be to use a non-static Yaml\Dumper::$indent property, supplied through the constructor; i.e.:
public function __construct($indent = 4)
...or alternatively, ::setIndentation(). Essentially requiring people to use and instantiate Yaml\Dumper directly, if they want to use a custom indentation.
1. Though in the end, I don't want to sound pedantic, but I *do* wonder a bit about the exact extent of the `@api` tags, as well as `@param`s that are explicitly documented as "internal use only"... :) That is, because only Yaml\Yaml is tagged with `@api`, but nothing in Yaml\Dumper. The same sorta applies to DependencyInjection\Dumper\YamlDumper::dump(), which is tagged with `@api`, but the :.addService() method being adjusted accordingly here is not. So essentially, when taking those tags (plus the param's description) seriously and in a nitpicky way, then there is no BC break, since no one should rely on their exact implementation... ;) (I perfectly realize that this is a long shot :))
So... depending on the final stance on the last point, I'd either move forward with the current proposal in this PR. Otherwise, I'd suggest the non-static property on Yaml\Dumper - in which case we'd likely try to swap out the static Symfony\Component\Yaml\Yaml helper with a Drupal\Component\Yaml\Yaml in order to always instantiate the dumper with the custom indentation. What do you think?
---------------------------------------------------------------------------
by sun at 2012-08-04T22:57:21Z
Alright. While I believe I made some good points in my last comment, I've taken the fully backwards-compatible path:
- added the new $indent parameter only to Yaml::dump()
- added a new Dumper::$intendation property and Dumper::setIndentation() method, to control the indentation level within the scope of a single Dumper instance only.
Do you think this is acceptible? :)
---------------------------------------------------------------------------
by travisbot at 2012-08-05T06:16:22Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2039120) (merged 5be7237b into c99f9d29).
---------------------------------------------------------------------------
by travisbot at 2012-08-07T07:51:04Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/2055164) (merged 3e1a1abd into c99f9d29).
---------------------------------------------------------------------------
by sun at 2012-08-07T07:53:18Z
Only one environment failed, and the [test failure](http://travis-ci.org/#!/symfony/symfony/jobs/2055165/L203) seems unrelated to this PR.
Commits
-------
c2207da [Console] Removed unused phpunit annotation.
c0c61da [Console] Added current style appliance for all styled text.
696a653 [Console] Removed text transformation for empty styles.
c7e324a [Console] Added test for non style tag formatting.
Discussion
----------
[Console] Non style tags dont disturb formatting anymore
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes issue #5196.
---------------------------------------------------------------------------
by travisbot at 2012-08-06T19:35:20Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2050455) (merged c0c61da0 into 842b599c).
---------------------------------------------------------------------------
by travisbot at 2012-08-07T07:47:31Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2055153) (merged c2207da6 into 842b599c).
Commits
-------
b3cf36a [Config] Missing type argument passed to loader.
Discussion
----------
[Config] Missing type argument passed to loader.
In FileLoader the $type is not passed to the child loader.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
---------------------------------------------------------------------------
by travisbot at 2012-08-06T14:22:30Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2047982) (merged b3cf36af into 89dce2df).
YAML does not specify an absolute indentation level, but a consistent indentation of nested nodes only: http://www.yaml.org/spec/1.2/spec.html#space/indentation/
Projects that are generally using 2 spaces for indentation should be able to retain consistency with their coding standards by supplying a custom value for the new $indent parameter added to Yaml::dump(), or the new Dumper::setIndentation() method.
The new parameter is a backwards-compatible API addition and defaults to the previous default of 4 (which was changed from 2 via PR #2242 only recently).
Commits
-------
aa890ae validateOptionTypes checking existence of key rather than value
c6a9638 OptionsResolver#validateOptionTypes should check if option value exists before checking its type; added corresponding test
Discussion
----------
[OptionsResolver] validateOptionTypes raises error for optional, nonexistant option values
The [`OptionsResolver#validateOptionTypes`](https://github.com/symfony/OptionsResolver/blob/master/OptionsResolver.php#L315) method should check if a given option value exists before trying to test its type:
```php
<?php
// Two optional 'one' and 'two' options that, if they exist, must be a string and int respectively
$resolver = new OptionsResolver();
$resolver->setOptional(array('one', 'two'))->setAllowedTypes(array('one' => 'string', 'two' => 'int'));
// Correctly fails as wrong type
$resolver->resolve(array('one' => 1, 'two' => 'alpha'));
// Correctly succeeds
$resolver->resolve(array('one' => 'alpha', 'two' => 1));
// Raises error "Undefined index: two", see OptionsResolver.php line 315
$resolver->resolve(array('one' => 'alpha'));
```
---------------------------------------------------------------------------
by travisbot at 2012-08-01T01:46:50Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2006049) (merged c6a9638a into b1618d21).
---------------------------------------------------------------------------
by ericclemmons at 2012-08-03T05:15:27Z
I hope this makes it in for the 2.1 release. We have several types attempting to take advantage of the `OptionsResolver` component and want to help resolve this.
---------------------------------------------------------------------------
by stof at 2012-08-03T07:30:44Z
👍
---------------------------------------------------------------------------
by travisbot at 2012-08-03T17:25:12Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2030053) (merged aa890aed into b1618d21).
Commits
-------
d1aae0b [Form] Fix UnexpectedTypeException from HttpFoundation extension
Discussion
----------
[Form] Fix UnexpectedTypeException from HttpFoundation extension
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Fixes exception thrown by the `AbstractExtension` because the wrong method has been implemented.
```
Uncaught exception 'Symfony\\Component\\Form\\Exception\\UnexpectedTypeException' with message 'Expected argument of type "Symfony\\Component\\Form\\FormTypeInterface", "Symfony\\Component\\Form\\Extension\\HttpFoundation\\Type\\FormTypeHttpFoundationExtension" given' in vendor/symfony/form/Symfony/Component/Form/AbstractExtension.php:153
```
---------------------------------------------------------------------------
by stof at 2012-08-03T12:36:55Z
👍
* 2.0:
Fixes incorrect class used in src/Symfony/Bundle/FrameworkBundle/Console/Application.php
[FrameworkBundle] added test for fix broken command registration
corrected phpdoc
Commits
-------
22cb817 Caching variables for the PHP templating engine
Discussion
----------
[Templating] PHP templating engine speed-ups
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/dlsniper/symfony.png?branch=php-engine-escape-cache)](http://travis-ci.org/dlsniper/symfony)
Fixes the following tickets: ~
Todo: ~
License of the code: MIT
Documentation PR: ~
This PR should improve the speed for rendering the form present here: https://github.com/dlsniper/symfony-standard . On my computer, Ubuntu 12.04 Apache 2.2.22 + mod_php 5.3.10 default packages from Ubuntu on a core i7 I get about 30-40ms improvement / request with the first commit and with the second one I get a further smaller boost and also a small memory usage decrease.
---------------------------------------------------------------------------
by dlsniper at 2012-07-31T06:18:54Z
ping @bschussek This should help a bit more on the effort for optimizing the example provided for the Forms component.
If there's another example of complex form(s) let me know so that I can have a look on them as well. Thanks!
---------------------------------------------------------------------------
by travisbot at 2012-07-31T19:55:03Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2003907) (merged 240152b9 into a172a812).
---------------------------------------------------------------------------
by dlsniper at 2012-08-02T07:41:03Z
@fabpot what do you think about this? or anyone else for that matter?
---------------------------------------------------------------------------
by travisbot at 2012-08-02T12:55:54Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2018613) (merged 5e773e79 into a172a812).
---------------------------------------------------------------------------
by fabpot at 2012-08-03T07:42:31Z
Can you squash your commits?
---------------------------------------------------------------------------
by dlsniper at 2012-08-03T08:32:05Z
@fabpot Done
---------------------------------------------------------------------------
by travisbot at 2012-08-03T08:40:46Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2026559) (merged 22cb8173 into 6f32078b).
OptionsResolver#validateOptionsCompleteness would already have thrown exception if the option were required, so this should only affect something explicitly marked as optional
Commits
-------
b982883 [Form] Moved FormHelper back to FrameworkBundle
cb62d05 [Form] [Validator] Fixed issues mentioned in the PR
2185ca8 [Validator] Added entry point "Validation" for more convenient usage outside of Symfony2
ed87361 [Form] Moved FormHelper creation to TemplatingExtension
87ccb6a [Form] Added entry point "Forms" for more convenient usage outside of Symfony
Discussion
----------
[Form] [Validator] Added more convenient entry points for stand-alone usage
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This PR greatly simplifies the usage of the Form and Validator component when used outside of Symfony2. Check out the below code to get an idea about the simplified usage:
```php
<?php
use Symfony\Component\Validator\Validation;
use Symfony\Component\Validator\Mapping\Cache\ApcCache;
use Symfony\Component\Form\Forms;
use Symfony\Component\Form\Extension\HttpFoundation\HttpFoundationExtension;
use Symfony\Component\Form\Extension\Csrf\CsrfExtension;
use Symfony\Component\Form\Extension\Csrf\CsrfProvider\SessionCsrfProvider;
use Symfony\Component\Form\Extension\Templating\TemplatingExtension;
use Symfony\Component\Form\Extension\Validator\ValidatorExtension;
use Symfony\Component\HttpFoundation\Session;
use Symfony\Component\Templating\PhpEngine;
$session = new Session();
$secret = 'V8a5Z97e...';
$csrfProvider = new SessionCsrfProvider($session, $secret);
$engine = new PhpEngine(/* ... snap ... */);
$validator = Validation::createValidator();
// or
$validator = Validation::createValidatorBuilder()
->addXmlMapping('path/to/mapping.xml')
->addYamlMapping('path/to/mapping.yml')
->addMethodMapping('loadValidatorMetadata')
->enableAnnotationMapping()
->setMetadataCache(new ApcCache())
->getValidator();
$formFactory = Forms::createFormFactory();
// or
$formFactory = Forms::createFormFactoryBuilder()
// custom types, if you're too lazy to create an extension :)
->addType(new PersonType())
->addType(new PhoneNumberType())
->addTypeExtension(new FormTypeHelpTextExtension())
// desired extensions (CoreExtension is loaded by default)
->addExtension(new HttpFoundationExtension())
->addExtension(new CsrfExtension($csrfProvider))
->addExtension(new TemplatingExtension($engine, $csrfProvider, array(
'FormBundle:Form'
))
->addExtension(new ValidatorExtension($validator))
->getFormFactory();
$form = $formFactory->createBuilder()
->add('firstName', 'text')
->add('lastName', 'text')
->add('age', 'integer')
->add('gender', 'choice', array(
'choices' => array('m' => 'Male', 'f' => 'Female'),
))
->getForm();
if (isset($_POST[$form->getName()])) {
$form->bind($_POST[$form->getName()]);
if ($form->isValid()) {
// do stuff
}
}
return $engine->render('AcmeHelloBundle:Hello:index.html.php', array(
'form' => $form->createView(),
));
```
---------------------------------------------------------------------------
by bschussek at 2012-07-30T10:08:42Z
I should maybe add a comment about the benefits of this change, in case they are not self-explanatory:
* class construction with default configuration is now a one-liner
* userland code is decoupled from core implementations → userland code doesn't break if we change constructor signatures
* easier to understand, since many core classes are now created internally
* easy to discover the possible settings → just look at (FormFactory|Validator)BuilderInterface
* usage of custom interface implementations is supported, just like before
---------------------------------------------------------------------------
by fabpot at 2012-07-31T08:18:53Z
The new syntax is great.
I have one comment though about this PR about support of PHP as a templating system (support for Twig is provided by the bridge and it was already easy to configure Twig as a templating system for forms -- see Silex for instance).
The `FormHelper` has been moved into the Form component. This helper is only useful when using the PHP templating system (which is not what we recommend people to use), but the default templates are still in the Framework bundle. So using the Form component as standalone with PHP as a templating system still requires to install the bundle to get access to the default templates. Am I missing something? Do we want to move the PHP templates to the Form component too?
---------------------------------------------------------------------------
by stof at 2012-07-31T08:28:28Z
@fabpot it is even worse than that: the FormHelper currently uses the theme by using ``$theme . ':' . $block . '.html.php`` IIRC. This is not compatible with the default template name parser of the component expecting a path. And the FrameworkBundle template name parser does not support accessing a template outside a bundle AFAIK.
So moving the templating to the component would require some refactoring in the FormHelper and the template name parser. However, I think it is worth it. Some people complained that using the form rendering (outside the full-stack framework) was requiring either setting up Twig with the bridge, or adding FrameworkBundle in the project (which means including most of the code of the full-stack framework). Having the Templating rendering in the standalone component could be a great idea
---------------------------------------------------------------------------
by fabpot at 2012-07-31T08:42:53Z
But then, I don't want to promote the Templating component or the PHP templating system. Twig is always a better alternative and this should be what people use most of the time, PHP being the rare exception.
Anyway, we are too close from the first 2.1 RC, so any big refactoring will have to wait for 2.2.
---------------------------------------------------------------------------
by stof at 2012-07-31T09:02:10Z
then maybe we should keep the FormHelper in FrameworkBundle for now as it is tied to the FrameworkBundle template name parser anyway currently.
---------------------------------------------------------------------------
by bschussek at 2012-07-31T14:22:35Z
> it it is even worse than that: the FormHelper currently uses the theme by using ``$theme . ':' . $block . '.html.php`` IIRC. This is not compatible with the default template name parser of the component expecting a path.
This is why the templates are still in FrameworkBundle. I think they should be moved too, but then we have to change
* the default theme to an absolute file path
* the FrameworkBundle name parser to accept absolute paths
I think this can wait until 2.2. Baby steps.
> I don't want to promote the Templating component or the PHP templating system.
We can both promote Twig while making Templating as easy to use as possible. If people want to use Templating, they probably have a reason. We don't have to make their lives more painful than necessary.
Btw: Templating is a *lot* faster for rendering forms than Twig. On Denis' form, Templating takes 1.15 seconds while Twig takes 2.
About moving the helpers, we have two choices:
* Move each helper to the respective component. This would not require new releases of the Templating component when we add more helpers in other component.
* Move all helpers to Templating. This does not make that much sense for Form, as then Form has support for Templating (TemplatingRendererEngine) and Templating has support for Form (FormHelper), which is a bit weird. I personally prefer a stacked architecture, where Templating is at the bottom and Form-agnostic, and Form (or any other component) builds upon that.
I'm fine with both approaches. I'll move FormHelper back to FrameworkBundle, and we can decide for a direction in 2.2.
---------------------------------------------------------------------------
by bschussek at 2012-07-31T14:36:30Z
Done.
Commits
-------
03bbaaf [Routing] Add an interface for configuring strict_parameters
Discussion
----------
[RFC][Routing] Add an interface for configuring strict_parameters
This is a proposal to fix#4697 (related to #4592).
The main point left to discuss was the name of the interface, which is now `LenientInterface`. We could change the name to anything else is someone has a better idea.
@stof @Tobion what do you think ?
---------------------------------------------------------------------------
by stof at 2012-07-30T16:34:20Z
@vicb I already said I had no idea to name it, and it has not changed. :)
So let's wait for other people to see if they have a better idea
---------------------------------------------------------------------------
by breerly at 2012-07-30T16:38:38Z
Maybe `PermissibleInterface` or `PermissiveInterface`.
---------------------------------------------------------------------------
by Partugal at 2012-07-30T17:00:09Z
`StrictUrlGeneratorInterface`, `StrictParametersInterface` or `StrictInterface`
---------------------------------------------------------------------------
by pborreli at 2012-07-30T17:04:46Z
👍 for `PermissiveInterface`
---------------------------------------------------------------------------
by stof at 2012-07-30T17:07:59Z
yes, because the Router currently can only use this interface to set it to ``not-strict``. It assumes that the url generator is already strict by default (which is probably a bad assumption btw as the base class for the generated generator can be changed)
---------------------------------------------------------------------------
by pborreli at 2012-07-30T17:09:33Z
@stof thx, got it
---------------------------------------------------------------------------
by Partugal at 2012-07-30T17:10:03Z
this interface realize setting Strict by setStrictParameters, and get by getStrictParameters, and imho named it by `Strictable` is more logic
---------------------------------------------------------------------------
by pborreli at 2012-07-30T17:11:07Z
@Partugal let's try to find an english term :)
---------------------------------------------------------------------------
by Partugal at 2012-07-30T17:11:31Z
)
---------------------------------------------------------------------------
by breerly at 2012-07-30T17:15:23Z
@Partugal I like using "able" in interface names because it describes a behavior instead of a noun. This type of naming makes following the Interface Segregation Principle easy to follow. Good work.
---------------------------------------------------------------------------
by vicb at 2012-07-30T18:24:26Z
As explained by @stof I did not consider `StrictInterface` because as of now the interface is used to disabled the strict bevahior (which is enabled by default).
I am not satisfied with `PermissiveInterface` / `LenientInterface` because implementing this interface does not mean that the generator will be permissive but only that the behavior is configurable - yes I did consider `Configurable` but the term is a too vague.
---------------------------------------------------------------------------
by breerly at 2012-07-30T18:35:45Z
I see. Perhaps ```StrictConfigurableInterface``` would do the trick.
---------------------------------------------------------------------------
by Tobion at 2012-07-30T21:02:21Z
I think renaming strict_parameters to `strict_requirements` is the way to go because it determines how requirements are handled when generating a URL. Also we should allow another option:
strict_requirements = true: throw exception for mismatching requirements
strict_requirements = null: return null as URL for mismatching requirements and log it.
strict_requirements = false: return the URL with the given parameters without checking the requirements and don't log it.
(Maybe use constants for these).
The Interface I would then call `ConfigurableRequirementsInterface` or `RequirementsHandlingInterface`.
---------------------------------------------------------------------------
by vicb at 2012-07-31T07:23:24Z
Thanks all for the feeback, this is what is now implemented:
- A `ConfigurableRequirementsInterface` that should be implemented by generators that can be configure not to throw an exception when the parameters do not match the requirements,
- The interface has two methods: `setStrictRequirements()` and `isStrictRequirements()`,
- `setStrictRequirements()` always gets called to configure the generator (whatever the option value is)
Note: The Router option name has not changed (i.e. `strict_parameters`)
Does that fit everyone ?
---------------------------------------------------------------------------
by vicb at 2012-07-31T07:39:22Z
So the option name is now consistent (`strict_requirements`) with the interface. We should sync the change [in the standard edition](https://github.com/symfony/symfony-standard/blob/master/app/config/config.yml#L11) if we agree to merge this.
---------------------------------------------------------------------------
by fabpot at 2012-07-31T07:51:47Z
@vicb you forgot to rename the property in `UrlGenerator` as @stof mentioned above.
---------------------------------------------------------------------------
by vicb at 2012-07-31T07:59:57Z
@fabpot fixed. If the code is ok, I'll squash the commits and open a PR on symfony-standard
Commits
-------
a47922b [OptionsResolver] Fix Options::has() when the value is null
Discussion
----------
[OptionsResolver] Fix Options::has() when the value is null
`isset()` would have returned `false` when the value is `null`
Commits
-------
03c3712 [Filesystem] Fixed 2 tests throwing error on windows
3689bb8 [Filesystem] Fixed 3 failing tests on windows
Discussion
----------
[Filesystem] Fixed 5 tests on windows
Fixing 3 test expecting wrong folders :
```
-'C:\Users\pascal\AppData\Local\Temp\\1343425847694\file'
+'C:\Users\pascal\AppData\Local\Temp\1343425847694\file'
```
Fixed 2 tests on Windows caused by symlink function throwing error when first argument is not existent :
```
symlink(): Could not fetch file information(error 2)
```
Commits
-------
ae6016c [Finder] Workaround for FilterIterator-FilesystemIterator-rewind issue
Discussion
----------
[Finder] Workaround for the problem with rewind of FilterIterator with inner FilesystemIterator.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4922
Todo: -
License of the code: MIT
Documentation PR: -
---------------------------------------------------------------------------
by stof at 2012-07-20T10:28:05Z
Please add some tests
---------------------------------------------------------------------------
by alebo at 2012-07-24T09:50:36Z
Any feedback yet? The new commit includes tests.
Commits
-------
4d09907 [Serializer] Add a docblock to help type hinting
Discussion
----------
[Serializer] Add a docblock to help type hinting
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:
Commits
-------
9e28593 fixed error on oracle db related to clob data. https://github.com/symfony/symfony/issues/3255
Discussion
----------
fixed error on pdosession storage for oracle db. Related to clob data
Did a change on 2.0 branch to fix this error
https://github.com/symfony/symfony/issues/3255
Tested on mysql and Oracle DB.
I also ran a phpunit test before commit:
phpunit tests/Symfony/Tests/Component/HttpFoundation/SessionStorage/PdoSessionStorageTest.php
PHPUnit 3.6.10 by Sebastian Bergmann.
Time: 0 seconds, Memory: 2.75Mb
OK (3 tests, 5 assertions)
Time: 0 seconds, Memory: 2.75Mb
Commits
-------
d858f7b [OptionsResolver] Optimized previous values of a lazy option not to be evaluated if the second argument is not defined
8a338cb [OptionsResolver] Micro-optimization
e659f0e [OptionsResolver] Improved the performance of normalizers
Discussion
----------
[OptionsResolver] Improved the performance of normalizers
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Normalizers are now stored in the Options instance only once. Previously, normalizers were stored in Options upon resolving, which meant that they were added a lot of time if the same resolver was used for many different options arrays.
This improvement led to an improvement of 30ms on http://advancedform.gpserver.dk/app_dev.php/taxclasses/1
---------------------------------------------------------------------------
by beberlei at 2012-07-26T13:34:23Z
@bschussek do you have the code for this forms somewhere btw?
---------------------------------------------------------------------------
by bschussek at 2012-07-26T13:54:52Z
@beberlei https://github.com/stof/symfony-standard/tree/twig_forms
Normalizers are now stored in the Options instance only once. Previously,
normalizers were stored in Options upon resolving, which meant that
they were added a lot of time if the same resolver was used for many
different options arrays.
This improvement led to an improvement of 30ms on
advancedform.gpserver.dk/app_dev.php/taxclasses/1
Commits
-------
1d6611b [Filesystem] Fixed tests on system without posix
Discussion
----------
[Filesystem] Fixed tests on system without posix
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: master is broken
Fixes the following tickets: None
License of the code: MIT
Commits
-------
fb002d8 [Form] Fixed variable passing from outer to inner blocks of the same FormView instance
Discussion
----------
[Form] Fixed variable passing from outer to inner blocks of the same FormView instance
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #5029
Todo: -
This PR fixes two bugs.
The first bug is described in #5029. The second parameter to the "form_label" function in Twig, if given, always overwrote whatever label was defined previously.
```
{# null would overwrite whatever is currently set #}
form_label(form, null, { ... })
```
The second bug affected passing variables from outer to inner blocks. In the following example, "label_attr" would not be forwarded to the "form_label" function.
```
form_row(form, { "label_attr": { "class": "my_class" }})
```
Both bugs are fixed now.
Commits
-------
0ea3769 Fix not recognized "type" option exception
Discussion
----------
[Form] Fixed not recognized "type" option exception
The exception about not recognized "type" option was raised when "date", "datetime", "time" type was guessed by validator type guesser using the date related constraint.
---------------------------------------------------------------------------
by bschussek at 2012-07-25T11:30:23Z
Thanks! 👍
Commits
-------
686bf6b [Form] Made original data of a form and choices accessible in templates
Discussion
----------
[Form] Made original data of a form and choices accessible in templates
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4171
Todo: -
Now you can access the normalized data of a form in the template:
```
form.vars.data
```
You can also access the original data of a choice, for example the entities in an entity type:
```
choice.data
```
The exception about not recognized "type" option was raised when "date", "datetime", "time" type was guessed by validator type guesser using the date related constraint.
Commits
-------
307d99c [Security] Fixed use_referer option not working properly when login_path is a route name
Discussion
----------
[Security] Fixed use_referer option not working properly when login_path...
... is a route name
When use_referer is set to true and the request comes from the login page,
the user should not be redirected to the login form again (the referer) but
to the default_target_path. The problem arises when our login_path option
is not a path but a route name, as the ```getUriForPath()``` method is not
made to create routes from route names.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/acasademont/symfony.png?branch=master)](http://travis-ci.org/acasademont/symfony)
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -
---------------------------------------------------------------------------
by stloyd at 2012-07-24T16:24:28Z
👍
When use_referer is set to true and the request comes from the login page,
the user should not be redirected to the login form again (the referer) but
to the default_target_path. The problem arises when our login_path option
is not a path but a route name, as the ```getUriForPath()``` method is not
made to create routes from route names.
Commits
-------
dd2aa54 [Form] Disabled manual singulars in PropertyPath until the syntax is finalized
Discussion
----------
[Form] Disabled manual singulars in PropertyPath until the syntax is finalized
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Commits
-------
dc3a680 [Form] Improved FormRenderer API to reduce the size of the function call stack during rendering
Discussion
----------
[Form] Improved FormRenderer API to decrease the function call stack
Bug fix: no
Feature addition: no
Backwards compatibility break: **yes**
Symfony2 tests pass: yes
Fixes the following tickets: #4962, #4973
Todo: -
This PR reduces the function call stack size when rendering by directly calling the methods `renderBlock` and `searchAndRenderBlock` (formerly `renderSection`) and removing the delegating methods `render(Widget|Label|Row|...)`.
It breaks BC in that PHP templates now need to pass the FormView instance to `block` (formerly `renderBlock`). This is necessary, otherwise that function may behave buggy in special circumstances.
Otherwise this PR cleans up API method and parameter names to improve clarity.
Commits
-------
24b764e [Form] Fixed issues mentioned in the PR
9216816 [Form] Turned Twig filters into tests
310f985 [Form] Added a layer of 2.0 BC methods to FormView and updated UPGRADE and CHANGELOG
5984b18 [Form] Precalculated the closure for deciding whether a choice is selected (PHP +30ms, Twig +30ms)
5dc3c39 [Form] Moved the access to templating helpers out of the choice loop for performance reasons (PHP +100ms)
0ef9acb [Form] Moved the method isChoiceSelected() to the ChoiceView class (PHP +150ms)
8b72766 [Form] Tweaked the generation of option tags for performance (PHP +200ms, Twig +50ms)
400c95b [Form] Replace methods in ChoiceView by public properties (PHP +100ms, Twig +400ms)
d072f35 [Form] The properties of FormView are now accessed directly in order to increase performance (PHP +200ms, Twig +150ms)
Discussion
----------
[Form] Made FormView and ChoiceView properties public for performance reasons
Bug fix: no
Feature addition: no
Backwards compatibility break: **yes**
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This PR changes the access to properties of `FormView` and `ChoiceView` objects from getters to direct property accesses. On [my example form](http://advancedform.gpserver.dk/app_dev.php/taxclasses/1) this improves rendering performance for **300ms** with PHP templates and **550ms** with Twig on my local machine.
Unfortunately, this breaks BC both with 2.0 and with the current master in Form Types and PHP templates. Twig templates are not affected by this change.
2.0:
```
$formView->set('my_var', 'foobar');
$formView->get('my_var');
$formView->getChild('childName');
$formView['childName'];
```
master:
```
$formView->setVar('my_var', 'foobar');
$formView->getVar('my_var');
$formView->get('childName');
$formView['childName'];
```
this PR:
```
$formView->vars['my_var'] = 'foobar';
$formView->vars['my_var'];
$formView->children['childName'];
$formView['childName'];
```
Should we add methods to keep BC with 2.0?
The second part of this PR contains improvements to the rendering of choice fields. These gain another **~500ms** for PHP templates and **80ms** for Twig. These improvements are BC, unless you overwrote the block "choice_widget_options" in your form themes which then needs to be adapted.
**Update:**
The PR now includes a BC layer for 2.0.
---------------------------------------------------------------------------
by stof at 2012-07-21T11:37:41Z
@bschussek couldn't we keep the getters and setters for BC even if the rendering accesses the public properties directly ?
---------------------------------------------------------------------------
by bschussek at 2012-07-21T11:52:33Z
@stof A BC layer for 2.0 is now included. People who upgraded to master already unfortunately need to adapt their code.
---------------------------------------------------------------------------
by sstok at 2012-07-21T12:40:57Z
👍
Commits
-------
134cc84 [Security] Fix DocBlock of attemptAuthentication
Discussion
----------
[Security] Fix DocBlock of attemptAuthentication
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: -
Commits
-------
9dc2011 Late static factory method
Discussion
----------
Late static factory method
When using `Symfony\CS\Finder\DefaultFinder::create()`, we lose all `Symfony\CS\Finder\DefaultFinder::__construct()` properties because main `Finder` does not use late static binding.
This commit resolves the issue.
Commits
-------
d4f4038 [Form] Reduced the number of setData() calls by deferring a Form's initialization (+40ms)
Discussion
----------
[Form] Reduced the number of setData() calls
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This PR decreases the number of expensive `setData()` calls on `Form` instances by deferring the form's initialization with default data to the first call to a `get*Data()` method. If `setData()` is called manually before invoking `get*Data()`, the initialization with the default data will not take place.
Before:
```
$form = new Form($config); // implicit setData($config->getData());
$form->setData($object); // setData() is now called twice
```
After:
```
$form = new Form($config); // no implicit setData()
$form->getData(); // implicit setData($config->getData())
// or
$form = new Form($config);
$form->setData($object);
$form->getData(); // setData() was called only once
```
Add Response as possible return type of the method because the method AbstractAuthenticationListener::handle() test if $returnValue is an instance of Response (line 148).
Commits
-------
ed8823c [HttpFoundation] Allow setting an unknown status code without specifying a text
Discussion
----------
[HttpFoundation] Allow setting an unknown status code without specifying...
... a text
fix#4978
Commits
-------
16a980b [Validator] Fix bug order for constraint, property, getter and group-sequence-provider in validation.xml
Discussion
----------
[Validator] Fix bug order for constraint, property, getter and group-seq...
Actually, there is a bug that force developers to write validation.xml file with the following nodes order:
- constraint
- property
- getter
So that's not possible to have the following XML (because I need to write my property(ies) first).
```xml
<?xml version="1.0" encoding="UTF-8" ?>
<constraint-mapping xmlns="http://symfony.com/schema/dic/constraint-mapping"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://symfony.com/schema/dic/constraint-mappinghttp://symfony.com/schema/dic/constraint-mapping/constraint-mapping-1.0.xsd">
<class name="Application\Eko\MyBundle\Entity\MyEntity">
<getter property="isBar">
<constraint name="True">
<option name="message">My error message</option>
</constraint>
</getter>
<property name="foo">
<constraint name="NotBlank" />
</property>
</class>
</constraint-mapping>
```
The XML below result in the following exception:
```
[ERROR 1871] Element '{http://symfony.com/schema/dic/constraint-mapping}property': This element is not expected. Expected is ( {http://symfony.com/schema/dic/constraint-mapping}getter ). (in /var/www/myproject/src/Application/Eko/MyBundle/Resources/config/validation.xml - line 14, column 0)
```
This is due to the sequence element that needs to respect the order given in the schema file.
The choice element is doing the same thing and permit to have a free order of elements so I have replaced the sequence by a choice element.
For more information: http://www.w3.org/TR/xmlschema-0/#ref17
Commits
-------
c81b2ad [Form] Rename UnmodifiableFormConfig to ImmutableFormConfig
274eb9e [EventDispatcher] Rename UnmodifiableEventDispatcher to ImmutableEventDispatcher
Discussion
----------
Rename unmodifiable to immutable
Maybe it's just me, but it sounded really wrong. The EventDispatcher one was added in 2.1 so no BC break. I don't know about the Form one, but I guess it's just used internally anyway.
RHEL5 will intermittently result in an exit code of -1 [1] due to
proc_get_status() being called after the process has completed
but outside of updateStatus() which saves the exit code.
[1]: https://github.com/composer/composer/issues/876
Commits
-------
1f33756 Update master
Discussion
----------
Allow HttpKernel ->render to be called directly
Hi,
I faced this problem in a funny situation.
Working on tests of individual components, I want to be able to test sub-requests. These elements are not mapped directly, relying on `FrameworkBundle` routing internal to achieve its goal.
The nature of my application leads to fully decoupled bundles, being a website responsible to define everything, from app configuration to routing elements. That way, an individual bundle controller don't know the route it should call, leading the test to be harder to do.
Together with this situation, it is also required in my testing scenario to be able to test the real world execution, then being an ESI include (a sub-request). This test then needs to be able to directly call controller to be rendered. That said, `HttpKernel` provides an API that does exactly what is required to achieve my goal, calling `render()` which triggers the sub-request and also accepts options which allows me to test the real world scenarios.
But as soon as you trigger the method, `ProfileListener` intercepts the kernel response to collect profiling information. Under this specific situation, since you called directly `render`, there's no master request, then leading the test to fail with the following PHP warning:
```
Warning: SplObjectStorage::offsetExists() expects parameter 1 to be object, boolean given in /var/www/nde/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/EventListener/ProfilerListener.php line 120
```
To fix that, all that is needed is a check for a possible parent request. That's the purpose of this patch. =)
---------------------------------------------------------------------------
by stof at 2012-07-17T15:11:13Z
This looks good to me, but as said on IRC, a test should be added to avoid regressions
Commits
-------
1474aa5 [Form] Fixed consideration of Twig's template inheritance and added another performance-improving check
b4ec7f5 Fixed my rubbish English
d11f8b5 [Form] Fixed passing of variables in the FormRenderer
629093e [Form] Extracted common parts of FormHelper and FormExtension into separate classes
216c539 [Form] Implemented a more intelligent caching strategy in FormHelper (PHP +100ms, Twig +100ms)
Discussion
----------
[Form] Merged FormHelper and FormExtension and implemented a better caching strategy
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This PR extracts common parts of `FormHelper` and `FormExtension` into implementations of the new interfaces `FormRendererInterface` and `FormRendererEngineInterface`. The implemented `AbstractRendererEngine` features a more intelligent caching strategy than the one used before. When this strategy was implemented directly in `FormHelper`, the performance of [this specific, heavy form](http://advancedform.gpserver.dk/app_dev.php/taxclasses/1) could be improved from **2.5** to **2.25 seconds** on my machine for PHP templates.
Due to the abstraction and delegation, the performance gain is not that big anymore, but we still have a performance gain of about **0.1 seconds** for both PHP and Twig in the above example. The second, big improvement of this PR is maintainability - the differences between PHP and Twig templates are now contained in relatively small classes - and extendability (it is very easy now to support different template engines).
---------------------------------------------------------------------------
by stof at 2012-07-14T13:47:19Z
should a similar refactoring be done for the [Twig rendering](https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Extension/FormExtension.php) ?
---------------------------------------------------------------------------
by bschussek at 2012-07-14T13:49:25Z
Yes. I would like to merge the common parts of Twig's FormExtension and PHP's FormHelper into an abstract class. Before that I need to have a [working, heavy Twig Form](https://twitter.com/webmozart/status/224135287377371138) in order to measure whether I don't actually decrease the performance with Twig. Can you help me there?
---------------------------------------------------------------------------
by vicb at 2012-07-16T21:48:24Z
Would it make sense to create a 'renderer' folder in the form component and move related classes there ?
---------------------------------------------------------------------------
by stof at 2012-07-16T22:06:58Z
@vicb It makes sense to keep the Twig renderer in the brisge. This is what the bridge is about. Moving the Twig class to the component would not be consistent. And the PHP renderer is already in the component (but it could make sense to move the helper from FrameworkBundle to the TemplatingExtension of the Form component though)
---------------------------------------------------------------------------
by vicb at 2012-07-16T22:16:50Z
@stof I was only referring to the classes located in the Component/Form folder.
---------------------------------------------------------------------------
by vicb at 2012-07-16T22:27:27Z
Overall I don't really know what to think of this PR. PHP and Twig use a different way to support blocks:
- PHP has one block per file,
- Twig could have many blocks per templates.
I am not sure if this PR is optimal for Twig and improves maintainability ?
---------------------------------------------------------------------------
by stof at 2012-07-16T22:46:11Z
@vicb it avoids duplicating the whole rendering logic for each engine (there is at least a third one in [SmartyBundle](https://github.com/noiselabs/SmartyBundle/blob/master/Extension/FormExtension.php) btw)
---------------------------------------------------------------------------
by bschussek at 2012-07-17T07:16:42Z
@vicb I don't think a renderer subfolder makes sense. The interfaces belong to the main namespace, and then the subfolder would only contain two classes.
Considering maintainability for Twig, I think that this PR in fact increases it. TwigExtension before always had to check the whole type hierarchy, while now the code in AbstractRendererEngine makes sure that this process is speeded up.
Before:
```
load _some_entity_field_label:
- check _some_entity_field_label
- check entity_label
- check choice_label
- check form_label
load _some_other_entity_field_label
- check _some_other_entity_field_label
- check entity_label
- check choice_label
- check form_label
a.s.o.
```
After:
```
load _some_entity_field_label:
- check _some_entity_field_label
- check entity_label (hits the cache if entity_label was checked before)
- check choice_label (hits the cache if choice_label was checked before)
- check form_label
load _some_other_entity_field_label
- check _some_other_entity_field_label
- check entity_label (now definitely hits the cache)
a.s.o.
```
Since many fields share the same ancestors in the inheritance tree, this definitely improves performance.
As can also be deducted here, custom block names such as `_some_entity_field_label` are now a major drawback. There is nothing we can cache for them, so they need to be checked for every individual block that we load. Removing this feature surprisingly gains no performance for Twig (I need to investigate why at some point), but it speeds up rendering for **250ms** using the PHP engine on [this example form](advancedform.gpserver.dk/app_dev.php/taxclasses/1), dropping the rendering time from 1.25 to 1 sec on my local machine. I'm not sure what we should do here.
---------------------------------------------------------------------------
by stof at 2012-07-17T07:21:31Z
@bschussek could it be possible to have an implementation checking the custom block and another one skipping it ? This way, the user could disable this feature when he does not need it.
---------------------------------------------------------------------------
by bschussek at 2012-07-17T07:38:34Z
@stof It would be possible to add a switch to `FormRenderer` that controls whether custom blocks are checked or not.
If this switch is disabled by default, we break BC. If this switch is enabled by default, it will be pretty useless. People will start designing away for custom blocks, and once they want to improve performance, they can't turn off the switch anymore because it would require too many changes.
---------------------------------------------------------------------------
by stof at 2012-07-17T08:08:38Z
@fabpot what do you think about it ?
---------------------------------------------------------------------------
by bschussek at 2012-07-17T08:41:43Z
Another option that just came to mind is to remove inheritance checks for anything but _widget and _row. I.e., if we render `entity_widget`, check
```
_id_widget
entity_widget
choice_widget
form_widget
```
But if we render `entity_label`, only check
```
_id_label
form_label
```
This improves PHP Templating for **170ms** and Twig for **20ms**. We gain another **150ms** for PHP Templating and **~15ms** for Twig if we also restrict custom fields (_id_widget) to the _widget and _row suffixes (it's really hard to tweak the renderer for Twig.. I think a lot of its performance bottlenecks lie in Twig itself).
Do you have any data on how often blocks other than _widget and _row are customized for specific types/IDs?
---------------------------------------------------------------------------
by stof at 2012-07-17T09:47:38Z
Well, I think most of the time other blocks are not even customized based on the type :)
---------------------------------------------------------------------------
by Tobion at 2012-07-17T14:32:39Z
From my experience rendering the form components individually is easier and more flexible than customizing by ID or type.
But there are still use cases for customizing like library-like bundles (e.g. Bootstrap).
Commits
-------
610c602 [OptionsResolver] Slightly tweaked the performance of the Options class
Discussion
----------
[OptionsResolver] Slightly tweaked the performance of the Options class
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Commits
-------
1f2f866 fixed the serialization of the SwitchUserRole
b55930a [Security] Implemented the Serializable interface in the Role class
Discussion
----------
[Security] Implemented the Serializable interface in the Role class
The Role class is serialized in the session for each role of the user. Implementing the Serializable interface allows to reduce the size of the data.
Commits
-------
df2406f [Security] Add note to changelog about BC break
01b2e39 [Security] Extract default logout success handling logic
Discussion
----------
[Security] Extract default logout success handling logic
Bug fix: no
Feature addition: no
Backwards compatibility break: yes, small one for people using the component
Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/asm89/symfony.png?branch=default-logout-success-handler)](http://travis-ci.org/asm89/symfony)
License of the code: MIT
As discussed earlier with @fabpot and @schmittjoh. This PR extracts the default logout success handling logic to a separate class that users can extend.
Note: build status is red, but that is because of a failing performance test in the form component? ..
Commits
-------
7d53909 Earlier PHP output buffer flush for non FPM environments
Discussion
----------
Earlier PHP output buffer flush for non FPM environments
In the Response::send() method you are calling the fastcgi_finish_request() in case it exists. This will provide a respectful performance boost when you have significant work being done by listeners acting on kernel terminal events; Sadly you are forgetting people that don't use FPM doing this.
The performance boost for a Vanilla PHP is not much: flushing earlier potentially helps higher layers such as the HTTPd or potential other cache layers: the sooner their buffer gets filled, the sooner they release information to the browser, even if the output buffer is still open. The explicit flush() is supposed to do exactly this.
Commits
-------
33f29ed [Form] '@group benchmark' for form performance tests
Discussion
----------
[Form] '@group benchmark' for form performance tests
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/asm89/symfony.png?branch=form-performance)](http://travis-ci.org/asm89/symfony)
License of the code: MIT
I think a PR or note about this has been rejected before, but since build statuses on PRs sometimes seem to fail if travis is busy I think moving the form performance tests to `@group benchmark` should be reconsidered.
Edit: even master is currently failing on this
Commits
-------
07992d3 [Validator] Added inheritDoc phpdoc for validate methods
Discussion
----------
[Validator] Added inheritDoc phpdoc for validate methods
Was instructed by @stof to do this for a PR on comparison validators and noticed none of the validators used inheritDoc.
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: n/a
Todo: I haven't looked around too much, but I assume if none of the validators followed this standard that there would be a fair few other classes not using. Obviously not a big issue though
License of the code: MIT
Documentation PR: n/a
Commits
-------
5ae0da0 Update changelogs
ff91b9a [FrameworkBundle] Make FlashBag the default.
Discussion
----------
[2.1][FrameworkBundle] Make FlashBag the default.
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes (but only technically)
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
The difference between `AutoExpireFlashBag` and `FlashBag` is simply that the first will expire flashes regardless of being displayed on the next pageload. This can result in lost messages. It was created simply for BC with 2.0.
`FlashBag` expires flashes once they are retrieved. This also makes it ESI compatible.
/cc @lsmith77
---------------------------------------------------------------------------
by jalliot at 2012-07-14T18:13:40Z
+1!
You should add it to the changelog and upgrade files though :)
Commits
-------
480ab14 Further improving the MessageSelector exception
Discussion
----------
Further improving the MessageSelector exception
Hey guys!
The goal is just to give the users a better starting point when they see this exception.
See previous change in #4173 and conversation in #4207
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4207
Todo: -
License of the code: MIT
Documentation PR: n/a
Thanks!
---------------------------------------------------------------------------
by stof at 2012-07-14T23:11:17Z
Shouldn't it be done in 2.0 instead ?
---------------------------------------------------------------------------
by weaverryan at 2012-07-15T00:15:42Z
I decided to go against 2.1 when I saw that #4173 was against 2.0. If we do it against 2.0, it'll cause a conflict when 2.0 is merged into master - seemed like too much trouble for such a small change.
Commits
-------
12bdec3 Moved the NormalizationAwareInterface check to the ChainEncoder
28e137c [Serializer] Added a ChainEncoder and a ChainDecoder
Discussion
----------
[Serializer] Added a ChainEncoder and a ChainDecoder
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/stof/symfony.png?branch=serializer_improvement)](http://travis-ci.org/stof/symfony)
Fixes the following tickets: -
Todo: -
These classes contains the logic previously defined in the Serializer itself to handle the choice of a serializer. This allows reusing it when using only the encoding part of the component, without having to use the Serializer class (which is not as handy to bootstrap when you want to use only encoders and decoders as normalizers come first)
I was wondering if these classes should have adders but I kept the constructor injection only to be consistent with the current code (encoders cannot be registered after the instantiation) and to avoid implementing the SerializerAwareInterface in them (to allow injecting the Serializer in serializer-aware encoders and decoders added later).
Note that this change is fully BC as it only changes the internal implementation of the Serializer.
---------------------------------------------------------------------------
by fabpot at 2012-07-14T11:07:32Z
ping @lsmith77 @Seldaek
---------------------------------------------------------------------------
by Seldaek at 2012-07-14T15:17:42Z
After a quick look, I'd say +1
Commits
-------
dbd169f [Form] Error in the SimpleFormTest case.
Discussion
----------
[Form] Error in the SimpleFormTest case.
Symfony2 tests pass: yes
---------------------------------------------------------------------------
by bschussek at 2012-07-14T13:25:28Z
Thanks, looks like a copy paste error. @fabpot 👍
Commits
-------
6489a65 [Form] Added an exception for invalid type services
Discussion
----------
[Form] Added an exception for invalid type services
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes [![Build Status](https://secure.travis-ci.org/stof/symfony.png?branch=form_safeguard)](http://travis-ci.org/stof/symfony) (Travis fails randomly for the performance test)
Fixes the following tickets: -
Todo: -
Before the introduction of the FormRegistry, the getName() method was
never used for types registered through the DI container. The
FormRegistry now uses the getName() method and missconfigured services
will trigger a notice.
This was reported in FriendsOfSymfony/FOSCommentBundle#234
Before the introduction of the FormRegistry, the getName() method was
never used for types registered through the DI container. The
FormRegistry now uses the getName() method and missconfigured services
will trigger a notice.
This was reported in FriendsOfSymfony/FOSCommentBundle#234
These classes contains the logic previously defined in the Serializer
itself to handle the choice of a serializer. This allows reusing it when
using only the encoding part of the component.
Commits
-------
cd7835d [Form] Cached the form type hierarchy in order to improve performance
2ca753b [Form] Fixed choice list hashing in DoctrineType
2bf4d6c [Form] Fixed FormFactory not to set "data" option if not explicitely given
7149d26 [Form] Removed invalid PHPDoc text
Discussion
----------
[Form] WIP Improved performance of form building
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: **Update the Silex extension**
This PR is work in progress and up for discussion. It increases the performance of FormFactory::createForm() on a specific, heavy-weight form from **0.848** to **0.580** seconds.
Before, the FormFactory had to traverse the hierarchy and calculate the default options of each FormType everytime a form was created of that type.
Now, FormTypes are wrapped within instances of a new class `ResolvedFormType`, which caches the parent type, the type's extensions and its default options.
The updated responsibilities: `FormFactory` is a registry and proxy for `ResolvedFormType` objects, `FormType` specifies how a form can be built on a specific layer of the type hierarchy (e.g. "form", or "date", etc.) and `ResolvedFormType` *does the actual building* across all layers of the hierarchy (by delegating to the parent type, which delegates to its parent type etc.).
---------------------------------------------------------------------------
by schmittjoh at 2012-07-12T18:25:40Z
Maybe ResolvedFormType
---------------------------------------------------------------------------
by jmather at 2012-07-13T02:56:38Z
I really like ResolvedFormType. That's the naming method I took for my tag parser that handes the same conceptual issue.
---------------------------------------------------------------------------
by axelarge at 2012-07-13T05:25:00Z
ResolvedFormType sounds very clear.
This change is great and I desperately hope to see more of this kind
---------------------------------------------------------------------------
by Baachi at 2012-07-13T06:41:26Z
Yes `ResolvedFormType` sounds good :) 👍
---------------------------------------------------------------------------
by fabpot at 2012-07-13T07:11:33Z
I like `ResolvedFormType` as well.
---------------------------------------------------------------------------
by henrikbjorn at 2012-07-13T07:46:48Z
👍 `ResolvedFormType` :shipit:
---------------------------------------------------------------------------
by stof at 2012-07-13T18:01:51Z
This looks good to me
Commits
-------
a924dab [OptionsResolver] Made the OptionsResolver clonable
70307e5 [Form] Improved EntityType performance by caching the EntityChoiceList
8298d8c [Form] Improved ChoiceType performance by caching ChoiceList objects
Discussion
----------
[Form] Improved performance of ChoiceType and EntityType
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Commits
-------
a92f80b [Validator] Added Length constraint and deprecated MinLength and MaxLength
83a3f75 [Validator] Deprecated the constraints Min and Max in favor of Range
0cdacee [Validator] Removed MinCount and MaxCount and replaced them by the constraint Count
741c147 [Validator] Renamed deprecated Size constraint to Range
Discussion
----------
[Validator] Reintroduced Range constraint and created Count and Length constraints
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
After @Tobion's comment to #4851, this is the next try to streamline the constraints and reduce duplication of logic. The downside of the current MinLength/MaxLength and MinCount/MaxCount pairs is that they cannot output a fitting error message if a value should have an *exact* length/count. So this PR introduces
* Range (formerly Size) to replace Min/Max
* Count to replace MinCount/MaxCount
* Length to replace MinLength/MaxLength
Feedback is appreciated.
---------------------------------------------------------------------------
by Tobion at 2012-07-11T20:40:08Z
The `choice` constraint also cannot handle `min = max`. Or maybe we don't need these options on choice anymore as we can achieve the same with the new `count` constraint?!
---------------------------------------------------------------------------
by beberlei at 2012-07-12T08:59:44Z
Dude, nobody has time to fix the BC breaks you introduce :-)
---------------------------------------------------------------------------
by TomAdam at 2012-07-12T12:38:49Z
The changes to the `Size` validator yesterday broke my project, and I started rewriting to use `MaxLength / MinLength` validators today, until I spotted this. It would be good if this PR could have a reasonably high priority (whether or not it is accepted) as it will change how I fix my issues. I suspect a lot of people using the master branch will be in the same situation.
Commits
-------
34abe37 Added mime type for RAR archive, sending from Linux Chrome via nginx
Discussion
----------
Added new mime type for RAR archive
This mime type detected when sending file from Linux Chrome Browser via nginx
Commits
-------
5e6c06f [Security] Remove hard dependency on $providerKey for default auth success handler
Discussion
----------
[Security] Remove hard dependency on $providerKey for default auth success handler
Bug fix: yes?
Feature addition: yes?
Backwards compatibility break: no
Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/asm89/symfony.png?branch=fix-default-auth-successhandler-extension)](http://travis-ci.org/asm89/symfony)
License of the code: MIT
In 8ffaafa867 a hard dependency was introduced between the default authentication success handling code and the active firewall. This makes sense. However, for people implementing their own success handler this makes it impossible to extend the default class as the `$providerKey` is set in the extension of the security bundle.
This PR makes the dependency a soft one so people can extend the class and use the default definition as a parent for their own service. However it is the responsibility of the developers to set the appropriate `$providerKey` if they want to use the target url saved in the session. Imo this is the right way as the developer should also set the appropriate options for the parent class in the overriding constructor.
---------------------------------------------------------------------------
by stof at 2012-07-11T19:01:12Z
@asm89 this PR need to be rebased according to github
---------------------------------------------------------------------------
by asm89 at 2012-07-11T19:13:09Z
@stof Done :)
---------------------------------------------------------------------------
by asm89 at 2012-07-12T10:07:53Z
@fabpot Done.
Commits
-------
0be602d [Validator] Deprecated the Size constraint
d661837 [Validator] Reverted the changes done to the Size constraint in 3a5e84f4a7d84b689 [Validator] Added the constraints MinCount and MaxCount
1a732e4 [Validator] Removed the Range constraint as it duplicates functionality given in Min and Max
Discussion
----------
[Validator] Deprecated the Size constraint in favor of MinCount and MaxCount
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This PR cleans up with the current ambiguity between
* Min
* Max
* MinLength
* MaxLength
* Range
* Size
in the following ways:
* The Range constraint was removed again as it can be completely replaced by Min and Max.
* The Size constraint was reverted to it's 2.0 feature set and deprecated.
* The constraints MinCount and MaxCount were added to make up for the functionality that was added to Size.
Commits
-------
b7aae48 [Locale] Fixed error resetting in StubIntlDateFormatter::parse()
Discussion
----------
[Locale] Fixed error resetting in StubIntlDateFormatter::parse()
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4242
Todo: -
---------------------------------------------------------------------------
by bschussek at 2012-07-11T08:00:15Z
ping @eriksencosta, @igorw - is this solved as intended?
---------------------------------------------------------------------------
by eriksencosta at 2012-07-11T11:20:24Z
Yes, thanks!
Commits
-------
7a76dba [Form] Renamed the options "data_timezone" and "user_timezone"
655d645 [Form] Fixed tests failing on systems with timezones other than +01:00
Discussion
----------
[Form] Fixed tests and renamed the options "data_timezone" and "user_timezone"
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
The option names were renamed for better consistency with the terms "model data" or "model format" and "view data" or "view format".
Commits
-------
ded6c03 [Form] DateTimeType now handles RFC 3339 dates as provided by HTML5
7e8b622 [Form] Added the option "format" to DateTimeType
9eeb200 [Form] Changed the default format of DateType to "yyyy-MM-dd" to support HTML 5 out of the box
d621a76 [Form] Improved DateTimeType code
Discussion
----------
[Form] Changed DateType and DateTimeType to support HTML5 by default
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #2849, #3162
Todo: -
This PR changes DateType and DateTimeType to support HTML5 by default when setting the option "widget" to "single_text".
Also, the option "format" was added to DateTimeType.
---------------------------------------------------------------------------
by stof at 2012-07-10T15:38:44Z
This loos OK to me
---------------------------------------------------------------------------
by MDrollette at 2012-07-10T16:36:26Z
@stof typo: "looks" #meta-stoffed
Commits
-------
02e0a8f Allow Kernel::$name to be overridden by subclasses
Discussion
----------
Allow Kernel::$name to be overridden by subclasses
Because the name of the kernel is calculated in the constructor,
any child class that had overriden the kernel name, will be
ignored.
By setting the kernel name in the child class, we can avoid having
to execute the regex to calculate the name upon every construction
of a Kernel.
A test (and a kernel fixture) is added to prove that the override
works correctly.
Note: the Kernel API has not been touched, so there should be no
issues with BC.
What do you think?
Because the name of the kernel is calculated in the constructor,
any child class that had overriden the kernel name, will be
ignored.
By setting the kernel name in the child class, we can avoid having
to execute the regex to calculate the name upon every construction
of a Kernel.
A test (and a kernel fixture) is added to prove that the override
works correctly.
Note: the Kernel API has not been touched, so there should be no
issues with BC.
Commits
-------
5b057f8 [Form] Fixed DateType to use "format" for creating the year and day choices
Discussion
----------
[Form] Fixed DateType to use "format" for creating the year and day choices
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3490
Todo: -
Commits
-------
f71e2a8 [Form] FormBuilder Bug Fix: remove() was not properly removing children
Discussion
----------
[Form] FormBuilder Bug Fix: remove() was not properly removing children
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4803
License of the code: MIT
FormBuilder initially sets unresolved children as NULL, until resolved.
If FormBuilder::remove() is called before that child is resolved, the
if statement turns false, because isset(null) is false, when it should
be true. Instead, we should check to see if the key exists, and if so,
process and unset it.
Closes#4803
---------------------------------------------------------------------------
by bschussek at 2012-07-10T07:41:55Z
Can you please add a test covering this case?
---------------------------------------------------------------------------
by ChrisTickner at 2012-07-10T09:43:07Z
Sure, added a test case. It fails before the patch and passes after.
---------------------------------------------------------------------------
by bschussek at 2012-07-10T09:47:06Z
Thanks. Can you please add a comment to the test with the URL of this PR? Also, please squash your commits into one when your done.
---------------------------------------------------------------------------
by ChrisTickner at 2012-07-10T10:02:16Z
Oops, I deleted the remote branch and re-pushed without realizing we'd lose some history on this PR page. Live and learn I suppose.
---------------------------------------------------------------------------
by bschussek at 2012-07-10T10:18:20Z
Thanks!
FormBuilder initially sets unresolved children as NULL, until resolved.
If FormBuilder::remove() is called before that child is resolved, the
if statement turns false, because isset(null) is false, when it should
be true. Instead, we should check to see if the key exists, and if so,
process and unset it.
Closes#4803
Commits
-------
6ad4018 [Form] Also display the hint about adder/remover on invalid property access
Discussion
----------
[Form] Also display the hint about adder/remover on invalid property access
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
This PR follows up #4777. In this case the hint about adders and removers is also added when a property is found, but is not public, a common case.
Commits
-------
c6cb8b2 [Form] Removed unused option "inline" that was introduced by accident
Discussion
----------
[Form] Removed unused option "inline" that was introduced by accident
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Commits
-------
9c94b48 [Form] Fixed the "data" option to supersede default data set in the model
Discussion
----------
[Form] Fixed the "data" option to supersede default data set in the model
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3899
Todo: -
Commits
-------
7727de7 [Form] Deprecated Form::bindRequest() and replaced it by a PRE_BIND listener
Discussion
----------
[Form] Deprecated Form::bindRequest() and replaced it by a PRE_BIND listener
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Instead of `bindRequest`, you should now simply call `bind`:
Before:
```
$form->bindRequest($request);
```
After:
```
$form->bind($request);
```
Commits
-------
ec8c023 Serbian Cyrillic translation updated.
Discussion
----------
Serbian Cyrillic translation updated.
Shouldn't source messages be updated in all files in one commit? Also, version is 1.2 in both, en and sr files, but source messages are not.
I would like to be notified whenever translation is changed, maybe we should have some topic on mailing list so all translators can receive notifications once messages are changed.
Commits
-------
f093caa [Form] Fixed invalid value passed to the constraint violation by the MinLength and MaxLength validators
Discussion
----------
[Form] Fixed invalid value passed to the constraint violation by the MinLength and MaxLength validators
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4573
Todo: -
Commits
-------
eba7dfe Revert "[Form] added a circular reference safeguard for form type"
Discussion
----------
Revert "[Form] added a circular reference safeguard for form type"
This reverts commit ea93e4cafa.
Conflicts:
src/Symfony/Component/Form/FormBuilder.php
src/Symfony/Component/Form/FormFactory.php
Commits
-------
df5bb4a [Form] Unified rendering of errors for nested elements
Discussion
----------
[Form] Unified rendering of errors for nested elements
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes?
Symfony2 tests pass: yes
Fixes the following tickets: #4615
Todo: -
Commits
-------
1345360 [Form] Fixed PropertyPath handling of offsetGet() that returns a constant value
6e1462e [Form] Fixed PropertyPath handling of __get() method that returns a constant
Discussion
----------
[Form] Fixed "Indirect modification.." exceptions in PropertyPath
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4450, #4535?, #4612
Todo: -
Commits
-------
3462afc Tests for PluralizationRules.
Discussion
----------
[WIP][Translations][tests] Tests for PluralizationRules.
Currently we have no tests PluralizationRules. This patch is an initial one to show we have not enough langcodes in PluralizationRules.
I hope this gets in so others can fix the missing langcodes.
I'm waiting for [RFC [MessageCatalogue*] Add Metadata to MessageCatalogue](https://github.com/symfony/symfony/pull/4399) to get in to continue working on the [[WIP]: Allow Drupal to use Translate component](https://github.com/symfony/symfony/pull/4249).
---------------------------------------------------------------------------
by travisbot at 2012-05-25T14:38:37Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1433558) (merged 3462afc0 into 023dbf80).
---------------------------------------------------------------------------
by drak at 2012-07-01T09:47:05Z
Is there anything pending in this PR?
Commits
-------
28b6557 class_exists was returning fals for the container aware interface
Discussion
----------
class_exists was returning fals for the container aware interface
This test was marked skipped when it should not have been.
Commits
-------
e6b5595 [Form] Fixed display of empty values in DateType, TimeType and DateTimeType if they are not required
14e293f [Form] Refactored processing of the "empty_value" option in DateType
Discussion
----------
[Form] Fixed display of empty values in DateType, TimeType and DateTimeType
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4596
Todo: -
Commits
-------
b4d7a7e [Component][Finder][SplFileInfo] file_get_contents=>fpassthru
Discussion
----------
[Component][Finder][SplFileInfo] file_get_contents=>fpassthru
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: https://github.com/symfony/symfony/pull/4335/files#r1016152
Todo: -
License of the code: MIT
Documentation PR: -
Commits
-------
040ba8f [Form] Fixed: ChoiceType omits the "empty_value" option if the choices contain an empty element
Discussion
----------
[Form] Fixed: ChoiceType omits the "empty_value" option if the choices contain an empty element
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3854, #3864
Todo: -
Commits
-------
6d94f3e Ensure there is a session before getting the session id
Discussion
----------
[Form] Ensure there is a session before getting the session id
Solves "The CSRF token is invalid. Please try to resubmit the form" error when a form is generated before the session is started.
---------------------------------------------------------------------------
by fabpot at 2012-07-09T10:23:32Z
Adding a CSRF token only makes sense if you are on a page with a "user". If not (and if you don't use HTTP auth or whatever), then there is no need for a CSRF token.
---------------------------------------------------------------------------
by frosas at 2012-07-09T14:42:40Z
This PR doesn't change any logic on whether a CSRF token is added or not, it just fixes a bug when a token is requested.
Commits
-------
6c9c2ec [ClassCollectionLoader] fixed comment striping on classes in global namespace
Discussion
----------
[ClassCollectionLoader] fixed comment striping on classes in global namespace
Comments aren't striped when the class is in the global namespace, this adds a fake namespace to let the fixNamespaceDeclaration method handling the formating, so we can remove a method.
I've also put dev as minimum policy in composer, otherwise I couldn't install dependencies.
Commits
-------
1fa22d9 [Form] Output a more usable error when PropertyPath has tried to find adders and getters, but failed to find them
Discussion
----------
[Form] Output a more usable error when PropertyPath has tried to find ad...
...ders and getters, but failed to find them
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
I've refactored the writeProperty method of propertypath in order to supply a better error message when writing has failed.
The writeProperty method itself now finds singulars (if a singular was not passed) for the private findAdderAndRemover method which allowed for some duplicate code to be removed and since the writeProperty now holds this data, it can provide a more verbose exception message.
---------------------------------------------------------------------------
by bschussek at 2012-07-09T13:54:35Z
Apart from the typo this PR looks good.
---------------------------------------------------------------------------
by Burgov at 2012-07-09T14:01:04Z
fixed&squashed
Commits
-------
bb138da [Security] Fix regression after rebase. Target url should be firewall dependent
eb19f2c [Security] Add note to CHANGELOG about refactored authentication failure/success handling [Security] Various CS + doc fixes [Security] Exception when authentication failure/success handlers do not return a response [Security] Add authors + fix docblock
f9d5606 [Security] Update AuthenticationFailureHandlerInterface docblock. Never return null
915704c [Security] Move default authentication failure handling strategy to seperate class [Security] Update configuration for changes regarding default failure handler [Security] Fixes + add AbstractFactory test for failure handler
c6aa392 [Security] Move default authentication success handling strategy to seperate class [Security] Update configuration for changes regarding default success handler [Security] Fix + add AbstractFactory test
Discussion
----------
[Security] Refactor authentication success handling
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/asm89/symfony.png?branch=refactor-authentication-success-handling)](http://travis-ci.org/asm89/symfony)
License of the code: MIT
This PR extracts the default authentication success handling to its own class as discussed in #4553. In the end the PR will basically revert #3183 (as suggested by @schmittjoh) and fix point one of #838.
There are a few noticeable changes in this PR:
- This implementation changes the constructor signature of the `AbstractAuthentictionListener` and `UsernamePasswordFormAuthenticationListener` by making the `AuthenticationSuccessHandler` mandatory (BC break). If this WIP is approved I will refactor the failure handling logic too and then this will also move one place in the constructor
- This PR reverts the change of making the returning of a `Response` optional in the `AuthenticationSuccessHandlerInterface`. Developers can now extend the default behavior themselves
@schmittjoh Any suggestions? Or a +1 to do the failure logic too?
---------------------------------------------------------------------------
by schmittjoh at 2012-06-17T23:53:07Z
+1 from me
@fabpot, what so you think?
---------------------------------------------------------------------------
by fabpot at 2012-06-19T08:15:48Z
Can you add a note in the CHANGELOG? Thanks.
---------------------------------------------------------------------------
by asm89 at 2012-06-19T10:22:20Z
I will, but I'll first do the same for the failure logic.
---------------------------------------------------------------------------
by travisbot at 2012-06-21T08:03:14Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1671555) (merged 17c8f66f into 55c6df99).
---------------------------------------------------------------------------
by asm89 at 2012-06-21T08:45:38Z
👍 thank you @stof. I think this is good to go now.
---------------------------------------------------------------------------
by travisbot at 2012-06-21T08:50:28Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1671817) (merged 8982c769 into 55c6df99).
---------------------------------------------------------------------------
by asm89 at 2012-06-21T14:23:58Z
@schmittjoh @fabpot The `LogoutListener` currently throws an exception when the successhandler doesn't return a `Response` ([link](9e9519913d/src/Symfony/Component/Security/Http/Firewall/LogoutListener.php (L101))). Should this code check for this too?
---------------------------------------------------------------------------
by schmittjoh at 2012-06-21T14:26:49Z
Yes, this code was removed, but needs to be re-added here as well.
---------------------------------------------------------------------------
by travisbot at 2012-06-21T15:08:59Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1674437) (merged 5afa240d into 55c6df99).
---------------------------------------------------------------------------
by asm89 at 2012-06-26T06:01:02Z
@fabpot Can you make a final decision on this? If you decide on point 3, this code can be merged. I agree with the arguments of @stof about the option handling and it 'only' being a BC break for direct users of the security component. I even think these direct users should be really careful anyway, since the behavior of the success and failurehandlers now change back to how they acted in 2.0.
Now I am thinking about it, can't the optional parameters of this class move to setters anyway? That will make it cleaner to extend.
---------------------------------------------------------------------------
by asm89 at 2012-06-28T10:29:50Z
ping @fabpot
---------------------------------------------------------------------------
by fabpot at 2012-06-28T17:23:02Z
I'm ok with option 1 (the BC break). After doing the last changes, can you squash your commits before I merge? Thanks.
---------------------------------------------------------------------------
by asm89 at 2012-07-06T21:59:54Z
@fabpot I rebased the PR, added the authors and also ported the fix that was done in 8ffaafa867 to be contained in the default success handler. I also squashed all the CS and 'small blabla fix' commits. Is it ok now?
Edit: travisbot will probably say that the tests in this PR fail, but that is because current master fails on form things
---------------------------------------------------------------------------
by asm89 at 2012-07-08T18:53:05Z
I rebased the PR, tests are green now: [![Build Status](https://secure.travis-ci.org/asm89/symfony.png?branch=refactor-authentication-success-handling)](http://travis-ci.org/asm89/symfony).
[Security] Various CS + doc fixes
[Security] Exception when authentication failure/success handlers do not return a response
[Security] Add authors + fix docblock
Commits
-------
6de6806 [Component][HttpKernel] fixed testGetRootDir() on Win
Discussion
----------
[Component][HttpKernel] fixed testGetRootDir() on Win
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/gajdaw/symfony.png?branch=kernel_test_fix)](http://travis-ci.org/gajdaw/symfony)
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -
Method `getRootDir()` returns path containing slashes. On Windows machine `__DIR__` and `DIRECTORY_SEPARATOR` are backslashes. To pass the test on win machine we have to translate `\` into `/`.
Commits
-------
df66d90 slovenian validators translations updated
Discussion
----------
Slovenian validators translations updated
Updated Slovenian validators translations for master branch.
Commits
-------
d6e1f39 [Form] Fixed FormBuilder to maintain order of its children
Discussion
----------
[Form] Fixed FormBuilder to maintain order of its children
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4693
Todo: -
Commits
-------
5fe3f39 [Form] Made data mappers completely responsible for dealing with empty values. Removed duplication of code.
9bf6e8b [Form] Compound forms now always need a data mapper. Otherwise an exception is thrown.
Discussion
----------
[Form] Made requirements for data mappers stricter
Bug fix: no
Feature addition: no
Backwards compatibility break: (yes)
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
This cleanup was done while trying to fix#4715, which is not easily fixable right now. It breaks BC for those people who do not extend `FormType` and manually construct `Form` instances, which hopefully nobody does as it is absolutely *not* recommended at this time.
This is not a problem with Symfony, but when using the component
standalone (Silex for instance), the context listener might be
instantiated even if the firewall does not need to be fired. In that
case, the handle() method is not called, but the response listener is
called, which means that en empty token is stored in the session.
For Silex, it means that when authenticated, if you visit a 404 page,
you would be disconnected automatically.
Commits
-------
819fe34 [Form] refactored variable name to be more consistent with rest of the naming
Discussion
----------
[Form] Refactored config variable naming to be more consistent
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Since `clientTransformer` is renamed to `viewTransformer` everywhere, I think for consistency reasons config variable name should be changed too..
---------------------------------------------------------------------------
by stloyd at 2012-07-06T07:31:39Z
Maybe also rename of `normTransformers` to `modelTransformers` ? As this were changed too.
---------------------------------------------------------------------------
by Inori at 2012-07-06T07:40:53Z
@stloyd makes sense, done
---------------------------------------------------------------------------
by bschussek at 2012-07-06T08:24:43Z
Thanks for the cleanup. Can you squash the commits and prefix the message with "[Form]" please?
---------------------------------------------------------------------------
by Inori at 2012-07-06T08:43:49Z
@bschussek done
---------------------------------------------------------------------------
by bschussek at 2012-07-06T09:31:04Z
Thanks! @fabpot 👍
Commits
-------
6f9eda9 [Form][Validator] Fixed generation of HTML5 pattern attribute based on Assert\Regex to remove delimiters.
Discussion
----------
[Form][Validator] Fixed generation of HTML5 pattern attribute based on Assert\Regex by removing delimiters or using a new option: htmlPattern.
Hopefully, this time is the good one…
* Fixes: [#3766, #4077, #4513, #4520, #4521]
* Bug fix: yes
* Feature addition: yes
* BC break: no
* Symfony2 tests pass: yes
In Issue #3766, it was asked that Assert\Regex generates HTML5 pattern attribute.
It was done in PR #4077, but the generated Regex is in delimited format which is not supported by HTML5.
Hence, `/[a-z]+/` would be converted to `[a-z]+`.
If flags are specified like in `/[a-z]+/i`, it cannot be converted and pattern validation will be disabled client-side. If is however now possible, using a new option, `htmlPattern`, to specify the pattern you want to be used.
Example:
```php
<?php
/**
* @Assert\Regex(pattern="/^[0-9]+[a-z]*$/i", htmlPattern="^[0-9]+[a-zA-Z]*$")
*/
private $civic_number;
```
**Note**: [Documentation](http://symfony.com/doc/current/reference/constraints/Regex.html) should be updated accordingly.
---------------------------------------------------------------------------
by lavoiesl at 2012-06-08T15:45:17Z
God, I just found out you can "add more commits to this pull request by pushing to the master branch on lavoiesl/symfony"…
---------------------------------------------------------------------------
by travisbot at 2012-06-08T15:50:31Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1568634) (merged 2d767b41 into b84b46ba).
---------------------------------------------------------------------------
by petajaros at 2012-07-04T14:23:16Z
Anything new about this issue?
---------------------------------------------------------------------------
by lavoiesl at 2012-07-04T16:25:43Z
Alright, tests are passing using `phpunit -c phpunit.xml.dist --filter 'RegexValidatorTest'`. @travisbot reports errors because he can’t even start the tests due to dependencies, which is not related
---------------------------------------------------------------------------
by vicb at 2012-07-04T16:31:13Z
It should be ready to merge when you have taken the last comments into account. thanks.
---------------------------------------------------------------------------
by lavoiesl at 2012-07-04T16:39:05Z
So it seems this PR will finally pass, thanks a lot.
---------------------------------------------------------------------------
by vicb at 2012-07-04T17:03:35Z
Thank you for this PR and the changes.
---------------------------------------------------------------------------
by fabpot at 2012-07-04T17:10:20Z
@lavoiesl Can you squash your commits before I merge? Thanks.
---------------------------------------------------------------------------
by lavoiesl at 2012-07-04T17:25:18Z
There. I also left trace of some commits I did.
Thanks
[Validator] Added delimiter escaping to Validator\Constraints\Regex::getNonDelimitedPattern
[Form][Validator] Added htmlPattern option for Regex Validation.
[Validator] Fixed Validator\Constraints\Regex::getNonDelimitedPattern variable declarations
[Validator] Fixed tests for Regex htmlPattern option (instead of html_pattern)
[Validation] tweaked generation of pattern to include .* when not anchors are present. Also removed the exception and made getNonDelimitedPattern private
Commits
-------
18f8fc7 [Finder] tests for Yaml class
Discussion
----------
[Finder] tests for Yaml class
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/gajdaw/symfony.png?branch=master)](http://travis-ci.org/gajdaw/symfony)
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -
Additional tests improving tests coverage.
Commits
-------
3f9e8ff [ClassLoader] made ClassCollectionLoader::load() automatically include class dependencies
6f4d281 [ClassLoader] added missing support for PHP 5.4 traits
Discussion
----------
Classloader optimization
The first commit fixes support for PHP 5.4 trait.
The second one does several things:
* it optimizes the recent merge so that the reflection class instance is only loaded once;
* we use the fact that we now get all class dependencies to automatically add all class dependencies to the map.
---------------------------------------------------------------------------
by fabpot at 2012-07-03T17:26:46Z
I've updated to take into accounts traits.
---------------------------------------------------------------------------
by bamarni at 2012-07-04T11:58:57Z
great job 👍
I can't see it in the diff as this part hasn't changed, but somewhere in the autoReload block there is :
```
if ($meta[1] != $classes) {
$reload = true;
}
```
It should be array_unique($classes), otherwise the file would be perpetually regenerated in autoReload mode when the input contains duplicate, because they're implicitely removed when dumping the files.
---------------------------------------------------------------------------
by fabpot at 2012-07-04T13:20:04Z
@bamarni I've added an `array_unique` call at the top (this bug existed before by the way).
Commits
-------
c40a4e5 [HttpFoundation] fix query string normalization
f9ec2ea refactored test method
0880174 [HttpFoundation] added failing tests for query string normalization
Discussion
----------
[HttpFoundation] fix query string normalization
This fixes the query string normalization. There were several problems in it (see test cases that I added).
The main issue, that first catched my eye, was that the query string was urldecoded before it was exploded by `=`. See old code: `explode('=', rawurldecode($segment), 2);`. This means an encoded `=` (`%3D`) would falsely be considered a separator and thus lead to complete different parameters. The fixed test case is at `pa%3Dram=foo%26bar%3Dbaz&test=test`.
---------------------------------------------------------------------------
by Tobion at 2012-07-04T02:21:25Z
cc @simensen considering your PR 4711
Commits
-------
d37003e [HttpFoundation] small fixes in Request
Discussion
----------
[HttpFoundation] small fixes in Request
phpdoc fixes,
making http_build_query explicit
fixing query string of '0', that was ignored.
Unfortunately this '0' problematic is omnipresent because PHP makes it so easy to get wrong (as it is converted to boolean false). I don't know how often I fixed such issue already.
Commits
-------
45219ef [Process] Add default xampp path to the list of possible paths to check
28e1313 [Process] Clean-up/simplify code
56f473a [Process] Add extra dirs argument to the executable finder to allow searching more dirs
Discussion
----------
ExecutableFinder changes
It cleans up stuff a bit and adds a guess for xampp users that wouldn't have php in their path
Commits
-------
eda439f [EventDataCollector] Display a better message when no events have been recorded
6b87981 [TimeDataCollector] Do not throw an exception when no events are recorded
Discussion
----------
[Profiler] Better support for collector in a production env
relates to #3706.
With this PR it is possible to:
- enable only the profiler in a production environment - the wdt being disabled you have to switch to the development environment to inspect the collected data,
- enable both the profiler and the wdt in a production environment (the use case form #3706).
@jmikola would this solve your use case ?
Commits
-------
26a1e0b [ClassLoader] ordered ClassCollectionLoader writing to avoid redeclaration at runtime
Discussion
----------
[HttpKernel] allowed classes to compile to be prepended
I had an issue when registering JMSDIExtraBundle before the frameworkBundle, because the bundle is adding a class to compile wich extends a class to compile added by the frameworkbundle (https://github.com/schmittjoh/JMSDiExtraBundle/blob/master/HttpKernel/ControllerResolver.php#L39).
In my kernel, the bundle was registered before the frameworkbundle, if it's the case, the class is writed before the symfony core class in the cache file, so it will trigger the autoloader to load the symfony core class, then we'll have a fatal error because we're declaring 2 times the same class.
I'm suggesting to add a way to prepend the classes to compile added by an extension, this way we could force classes from the core bundle to pe prepended, and avoid this kind of error with other bundles adding classes extending core classes or implementing core interface. I've also added it to the frameworkbundle.
---------------------------------------------------------------------------
by travisbot at 2012-07-01T12:32:28Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1750090) (merged a989d35c into a1b73887).
---------------------------------------------------------------------------
by schmittjoh at 2012-07-01T12:45:46Z
This doesn't sound really failure proof, better would be to resolve the compiled classes in a way that handles the dependencies gracefully regardless of the order that they are registered in.
---------------------------------------------------------------------------
by bamarni at 2012-07-01T13:03:15Z
yes that would be much cleaner, even if it's not perfect, I don't have any other examples, but I think in most of situations this error happens because of the frameworkbundle late registration?
---------------------------------------------------------------------------
by bamarni at 2012-07-01T23:07:24Z
I've added an automatic reordering as suggested. Now I don't get the error whatever the order of FrameworkBundle and JMSDIExtraBundle is.
---------------------------------------------------------------------------
by vicb at 2012-07-02T10:02:33Z
Does the algo really works ?
Let's say I have the following hierarchy `A < B < C` and the classes added in the following order; `C, B, A`. I believe that the algo would return `B, A, C` instead of `A, B, C`.
An other question: should the dependency resolution be implemented in the `ClassCollectionLoader` rather than in the compiler pass ?
---------------------------------------------------------------------------
by vicb at 2012-07-02T10:23:11Z
@bamarni could you confirm the issue with the algo ? If it is confirmed, do you have time to work on a fix ?
---------------------------------------------------------------------------
by bamarni at 2012-07-02T11:05:53Z
the algo looks correct, there is a loop checking dependencies and appending the parent classes at the begining so it should also work with A > B > C, but I'll check and add a unit test.
You're right about the location it could belong there if we want a general fix, should I put it in the ClassLoader component?
---------------------------------------------------------------------------
by vicb at 2012-07-02T11:28:14Z
Yep please move the code to the ClassLoader.
You could also add some cache mechanism:
- ReflectionClass could be cached (they might be use mulitple times when some dependencies exist and are also used by the class loader)
- May be you could also cache the hierarchy of the classes
---------------------------------------------------------------------------
by bamarni at 2012-07-02T17:25:08Z
@vicb : indeed there were something wrong with the algo when it needed to handle more than 1 level of dependency, it's fixed, I've added a few test case and it passes.
Caching the dependencies found with reflection would be useful if people are dumping several files with some similar classes in the same process, but I don't see why we would need to cache the hierarchy?
---------------------------------------------------------------------------
by vicb at 2012-07-02T18:29:06Z
@bamarni I was referring to a local cache (ie local variable).
- On the first iteration, the code instantiate a reflection class,
- On other iteration you would instantiate again a reflection class for a subset of the classes
- ...
- In the end the former code instantiate again a reflection class for each class in order to get the file name, ...
I was also thinking of an other algo: count the number of parent classes for each classes to include and order classes according to this number. This would be a single loop only, what do you think ?
---------------------------------------------------------------------------
by bamarni at 2012-07-02T19:32:34Z
hum good idea counting the parents is cleaner, even though it looks enough, would it also make sense to treat interfaces and classes separately? I'm thinking about a file with all the interfaces at the begining (ordered by number of parents desc), then the classes would be appended (ordered by number of parents desc too).
---------------------------------------------------------------------------
by vicb at 2012-07-03T07:05:07Z
There is no real benefit in making the interfaces appear first but you can do it if it doesn't make the code more complex.
Please also add the `@throws` annotation to the methods.
---------------------------------------------------------------------------
by bamarni at 2012-07-03T09:37:28Z
@vicb : I've changed it to use the parents count, looks good to me.
---------------------------------------------------------------------------
by bamarni at 2012-07-03T12:23:46Z
changed
---------------------------------------------------------------------------
by bamarni at 2012-07-03T12:42:29Z
fixed
Commits
-------
3466896 [Routing] fix encoding of static static, so UrlGenerator produces valid URLs
Discussion
----------
[Routing] fix encoding of static text
Fixes#4166
As requested by Fabien, I split #4205 into multiple PRs.
---------------------------------------------------------------------------
by stof at 2012-06-09T12:49:32Z
github tells me this PR cannot be merged automatically. Could you rebase it ?
---------------------------------------------------------------------------
by Tobion at 2012-06-09T13:12:55Z
Done
---------------------------------------------------------------------------
by travisbot at 2012-06-09T13:18:18Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1576266) (merged 3466896a into 15b5aa4f).
Commits
-------
d9439ab made the charset overridable (closes#2072)
Discussion
----------
made the charset overridable (closes#2072)
The charset was configurable in a configuration file but it never worked:
framework:
charset: ISO-8859-1
Now, like for the cache and log dirs, you can configure the charset by
overriding the getCharset() method in the app kernel:
public function getCharset()
{
return 'ISO-8859-1';
}
---------------------------------------------------------------------------
by fabpot at 2012-07-03T07:26:04Z
See #2072 for the previous attempts to fix this issue.
The charset was configurable in a configuration file but it never worked:
framework:
charset: ISO-8859-1
Now, like for the cache and log dirs, you can configure the charset by
overriding the getCharset() method in the app kernel:
public function getCharset()
{
return 'ISO-8859-1';
}
Commits
-------
6296a24 Standalone query string normalization
Discussion
----------
[HttpFoundation] Standalone query string normalization
I wanted to leverage query string normalization in a test. I considered copying this code to my own library but I noticed that the only instance data `getQueryString` needed to know from the `Request` was `QUERY_STRING` so I broke the rest out into a standalone static function.
---------------------------------------------------------------------------
by simensen at 2012-07-02T20:05:59Z
I made the requested changes.
---------------------------------------------------------------------------
by fabpot at 2012-07-02T20:10:27Z
Can you squash your commits? Thanks.
---------------------------------------------------------------------------
by simensen at 2012-07-02T20:16:52Z
Sure, no problem. Hopefully I did it correctly.
Commits
-------
8ffaafa Make the session entry for the target url firewall dependent.
Discussion
----------
[Security] Make the session entry for the target url firewall dependent.
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets:
License of the code: MIT
If there are two firewalls (eg. main and admin), calling an protected admin url
will direct you to the login form of the admin. If I ignore this and go to the login
form of the main firewall directly I will end up being redirected to the stored
admin target url, which will lead me to the admin login form again.
---------------------------------------------------------------------------
by travisbot at 2012-05-25T09:33:44Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1431566) (merged 8ffaafa8 into 45849ce3).
---------------------------------------------------------------------------
by uwej711 at 2012-06-09T08:05:54Z
Doesn't this make sense or did this slip through? Or is there something missing?
Commits
-------
88caf3a [HttpKernel] removed the storage of the current locale in the session
Discussion
----------
[HttpKernel] removed the storage of the current locale in the session
Before this commit, the current locale was stored in the session (if one
was already started). That way, for the next requests, even if the
request locale attribute was not set, the locale was "restored".
But this is a really bad practice as it means that the same URL can have
a different content depending on the previous requests. It would have
been better if the Vary header was set but the locale can be different
from the value coming from the Accept-Language anyway.
This is a BC break but fortunately, you can restore the 2.0 behavior by
creating a simple event listener that contains the logic removed by this
commit.
---------------------------------------------------------------------------
by travisbot at 2012-07-01T06:56:48Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1748659) (merged 009e30f0 into 2e356c1a).
---------------------------------------------------------------------------
by schmittjoh at 2012-07-01T08:15:46Z
How about using a cookie instead? It would remove the BC break, and also be possible to use a vary header?
---------------------------------------------------------------------------
by fabpot at 2012-07-01T09:13:44Z
The goal is to make Symfony as stateless as possible; introducing a cookie would defeat this goal.
---------------------------------------------------------------------------
by drak at 2012-07-01T09:19:37Z
@fabpot - thank you for bringing this to attention. I was meaning to do it a long time ago. The requested language is entirely a per request issue and must always be so. URLs must only ever return one content, and not multiple (e.g. different languages). The correct way to behave is to detect the language based on URL and failing that where a language is not requested, to look at the preferred language from the browser request and if available it can be redirected to that resource (e.g. /fr). This is what we do in Zikula. We have a further session based setting for "preferred language" which if set will override the browser default.
In summary:
1. If the language is specified in the GET request, return that language always. E.g. domain.com/fr/foo should return a French version of foo
2. If no language is specified in the GET request: first check the session for a preferred language, otherwise check the browser string for the preferred language and then if necessary, redirect to that resource. We have a setting which additionally say "always have language in URL, and don't put language code in URL for default language"
This means what in Zikula we only ever have one URL per language version of a page, but it still allows for users to set their preferred language which is taken in to account mainly when they visit the homepage (but in fact any page without a specific language in the request).
---------------------------------------------------------------------------
by drak at 2012-07-01T09:38:06Z
+1 on this PR. Basically the request locale should be in the Request object and calculated according to the applications preferences.
---------------------------------------------------------------------------
by schmittjoh at 2012-07-01T12:38:25Z
I agree that content must be detected based on the request, but I strongly disagree with relying entirely on the URL.
@fabpot, if you think about it using a cookie would still be stateless. There would be no state whatsoever, the detection would be entirely based on the request. Whether the language information is transmitted in the URL or as part of request headers is for the developer to decide eventually, at least IMO. My suggestion would just provide a default which is more BC.
---------------------------------------------------------------------------
by drak at 2012-07-01T20:08:50Z
@schmittjoh it's not entirely from the URL, there are browser preferences and also user defaults ca nalso available but the latter is slightly higher level. IMO it's not really Symfony's job here, it's application level specific. We have a pretty good working example of that in Zikula. Anyone can easily implement your own requirements with a listener.
What is absolutely clear however is it is wrong for one URL to deliver more than one version of any content.
---------------------------------------------------------------------------
by schmittjoh at 2012-07-01T21:16:52Z
I'm 100% for this change. My suggestion would just be more BC while still keeping Symfony2 stateless. Of course, it can be easily implemented in userland if we do not care about BC here.
Regarding different URLs per content, I do not think that this is our decision to make. Generally, developers should be able to make whatever content negotation they see fit. Whether they rely solely on the URL, or also take other request headers into account should not be limited by Symfony2.
---------------------------------------------------------------------------
by fabpot at 2012-07-02T10:37:26Z
I've added a paragraph in the UPGRADE file with a listener example that can be used to keep BC.
Before this commit, the current locale was stored in the session (if one
was already started). That way, for the next requests, even if the
request locale attribute was not set, the locale was "restored".
But this is a really bad practice as it means that the same URL can have
a different content depending on the previous requests. It would have
been better if the Vary header was set but the locale can be different
from the value coming from the Accept-Language anyway.
This is a BC break but fortunately, you can restore the 2.0 behavior by
creating a simple event listener that contains the logic removed by this
commit.
Commits
-------
4d09fe6 [Finder] '*' and '?' are considered are glob pattern rather than delimiters (fix#4664)
Discussion
----------
[Finder] '*' and '?' are considered are glob pattern rather than delimit...
...ers (fix#4664)