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.
The previous code allowed to pass null as a Request but that does not
really make sense as rendering a sub-request can only happen from a
master request. This was done to ease testing but that was a mistake.
This PR was merged into the master branch.
Commits
-------
731cd49 made twig extension service private
Discussion
----------
[TwigBundle] made extension a private service
| Q | A
| ------------- | ---
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | ~
| License | MIT
| Doc PR | ~
This PR was merged into the master branch.
Commits
-------
cdf1d72 [FrameworkBundle] fixed requirement of the _controller palceholder for the proxy route (closes#6783)
Discussion
----------
[FrameworkBundle] fixed requirement of the _controller palceholder for the proxy route (closes#6783)
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #6783
| License | MIT
| Doc PR | n/a
---------------------------------------------------------------------------
by vicb at 2013-01-18T10:23:06Z
What about a UT ?
---------------------------------------------------------------------------
by vicb at 2013-01-18T11:28:41Z
and the syntax is wrong also !
---------------------------------------------------------------------------
by gimler at 2013-01-21T19:59:57Z
same problem the sonata admin bundle use
```
{% render 'sonata.admin.controller.admin:getShortObjectDescriptionAction' %}
```
rewrite to
```
{% render controller('sonata.admin.controller.admin:getShortObjectDescriptionAction') %}
```
throws
```
An exception has been thrown during the rendering of a template ("Parameter "_controller" for route "_proxy" must match "[^/\.]++" ("sonata.admin.controller.admin:getShortObjectDescriptionAction" given) to generate a corresponding URL.") in "SonataAdminBundle:CRUD:edit.html.twig".
```
with the requirement fix it throws
```
An exception has been thrown during the rendering of a template ("Unable to parse the controller name "sonata".") in "SonataAdminBundle:CRUD:edit.html.twig".
```
---------------------------------------------------------------------------
by fabpot at 2013-01-22T06:40:14Z
ok, I've updated the patch. There is now a static segment (`/for`) between the controller and the format, which should fix the problem.
While thinking about this, there is another option, which might be even better: removing the need for the proxy route altogether and check for a defined path like `/_proxy`. It would remove the dependency on a Url Generator in the rendering strategy, and would not make the router proxy listener any more complex.
---------------------------------------------------------------------------
by gimler at 2013-01-22T07:20:43Z
+1 for me the patch works i will open a PR for sonata doctrine orm bundle
```
{% render controller('sonata.admin.controller.admin:getShortObjectDescriptionAction', {}, {
'code': sonata_admin.field_description.associationadmin.code,
'objectId': sonata_admin.field_description.associationadmin.id(sonata_admin.value),
'uniqid': sonata_admin.field_description.associationadmin.uniqid
})
```
---------------------------------------------------------------------------
by gimler at 2013-01-22T07:22:21Z
When the proxy route is nessesary we should add a note into the upgrade guide.
+1 for less complexesy
---------------------------------------------------------------------------
by fabpot at 2013-01-22T08:02:12Z
There is one issue with removing the proxy route: when generating a proxy URL, we need a Request instance, which is not always the case. I'm going to submit another PR to "fix" that first.
---------------------------------------------------------------------------
by vicb at 2013-01-22T08:17:51Z
> It would remove the dependency
Paul leaves this body :)
---------------------------------------------------------------------------
by Tobion at 2013-01-22T08:53:52Z
I don't think removing the proxy route is good. That's the purpose of the routing system to handle generating and matching. Now if you do it manually it will probably show a bad approach to people to handle such stuff.
Also people cannot see what routes are defined explicitly and use tools like `router:debug`.
---------------------------------------------------------------------------
by fabpot at 2013-01-22T09:28:55Z
@Tobion: see #6829
---------------------------------------------------------------------------
by fabpot at 2013-01-22T09:57:57Z
I've again changed the route pattern to avoid any possible problems (even if a controller contains a `/`).
---------------------------------------------------------------------------
by Tobion at 2013-01-22T10:16:03Z
Can a controller contain `/`? It's neither a valid service nor a valid class name or?
---------------------------------------------------------------------------
by mvrhov at 2013-01-22T10:40:26Z
AFAIK yes, at least I used Namespace/SubController more then once...
This PR was merged into the master branch.
Commits
-------
94f6116 renamed hostname to host in the routing system (closes#6775)
001734a [Validator] fixed phpdoc
18b9e68 [HttpFoundation] renamed hostname to host in the test to be consistent
Discussion
----------
Hostname rename to host
| Q | A
| ------------- | ---
| Bug fix? | no
| New feature? | no
| BC breaks? | no (does not exist in 2.1)
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #6775
| License | MIT
| Doc PR | symfony/symfony-docs#2165
see #6775
---------------------------------------------------------------------------
by WouterJ at 2013-01-21T18:44:20Z
+1
As explained in #6775, this has been done for the following reasons:
1. It's also Request::getHost()
2. The term hostname has been obsoleted in
http://tools.ietf.org/html/rfc3986#appendix-D.2 and uses the host only
3. hostname in the RFC was defined as the registered domain name, but we
probably also want to match IP-Adresses with the pattern which is the
host = IP-literal / IPv4address / reg-name for.
This PR was merged into the master branch.
Commits
-------
aadefd3 [HttpKernel] refactored the HTTP content renderer to make it easier to extend
Discussion
----------
[HttpKernel] refactored the HTTP content renderer to make it easier to extend
| Q | A
| ------------- | ---
| Bug fix? | no
| New feature? | kinda
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | n/a
| License | MIT
| Doc PR | n/a
This makes the StreamedResponse logic reusable for other strategies and it also makes the RenderingStrategy interface less fuzzy about its contract.
That also makes features like #4470 easier to implement from the outside.
---------------------------------------------------------------------------
by stof at 2013-01-20T11:01:29Z
👍
This PR was merged into the master branch.
Commits
-------
32322a1 [TwigBridge] fixed format_file to include the line number even if the link text is passed
Discussion
----------
[TwigBridge] fixed format_file to include the line number even if the link text is passed
| 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
That fixes the logs in the profiler where lines were not displayed for deprecated calls.
---------------------------------------------------------------------------
by stof at 2013-01-20T10:58:58Z
shouldn't this be merged in older branches as it is a bugfix ?
---------------------------------------------------------------------------
by fabpot at 2013-01-20T15:14:50Z
It is a bug fix but people might rely on the current behavior. But then, nobody ever reported it. So, I prefer to keep it on 2.2 only.
This PR was merged into the master branch.
Commits
-------
1ab48db tweaked the deprecated error handler to actually display the deprecation message
Discussion
----------
tweaked the deprecated error handler to actually display the deprecation message
| Q | A
| ------------- | ---
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | n/a
| License | MIT
| Doc PR | n/a
I've also removed uninterested information (like the trigger_error stack
frame).
This PR was merged into the master branch.
Commits
-------
fcabadf Fix JsonDecode to work on PHP 5.3, update the CHANGELOG.md
b6bdb45 Completly refactor the Serializer Options Pull Request to push context information directly and avoid state and dependencies between SerializerInterface and encoders/normalizers.
ef652e2 Added context to JsonEncoder
eacb7e2 Rename $options to $context, as it makes the intent much more clear.
8854b85 Fix CS issues, removed global options
9c54a4b [Serializer] Allow options to be passed to SerialiizerInterface#serialize and #unserialize. Thsee options are available to all encoders/decoders/normalizers that implement SerializerAwareInterface.
Discussion
----------
[2.2] [Serializer] Configurable Serializer
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #4907, #4938
License of the code: MIT
Todo:
This is an extension of GH-6574 that removes the context state in favor of passing this information around.
---------------------------------------------------------------------------
by beberlei at 2013-01-18T13:12:39Z
@fabpot @lsmith I think this is how it should work from an OOP/OOD perpesctive, avoiding the context state. This makes for a much cleaner code and dependency graph.
---------------------------------------------------------------------------
by lsmith77 at 2013-01-18T14:14:37Z
makes sense. anything fancier would lose this components simplicity which IMHO is the main benefit versus JMS serializer.
---------------------------------------------------------------------------
by fabpot at 2013-01-18T14:26:25Z
Looks very good. 👍
---------------------------------------------------------------------------
by beberlei at 2013-01-18T14:37:32Z
I need to fix the failures with the JsonEncoder and then this is good to merge
---------------------------------------------------------------------------
by stof at 2013-01-18T14:40:21Z
you also need to update the CHANGELOG of the component
---------------------------------------------------------------------------
by beberlei at 2013-01-18T23:17:57Z
Fixed, only the Redis Profiler problem still failing the Travis builds. Also I updated the CHANGELOG.md.
@fabpot Good to merge from my POV
---------------------------------------------------------------------------
by stof at 2013-01-18T23:27:59Z
@beberlei see #6804 for the Redis profiler issue
This PR was merged into the master branch.
Commits
-------
d5e73e5 [SecurityBundle] changed includes to use the new Twig notation
Discussion
----------
[SecurityBundle] changed includes to use the new Twig notation
| Q | A
| ------------- | ---
| Bug fix? | no
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | no - couple of unrelated failures
| Fixed tickets | N/A
| License | MIT
| Doc PR | N/A
This PR was merged into the master branch.
Commits
-------
3d762dd [HttpKernel] Fixed the Redis profiler storage return value
44fe249 Fixed some tests on Windows
Discussion
----------
Fix tests
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets |
| License | MIT
| Doc PR | n/a
This fixes the testsuite in master.
For the Redis storage, the test failure seems to come from a bad resolution when merging 2.1 into master.
---------------------------------------------------------------------------
by stof at 2013-01-18T20:58:59Z
I still have some failing tests in master locally:
- some tests with the incremental output tests in the Process component
- a timing test for the Stopwatch component (but passing when running only the Stopwatch tests)
- some mock expectations in the Form component.
However, all these tests are passing on Travis so it looks weird.
This PR was merged into the master branch.
Commits
-------
94cb13d [FrameworkBundle] fixed circular reference for service templating (closes#6730, closes#6762)
Discussion
----------
[FrameworkBundle] fixed circular reference for service templating (closes#6730, closes#6762)
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | #6730, #6762
| License | MIT
| Doc PR | n/a
---------------------------------------------------------------------------
by tristanbes at 2013-01-18T14:55:39Z
@fabpot Do you have an idea when you'll merge it please ?
This PR was squashed before being merged into the master branch (closes#6789).
Commits
-------
917f473 [Console] Removing unnecessary sprintf in Application->getHelp
Discussion
----------
[Console] Removing unnecessary sprintf in Application->getHelp
Minor change of the `Symfony\Component\Console\Application->getHelp()` method.
I have spotted:
1. an unnecessary `sprintf` call (no `args` arguments);
2. two ways of adding a new line in the help (an empty string as new entry of the array and a `\n` at the end of the string).
It seems to be there since the begining and it looks like a forgoten change to me, so I fixed them by removing the `sprintf` call and using a new array entry (empty string) instead of the `\n`.
| Q | A
| ------------- | ---
| License | MIT
* 2.1:
[Yaml] fixed unneeded BC break
[DependencyInjection] fixed a bug in the YAML dumper where references where not converted to the @ notation
[Yaml] fixed typo
updated VERSION for 2.1.7
updated CHANGELOG for 2.1.7
updated VERSION for 2.0.22
update CONTRIBUTORS for 2.0.22
updated CHANGELOG for 2.0.22
Conflicts:
src/Symfony/Component/HttpKernel/Kernel.php
* 2.0:
[Yaml] fixed unneeded BC break
[DependencyInjection] fixed a bug in the YAML dumper where references where not converted to the @ notation
[Yaml] fixed typo
updated VERSION for 2.0.22
update CONTRIBUTORS for 2.0.22
updated CHANGELOG for 2.0.22
Conflicts:
src/Symfony/Component/HttpKernel/Kernel.php
* 2.1:
[Yaml] fixed default value
Added Yaml\Dumper::setIndentation() method to allow a custom indentation level of nested nodes.
added a way to enable/disable object support when parsing/dumping
added a way to enable/disable PHP support when parsing a YAML input via Yaml::parse()
fixed CS
[Process] Fix docblocks, remove `return` from `PhpProcess#start()` as parent returns nothing, cleaned up `ExecutableFinder`
fixes a bug when output/error output contains a % character
[Console] fixed input bug when the value of an option is empty (closes#6649, closes#6689)
[Profiler] [Redis] Fix sort of profiler rows.
Fix version_compare() calls for PHP 5.5.
Removed underscores from test method names to be consistent with other components.
[Process] In edge cases `getcwd()` can return `false`, then `proc_open()` should get `null` to use default value (the working dir of the current PHP process)
Fix version_compare() calls for PHP 5.5.
Handle the deprecation of IntlDateFormatter::setTimeZoneId() in PHP 5.5.
removed the .gitattributes files (closes#6605, reverts #5674)
[HttpKernel] Clarify misleading comment in ExceptionListener
Conflicts:
src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/toolbar_style.html.twig
src/Symfony/Component/Form/Tests/Extension/Core/Type/DateTimeTypeTest.php
src/Symfony/Component/Form/Tests/Extension/Core/Type/TimeTypeTest.php
src/Symfony/Component/Form/Tests/Util/PropertyPathTest.php
src/Symfony/Component/HttpKernel/Profiler/RedisProfilerStorage.php
src/Symfony/Component/Process/Process.php
* 2.0:
[Yaml] fixed default value
Added Yaml\Dumper::setIndentation() method to allow a custom indentation level of nested nodes.
added a way to enable/disable object support when parsing/dumping
added a way to enable/disable PHP support when parsing a YAML input via Yaml::parse()
[Console] fixed input bug when the value of an option is empty (closes#6649, closes#6689)
Conflicts:
src/Symfony/Component/Yaml/Dumper.php
src/Symfony/Component/Yaml/Parser.php
src/Symfony/Component/Yaml/Tests/DumperTest.php
src/Symfony/Component/Yaml/Tests/ParserTest.php
src/Symfony/Component/Yaml/Yaml.php
