This PR was merged into the master branch.
Commits
-------
ed3a875 [Finder] Fixed test
Discussion
----------
[Finder] Fixed test
Fixed tests broken by PR #6911.
Iterator keys were considered as incremented index in `FilePathsIteratorTest`.
This PR was merged into the master branch.
Commits
-------
ef593fa [Finder] Fixed iterator keys
d16e28a [Finder] Added iterator keys test
Discussion
----------
[Finder] Fixes iterator keys
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #6891
| License | MIT
Previous PR I did on finder introduced a BR break (finder iterator keys must be the pathname of the file, not an incremented index). This PR adds a test to ensure this wont be broken again and fixes the BC break.
This PR was squashed before being merged into the master branch (closes#6852).
Commits
-------
fde7585 [DIC] Better handling of enableable configurations
Discussion
----------
[DIC] Better handling of enableable configurations
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no, this feature has not been released yet
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | -
| License | MIT
| Doc PR | -
My definition of bug fix might be discussable. The thing which I think is not discussable is that this PR fixes the semantic - and I think it is important for a "semantic configuration": before this PR, some nodes had `->canBeDisabled` for nodes that were actually disabled by default. Those nodes now have `->canBeEnabled` which sounds right.
**Edit: Jan 28, 2013** - history:
See [the related comments](https://github.com/symfony/symfony/pull/6829#discussion_r2727742).
I think Symfony **must** get the configuration right as we can expect of lot of devs to use this as a template when writting their own configuration.
@schmittjoh could you please give me your feedback on [this change](https://github.com/symfony/symfony/pull/6852/files#L4R224) considering [the rationale](https://github.com/symfony/symfony/pull/6852/files#L3R7).
---------------------------------------------------------------------------
by stof at 2013-01-23T16:10:33Z
@vicb your links are broken as they are pointing to the PR creation page
---------------------------------------------------------------------------
by stof at 2013-01-23T16:10:55Z
and to create a TODO list, it has to be a list first
---------------------------------------------------------------------------
by vicb at 2013-01-23T16:31:10Z
@stof thanks for reporting the broken links, they are fixed /cc @schmittjoh
---------------------------------------------------------------------------
by vicb at 2013-01-23T16:31:50Z
@Tobion please submit a PR to my repo, I don't have much time to work on this. Thanks !
---------------------------------------------------------------------------
by vicb at 2013-01-25T15:14:47Z
@fabpot @schmittjoh I'd like your feedback on the latest commit, rationale is in the method phpDoc. It better matches what we do now and seem the most sensible thing to do.
edit: with this you can no more disable the node explicitly, I have to find a better solution
---------------------------------------------------------------------------
by schmittjoh at 2013-01-25T15:20:13Z
Looks good.
On Fri, Jan 25, 2013 at 4:15 PM, Victor Berchet <notifications@github.com>wrote:
> @fabpot <https://github.com/fabpot> @schmittjoh<https://github.com/schmittjoh>I'd like your feedback on the latest commit, rationale is in the method
> phpDoc. It better matches what we do now and seem the most sensible thing
> to do.
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/symfony/symfony/pull/6852#issuecomment-12704585>.
>
>
---------------------------------------------------------------------------
by vicb at 2013-01-28T14:37:57Z
@fabpot I know I keep insisting on this one and I am sorry for that but I think this should be considered as a bug fix (see the PR header for details) and should be merged in 2.2. I think the Symfony core should be exemplary as it is used by many developers as a template when creating their own bundle. *This PR is no more a WIP and can be merged right now*.
In addition to fixing the enableable nodes, this PR contain new UTs and some fixes to the code / tests.
---------------------------------------------------------------------------
by fabpot at 2013-01-28T16:43:42Z
@vicb As explained in a comment, this is not a BC break as this feature does not exist in 2.1. So, I can make the change to the CHANGELOG if you want after merging, or I can let you make the change.
---------------------------------------------------------------------------
by vicb at 2013-01-28T16:46:33Z
I am going to change it right now !
---------------------------------------------------------------------------
by vicb at 2013-01-28T16:46:56Z
(and thanks for having checked this)
---------------------------------------------------------------------------
by vicb at 2013-01-28T16:54:37Z
@fabpot I have updated the changelog and the PR header.
I am not sure if the commits should be squashed or not. On one side the multiple commits can help understand the changes but on the other side that's a lot of small commits which could pollute history. I let you choose what to do.
This PR was squashed before being merged into the master branch (closes#6893).
Commits
-------
7cff461 [Routing] Improve RouteNotFoundException messages
Discussion
----------
[Routing] Improve RouteNotFoundException messages
| Q | A
| ------------- | ---
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | Travis?
| Fixed tickets | -
| License | MIT
| Doc PR | -
The error message that is generated when a named route is not found can be a little confusing in the sense that one could think the name actually references a path for the route. This PR should make that a little clearer.
See [this Laravel ticket](https://github.com/laravel/framework/issues/177) for reference.
---------------------------------------------------------------------------
by franzliedke at 2013-01-28T09:44:43Z
Okay folks, I changed those. Thank you!
This PR was submitted for the master branch but it was merged into the 2.1 branch instead (closes#6884).
Commits
-------
c261760 Fixed missing class argument when throwing exception
Discussion
----------
Added missing parameter to Exception in DebugClassLoader
The DebugClassLoader was missing the $class argument to sprintf() when it tries to throw the exception, making the actual error you are getting more daunting :)
---------------------------------------------------------------------------
by stof at 2013-01-25T23:31:58Z
oops, sorry.
@fabpot this should be merged in 2.1 as you merged the change there
This PR was merged into the master branch.
Commits
-------
e3086cc [Console] added some missing information in the phpdoc (closes#6464)
Discussion
----------
[Console] added some missing information in the phpdoc (closes#6464)
| Q | A
| ------------- | ---
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #6464
| License | MIT
| Doc PR | n/a
This PR was merged into the master branch.
Commits
-------
ed64413 removed deprecated message in FieldType
Discussion
----------
removed deprecated messages coming for internal calls
| Q | A
| ------------- | ---
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #6407
| License | MIT
| Doc PR | n/a
It's probably not the cleanest code possible, but I don't see any other way to keep the type for BC and avoid the deprecated message when called internally.
That should fix the deprecated messages thrown by the Form and Validator components.
---------------------------------------------------------------------------
by stof at 2013-01-24T15:41:26Z
The cases where you should actually be warned about the deprecation of FieldType is when using ``field`` to create a field with the factory (which would not trigger the constructor)
---------------------------------------------------------------------------
by fabpot at 2013-01-24T15:46:23Z
@stof: what do you mean? That we can remove the `trigger_error()` call altogether?
---------------------------------------------------------------------------
by stof at 2013-01-24T15:49:31Z
Nobody will ever instantiate the FieldType directly as the framework is registering it (btw, you are missing the FrameworkBundle lazy registration here).
The constructor of a form type is not the right place to deprecate it as registering it does not mean it will be used in the app.
---------------------------------------------------------------------------
by fabpot at 2013-01-24T15:51:26Z
@stof: I've updated the PR to remove the `trigger_error` call.
This PR was merged into the master branch.
Commits
-------
749087e updated ValidatorExtension to avoid using a deprecated method
Discussion
----------
updated ValidatorExtension to avoid using a deprecated method
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | n/a
| License | MIT
| Doc PR | n/a
---------------------------------------------------------------------------
by stof at 2013-01-24T13:05:43Z
👍
Btw, this is needed to simply make it work against the ValidatorInterface by having the right expectation on getMetadataFactory()
This PR was merged into the master branch.
Commits
-------
8bfd7a7 Remove custom error handler for deprecations
efbff0c Change deprecated Min and Max constraint
c47f027 [Validator][Constraints] Update AllValidator using new Validator API
Discussion
----------
[Validator][Constraints] Update AllValidator using new Validator API
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets:
License of the code: MIT
This PR was merged into the master branch.
Commits
-------
9264431 Remove custom error handler for deprecations
c80c17e Change deprecated Min constraint to Range constraint
a39fdd8 Remove getPropertyPath in the tests
ffa4c6f [Validator][Constraints] CollectionValidator
Discussion
----------
[Validator][Constraints] Update CollectionValidator using new Validator API
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #6648
License of the code: MIT
In the tests `$i = 1` is because in `validate` the first call to `$this->context` is `$this->context->getGroup();`, should I write this in a comment before each declaration?
---------------------------------------------------------------------------
by stof at 2013-01-15T21:50:41Z
You should also modify the CollectionValidator tests to use a different constraint than ``Min`` which is deprecated. Once it is done, these tests should not need to register a custom error handler catching deprecations anymore
---------------------------------------------------------------------------
by franmomu at 2013-01-15T22:33:43Z
Perfect, I'm going to do the same in #6743
This PR was merged into the master branch.
Commits
-------
2061cc0 Update src/Symfony/Component/Validator/Mapping/ClassMetadata.php
Discussion
----------
[Validator] ClassMetadata use deprecated methods
Bug fix: yes
Feature addition: no
Backwards compatibility break: maybe yes (I don't have the knowlegde)
Symfony2 tests pass: yes
Fixes the following tickets: ?
Todo: Nothing
License of the code: MIT
Documentation PR: Nothing
getValue() is deprecated since version 2.2 and will be removed in 2.3. Use getPropertyValue() instead.
ClassMetadata is still using the deprecated method, changed it to getPropertyValue to prevent a trigger error.
---------------------------------------------------------------------------
by fabpot at 2013-01-14T15:35:28Z
ping @bschussek
* 2.1:
[DependencyInjection] fixed the creation of synthetic services in ContainerBuilder
[Security] PHPDoc in SecurityEvents
Fix typos in README
Added an error message in the DebugClassLoader when using / instead of \.
KNOWN_ISSUES with php 5.3.16
[FrameworkBundle] fixed Client::doRequest that must call its parent method (closes#6737)
[Yaml] fixed ignored text when parsing an inlined mapping or sequence (closes#6786)
[Yaml] fixed#6773
[Yaml] fixed#6770
bumped Symfony version to 2.1.8-DEV
bumped Symfony version to 2.0.23-DEV
Conflicts:
src/Symfony/Bundle/FrameworkBundle/Client.php
src/Symfony/Component/HttpKernel/Kernel.php
This PR was squashed before being merged into the master branch (closes#6734).
Commits
-------
4d51ec0 Fix for hardcode (#6384) in choice widget
Discussion
----------
Fix for hardcode (#6384) in choice widget
empty_value should not be disabled if field is not required!
#6384
---------------------------------------------------------------------------
by sstok at 2013-01-15T15:51:08Z
You need to revert the file mode changes (100644 → 100755)
---------------------------------------------------------------------------
by MaksSlesarenko at 2013-01-18T16:44:42Z
fixed tests
---------------------------------------------------------------------------
by MaksSlesarenko at 2013-01-21T15:36:59Z
ping @fabpot
---------------------------------------------------------------------------
by fabpot at 2013-01-21T15:58:26Z
ping @bschussek
---------------------------------------------------------------------------
by MaksSlesarenko at 2013-01-23T11:15:37Z
ping @fabpot @bschussek
---------------------------------------------------------------------------
by Tobion at 2013-01-23T12:08:19Z
I think it's good to squash and merge.
---------------------------------------------------------------------------
by fabpot at 2013-01-23T12:16:37Z
Can you rebase and squash before I merge? Thanks.
---------------------------------------------------------------------------
by MaksSlesarenko at 2013-01-23T19:51:36Z
@fabpot done
This PR was merged into the master branch.
Commits
-------
7944860 [DIC] Move PrependExtensionInterface to the Extension namespace
Discussion
----------
[DIC] Move PrependExtensionInterface to the Extension namespace
@fabpot Please merge before 2.2 (no BC break) /cc @lsmith77
This PR was merged into the master branch.
Commits
-------
65b4112 fixed a circular reference (closes#6730)
Discussion
----------
fixed a circular reference (closes#6730)
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #6730
| License | MIT
| Doc PR | n/a
---------------------------------------------------------------------------
by stof at 2013-01-23T13:56:19Z
shoudln't this be moved to the component ? Someone using the TwigBridge, HttpKernel and the DI component outside the full stack framework (let's say Drupal maybe) would also face the circular reference issue
---------------------------------------------------------------------------
by fabpot at 2013-01-23T13:58:28Z
No, they won't as the problem is only if you are using the templating component. So, Silex or Drupal won't have the problem.
---------------------------------------------------------------------------
by stof at 2013-01-23T14:01:03Z
ah, the issue is indeed with the TwigEngine, not with the Twig_Environment service.
This PR was merged into the master branch.
Commits
-------
23f5145 renamed proxy to router_proxy
e5135f6 [HttpKernel] renamed path to _path to avoid collision
3193a90 made the proxy path configurable
ad82893 removed the need for a proxy route for rendering strategies
b9f0e17 [HttpKernel] made the Request required when using rendering strategies
Discussion
----------
Content renderer simplification
| Q | A
| ------------- | ---
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | n/a
| License | MIT
| Doc PR | symfony/symfony-docs#2179
Todo:
- [x] submit a PR for documentation update
The first commit makes the Request required when dealing with rendering strategies (see the commit why this was a bad idea to make it optional in the first place).
The second commit removes the need for a proxy route and replaces it with the same system we have in the security component.
The third commit makes the proxy path configurable (default to `/_proxy`).
This PR has been triggered by a discussion on #6791.
---------------------------------------------------------------------------
by fabpot at 2013-01-22T09:49:37Z
My opinion:
* The first commit should be merged.
* For the second and third one, I don't have a strong opinion. One of the benefits that the content renderer and its strategies do not rely on the Routing component anymore.
---------------------------------------------------------------------------
by fabpot at 2013-01-22T15:22:47Z
Any comments? ping @Tobion @vicb
---------------------------------------------------------------------------
by Tobion at 2013-01-22T16:07:15Z
Shouldn't the class name be like `SubRequestRenderingStrategyInterface` because currently it does not say anything about what is rendered. `RenderingStrategyInterface` makes it look like it's for rendering a normal master request, i.e. templating.
---------------------------------------------------------------------------
by fabpot at 2013-01-22T16:19:26Z
@Tobion: This was actually the first name I had but I found it too long. It is indeed rendering a normal request, but only in the context of a master request.
---------------------------------------------------------------------------
by Tobion at 2013-01-22T16:23:25Z
I found the correct term for what this is about: http://en.wikipedia.org/wiki/Transclusion
So it should probably be like `Symfony/Component/HttpKernel/Transclusion/TransclusionInterface`
or `TransclusionStrategyInterface`.
So the term `rendering` is misleading as it does not really render the subrequest, but only prints a reference to the subrequest. The rendering is done by ESI processor or hinclude etc.
---------------------------------------------------------------------------
by fabpot at 2013-01-22T16:37:00Z
The `RenderingStrategyInterface` does render a request and returns a Response. One of the strategy consist of returning an ESI tag, but this is still a Response.
I don't like introducing the `Transclusion` word as (at least for me) it does not evoke anything.
---------------------------------------------------------------------------
by Tobion at 2013-01-22T16:46:10Z
Also `DefaultRenderingStrategy` is not saying anything. What is default? It should express that it directly includes the resource in the other (term `integrate` comes to my mind).
---------------------------------------------------------------------------
by kriswallsmith at 2013-01-22T17:23:21Z
How about `InlineRenderingStrategy`?
---------------------------------------------------------------------------
by vicb at 2013-01-22T17:25:17Z
@Tobion @kriswallsmith 👍
---------------------------------------------------------------------------
by kriswallsmith at 2013-01-22T17:26:17Z
Also, `SubRequestStrategyInterface` may be more apparent (`InlineSubRequestStrategy`, `EsiSubRequestStrategy`…)
---------------------------------------------------------------------------
by Tobion at 2013-01-22T18:10:19Z
`SubRequestStrategyInterface` is missing the verb somehow. A strategy for what? @kriswallsmith as an English native speaker, is transclusion also not convenient for you?
---------------------------------------------------------------------------
by fabpot at 2013-01-22T18:11:41Z
Thanks for all your suggestions, I appreciate them, but what about the whole approach? Do you agree that it is better than the current one? I'd like to avoid the bikeshedding if the approach is not better.
---------------------------------------------------------------------------
by kriswallsmith at 2013-01-22T18:22:47Z
👍 for removing the router dependency.
@Tobion perhaps request is the verb there?
---------------------------------------------------------------------------
by Tobion at 2013-01-22T19:51:20Z
I'm also fine with making it independent from the routing component because routing is about making routes configurable with placeholders etc. for nice URLs. But this is not needed for a proxy feature.
---------------------------------------------------------------------------
by kriswallsmith at 2013-01-22T20:13:48Z
@fabpot Do you anticipate ever wanting a sub request to be handled differently based on HTTP method? Just thinking of possible reasons to continue using the routing component here…
---------------------------------------------------------------------------
by fabpot at 2013-01-22T20:40:06Z
No, sub-requests only make sense for GET requests. In fact, we even enforce that in HttpContentRenderer.
---------------------------------------------------------------------------
by fabpot at 2013-01-23T06:51:54Z
I'm not going to discuss names further in the context of this PR as it has already been discussed in the initial PR that introduced these classes and because this PR does not change anything to the meaning of these classes. If you think the names can be better, feel free to open a ticket and discuss names there.
---------------------------------------------------------------------------
by vicb at 2013-01-23T07:48:36Z
If I understand correctly, both hsi and esi will generate path starting with "/_proxy", isn't it a problem wrt access_control ? should it be possible to configure a per strategy path ?
---------------------------------------------------------------------------
by fabpot at 2013-01-23T07:56:11Z
@vicb: Yes, all strategies use the `/_proxy` path when the developer uses a controller reference. But the router proxy takes care of securing the route, so there is no need to do it yourself.
---------------------------------------------------------------------------
by vicb at 2013-01-23T08:07:36Z
@fabpot that's smart, I've missed it.
Some questions though (they should be answered by UT I think - and might already have been, I have not checked)
- Isn't there a pb with urlencoding in the listener ?
- Would the listener work with fragments (`#...') ?
---------------------------------------------------------------------------
by vicb at 2013-01-23T08:31:37Z
Some more points:
- Should we validate that the router_proxy is enabled when esi are enabled (early failure ?)
- Should we be able to enable each strategy individually (ie no need to expose the signer when hsi are not used)
---------------------------------------------------------------------------
by fabpot at 2013-01-23T09:58:45Z
Enabling the router proxy when using ESI si not required. The router proxy is "only" required when you use the `controller` Twig function (or the equivalent in PHP -> `ControllerReference`). But we can probably throw an exception in the `ControllerReference` constructor if the proxy is not enabled.
Enabling each strategy individually is indeed a good idea (and that's more a general question as we could do the same for the translator loaders, or the service container loaders). Let's create another issue on this global topic.
---------------------------------------------------------------------------
by vicb at 2013-01-23T10:10:29Z
> But we can probably throw an exception in the ControllerReference constructor if the proxy is not enabled.
It should probably be in a wrapper class then ?
---------------------------------------------------------------------------
by fabpot at 2013-01-23T12:45:36Z
The listener does not need to work with fragments as URLs as they are never part of the generated URL.
