This PR was merged into the master branch.
Commits
-------
d7a1154 make it possible for bundles extensions to prepend settings into the application configuration of any Bundle
Discussion
----------
[2.2] add possibility for bundles extensions to prepend the app configs
Bug fix: #4652
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
As can be seen in the patch the extensions that should prepend the configuration are enabled automatically if they implement ``PrependExtensionInterface``.
Just as an example, here an extension, which checks if SonataAdminBundle is available and if not disables integration with it in several Bundles. It also sets some default settings for ``document_class`` and ``default_document_manager_name``:
```
diff --git a/DependencyInjection/SymfonyCmfCoreExtension.php b/DependencyInjection/SymfonyCmfCoreExtension.php
index 9f92410..c0a8dbb 100644
--- a/DependencyInjection/SymfonyCmfCoreExtension.php
+++ b/DependencyInjection/SymfonyCmfCoreExtension.php
@@ -3,11 +3,12 @@
namespace Symfony\Cmf\Bundle\CoreBundle\DependencyInjection;
use Symfony\Component\HttpKernel\DependencyInjection\Extension;
+use Symfony\Component\\DependencyInjection\PrependExtensionInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
use Symfony\Component\Config\FileLocator;
-class SymfonyCmfCoreExtension extends Extension
+class SymfonyCmfCoreExtension extends Extension implements PrependExtensionInterface
{
public function load(array $configs, ContainerBuilder $container)
{
@@ -15,4 +16,45 @@ class SymfonyCmfCoreExtension extends Extension
$loader->load('config.xml');
$loader->load('services.xml');
}
+
+ public function prepend(ContainerBuilder $container)
+ {
+ $bundles = $container->getParameter('kernel.bundles');
+ if (!isset($bundles['SonataDoctrinePHPCRAdminBundle'])) {
+ // disable SonataDoctrinePHPCRAdminBundle admin support in Bundles
+ $config = array('use_sonata_admin' => false);
+ foreach ($container->getExtensions() as $name => $extension) {
+ switch ($name) {
+ case 'symfony_cmf_menu':
+ case 'symfony_cmf_routing_extra':
+ case 'symfony_cmf_simple_cms':
+ $container->prependExtensionConfig($name, $config);
+ break;
+ }
+ }
+ }
+
+ // process the configuration of SymfonyCmfCoreExtension
+ $configs = $container->getExtensionConfig($this->getAlias());
+ $config = $this->processConfiguration(new Configuration(), $configs);
+ // add the default configs to various Bundles
+ foreach ($container->getExtensions() as $name => $extension) {
+ switch ($name) {
+ case 'symfony_cmf_content':
+ case 'symfony_cmf_simple_cms':
+ $container->prependExtensionConfig($name, $config);
+ break;
+ }
+ }
+ }
}
```
---------------------------------------------------------------------------
by stof at 2012-09-21T21:10:00Z
I think you are giving too much power to bundles here: a bundle becomes able to modify all the config defined explicitly by the user if it wants to do it.
I think it would be safer to give them the possibility to load an additional config file which would be prepended (so that user-defined config would still win). Giving the ability to load files means passing the loader used by the kernel, and it should then be called before calling the load method on the kernel itself (to respect the order of loaded files)
---------------------------------------------------------------------------
by lsmith77 at 2012-09-22T05:50:08Z
Not sure how a config file helps solve anything. I mean they can load as many config files as they want already. The key is being able to automatically apply configuration to multiple Bundles as well as enabling/disabling features based on if certain Bundles are registered.
BTW the end result in my examples is also prepended, so that user config wins. However yes this would be up to the person implementing the Bundle. We could however provide a dedicated method for prepending in addition to or instead of ``setExtensionConfig``.
---------------------------------------------------------------------------
by stof at 2012-09-22T11:40:29Z
@lsmith77 If you can load a file with the main loader, this file can provide some app-level configuration (be it for your own bundle or others).
And your code example is indeed prepending. But imagine what would occur when someone uses this feature without knowing well how the component works: he will likely call ``setExtensionConfig`` in a first implementation, thus dropping all userland config for the bundle. Your setup does not only allow to make a file win over the userland config but makes it even easier to remove the userland config.
---------------------------------------------------------------------------
by lsmith77 at 2012-09-22T18:11:29Z
but i dont get how that would help. the point is to be able for one bundle to configure other bundles before the load as this is obviously alot cleaner than trying to do the same via a compiler pass. so imho this is what is needed to encourage decoupled bundles. otherwise for example CMS or other reuseable and extensible apps will be forced to always put everything in one bundle.
---------------------------------------------------------------------------
by stof at 2012-09-22T19:23:45Z
@lsmith77 I agree about the feature, not about the way to implement it. If you allow bundles to load a file as it it were some app-level config, they would become able to provide some config for other bundles (and you could load several files depending of which bundles are enabled), but without allowing bundles to remove the userland config.
---------------------------------------------------------------------------
by lsmith77 at 2012-09-22T19:50:19Z
sorry but i dont understand what you suggest. more over i dont see the problem. its already possible to seriously break stuff with compiler passes which cannot be easily enabled/disabled. this is just convenience. if it doesnt work because of some obscure combo then simply dont use it for the app since it needs to be explicitly enabled in the kernel.
---------------------------------------------------------------------------
by lsmith77 at 2012-09-24T09:25:10Z
@stof thought about your comments, are you suggesting for a Bundle to be able to generate a config file that is prepended? in that case the current behavior would already be that if we change ``setExtensionConfig`` to just be a ``prependExtensionConfig`` .. however i am not sure if we really need this limitation since as i point out this would still by far be less dangerous than compiler passes and also i expect this to be used mainly by open source applications on top of Symfony2 rather than standard bundles.
---------------------------------------------------------------------------
by lsmith77 at 2012-10-13T13:28:29Z
@lolautruche i also think this is relevant for you guys. this way you could start preconfiguring 3rd party bundles as part of your main ezPublish bundle.
---------------------------------------------------------------------------
by lolautruche at 2012-10-13T13:57:09Z
While I suspect a nice feature, the implementation looks obscure to me...
---------------------------------------------------------------------------
by lsmith77 at 2012-10-13T17:43:02Z
The implementation of the example extension or the implementation of the actual changes proposed in this PR?
---------------------------------------------------------------------------
by lolautruche at 2012-10-13T17:46:57Z
The example, sorry 😃
---------------------------------------------------------------------------
by lsmith77 at 2012-10-13T17:50:38Z
The example was fairly quickly hacked together. The basic thing you need to do is fetch the config for the bundle you want to change, manipulate the config (usually by appending an array to the array of configs so that you dont affect explicit configuration) and then set it again.
As I explained to @stof it would alternative/additionally be possible to support a method that pushes a config array to the top of the array of config stack. Such a method might make the necessary code simpler.
---------------------------------------------------------------------------
by stof at 2012-10-13T21:39:07Z
@fabpot what do you think about it ?
---------------------------------------------------------------------------
by jrobeson at 2012-10-20T15:45:18Z
I've been porting much of an existing framework over to use more symfony components and bundles. I think that this might help some of the problems i've been having. I would really appreciate some better examples as how to one would use it (same for the cmf router, but that's another story).
---------------------------------------------------------------------------
by lsmith77 at 2012-10-21T07:28:52Z
not really sure what other examples i could give. the process is quite simple:
1) determine what configuration options to add to other Bundles
for example with the following code I determine that SonataAdmin for PHPCR is not installed (which means i should disable using it in other Bundles):
```
$bundles = $container->getParameter('kernel.bundles');
if (!isset($bundles['SonataDoctrinePHPCRAdminBundle'])) {
```
alternatively I could simply already process the configuration and then pick all or some of these configuration options:
```
$configs = $container->getExtensionConfig($this->getAlias());
$config = $this->processConfiguration(new Configuration(), $configs);
```
2) then add these configuration to what other Bundles I feel should get these options
usually I will add these to the top of the config array stack. this means that if the user would manually set the same setting in most cases the user setting will override what the pre-processor set.
```
$container->unshiftExtensionConfig($name, array('use_sonata_admin' => false));
```
---------------------------------------------------------------------------
by lsmith77 at 2012-10-24T12:52:38Z
added ``ContainerBuilder::unshiftExtensionConfig`` since this is the usual use case. with this method added it could be discussed if ``ContainerBuilder::setExtensionConfig`` is still needed or not.
---------------------------------------------------------------------------
by lsmith77 at 2012-11-24T14:48:44Z
I spoke to @fabpot today and he said that since this patch just allows you to set defaults and not really "process" the actual configuration I shouldn't call it "preProcess" so I renamed it to "prepend".
Furthermore as its just prepending @fabpot said there isnt really a need to require manually enabling it, so here is a patch to auto-enable the prepending logic:
```
diff --git a/src/Symfony/Component/HttpKernel/Kernel.php b/src/Symfony/Component/HttpKernel/Kernel.php
index b890fbf..7374b87 100644
--- a/src/Symfony/Component/HttpKernel/Kernel.php
+++ b/src/Symfony/Component/HttpKernel/Kernel.php
@@ -701,8 +701,8 @@ abstract class Kernel implements KernelInterface, TerminableInterface
*/
protected function prependExtensionConfigs(ContainerBuilder $container)
{
- foreach ($this->getPrependingExtensions() as $name) {
- $extension = $container->getExtension($name);
+ foreach ($this->bundles as $bundle) {
+ $extension = $bundle->getContainerExtension();
if ($extension instanceof PrependExtensionInterface) {
$extension->prepend($container);
}
@@ -710,16 +710,6 @@ abstract class Kernel implements KernelInterface, TerminableInterface
}
/**
- * Returns the ordered list of extensions that may prepend extension configurations.
- *
- * @return array
- */
- protected function getPrependingExtensions()
- {
- return array();
- }
-
- /**
* Gets a new ContainerBuilder instance used to build the service container.
*
* @return ContainerBuilder
```
---------------------------------------------------------------------------
by lsmith77 at 2012-11-25T19:31:01Z
ok .. i pondered the code some more and now i have enabled registering of the prepending extensions by default, since its now quite easy to just override the ``prependExtensionConfigs()`` method since there is almost no logic in there anymore.
@fabpot i am not 100% sure with the naming yet ..
---------------------------------------------------------------------------
by lsmith77 at 2012-12-05T14:03:43Z
@fabpot if you are ok with the PR as it is now, i can do the rebase so you can merge this?
---------------------------------------------------------------------------
by lsmith77 at 2012-12-05T18:30:29Z
@fabpot all good now? then i will squash the commits ..
---------------------------------------------------------------------------
by lsmith77 at 2012-12-05T18:34:50Z
actually looking at the full change set again i am no longer sure if it makes sense to have ``PrependExtensionInterface`` in the DI rather than the HttpKernel.
---------------------------------------------------------------------------
by lsmith77 at 2012-12-07T09:21:14Z
@fabpot all good now?
---------------------------------------------------------------------------
by fabpot at 2012-12-07T09:37:52Z
The code looks good to me now. There are two remaining task before merging:
* Is it something we need to add somewhere in the documentation?
* Can you add a note in the DI component CHANGELOG?
Thanks.
---------------------------------------------------------------------------
by lsmith77 at 2012-12-07T09:49:17Z
i have added a changelog entry and squashed the commits.
i will also work on a documentation entry, i guess i will make it a cookbook entry. not sure if it should be included in http://symfony.com/doc/2.0/cookbook/bundles/extension.html .. but imho it would better be a separate entry.
* 2.1:
[HttpFoundation] changed UploadedFile::move() to use move_uploaded_file() when possible (closes#5878, closes#6185)
[HttpFoundation] added a check for the host header value
[DoctrineBridge] Improved performance of the EntityType when used with the "query_builder" option
[DoctrineBridge] Improved exception message
[DoctrineBridge] Fixed: Exception is thrown if the entity class is not known to Doctrine
Removed useless branch alias for dev-master in composer.json
Conflicts:
composer.json
src/Symfony/Bridge/Doctrine/composer.json
src/Symfony/Bridge/Monolog/composer.json
src/Symfony/Bridge/Propel1/composer.json
src/Symfony/Bridge/Swiftmailer/composer.json
src/Symfony/Bridge/Twig/composer.json
src/Symfony/Bundle/FrameworkBundle/composer.json
src/Symfony/Bundle/SecurityBundle/composer.json
src/Symfony/Bundle/TwigBundle/composer.json
src/Symfony/Bundle/WebProfilerBundle/composer.json
src/Symfony/Component/BrowserKit/composer.json
src/Symfony/Component/ClassLoader/composer.json
src/Symfony/Component/Config/composer.json
src/Symfony/Component/Console/composer.json
src/Symfony/Component/CssSelector/composer.json
src/Symfony/Component/DependencyInjection/composer.json
src/Symfony/Component/DomCrawler/composer.json
src/Symfony/Component/EventDispatcher/composer.json
src/Symfony/Component/Filesystem/composer.json
src/Symfony/Component/Finder/composer.json
src/Symfony/Component/Form/composer.json
src/Symfony/Component/HttpFoundation/composer.json
src/Symfony/Component/HttpKernel/composer.json
src/Symfony/Component/Locale/composer.json
src/Symfony/Component/OptionsResolver/composer.json
src/Symfony/Component/Process/composer.json
src/Symfony/Component/Routing/composer.json
src/Symfony/Component/Security/composer.json
src/Symfony/Component/Serializer/composer.json
src/Symfony/Component/Templating/composer.json
src/Symfony/Component/Translation/composer.json
src/Symfony/Component/Validator/composer.json
src/Symfony/Component/Yaml/composer.json
* 2.0:
[HttpFoundation] changed UploadedFile::move() to use move_uploaded_file() when possible (closes#5878, closes#6185)
[HttpFoundation] added a check for the host header value
Conflicts:
src/Symfony/Component/HttpFoundation/File/File.php
src/Symfony/Component/HttpFoundation/Request.php
src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
This PR was merged into the 2.0 branch.
Commits
-------
447ff91 [HttpFoundation] changed UploadedFile::move() to use move_uploaded_file() when possible (closes#5878, closes#6185)
Discussion
----------
[HttpFoundation] changed UploadedFile::move() to use move_uploaded_file() when possible (closes#5878, closes#6185)
An alternative for #5878 and it fixes#6185.
This PR was merged into the 2.0 branch.
Commits
-------
0489799 [HttpFoundation] added a check for the host header value
Discussion
----------
[HttpFoundation] added a check for the host header value
alternative for #3865
This PR was merged into the master branch.
Commits
-------
459a09f [WebProfilerBundle] "View all" is "View last 10"
Discussion
----------
[WebProfilerBundle] "View all" is "View last 10"
Change a misleading link description
This PR was squashed before being merged into the master branch (closes#6005).
Commits
-------
577ee80 [HttpFoundation] Move IP check methods to a HttpUtils class for reuse
Discussion
----------
[HttpFoundation] Move IP check methods to a HttpUtils class for reuse
---------------------------------------------------------------------------
by vicb at 2012-11-13T18:05:18Z
Thanks @stof ! (didn't get my copy paste error as PHP allow calling non static method w/o a warning).
---------------------------------------------------------------------------
by GromNaN at 2012-11-17T23:19:29Z
Having an `Utils` class with mixed functions doesn't seem to be a good practice. I think the class should be called something like `Symfony\Component\HttpFoundation\IpAddress`.
---------------------------------------------------------------------------
by vicb at 2012-11-27T09:37:20Z
@fabpot could this be merged if `HttpUtils` is renamed to `IpUtils` ?
---------------------------------------------------------------------------
by fabpot at 2012-12-06T13:35:28Z
Renaming the class to `IpUtils` is indeed a good idea.
---------------------------------------------------------------------------
by vicb at 2012-12-06T14:07:59Z
ready !
---------------------------------------------------------------------------
by fabpot at 2012-12-06T14:39:19Z
Can you add an entry in the CHANGELOG?
---------------------------------------------------------------------------
by vicb at 2012-12-06T14:53:09Z
done, thanks for the reminder !
This PR was merged into the 2.1 branch.
Commits
-------
b604eb7 [DoctrineBridge] Improved performance of the EntityType when used with the "query_builder" option
db2ee54 [DoctrineBridge] Improved exception message
99321cb [DoctrineBridge] Fixed: Exception is thrown if the entity class is not known to Doctrine
Discussion
----------
[DoctrineBridge] fixed caching when EntityType is used with the "query_builder" option
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 PR was squashed before being merged into the master branch (closes#6207).
Commits
-------
57e9d28 [DI] Add a base class for extension
Discussion
----------
[DI] Add a base class for extension
depends on #6148
@fabpot should we change `addClassesToCompile` & the likes (thinking of traits).
---------------------------------------------------------------------------
by fabpot at 2012-12-06T13:05:05Z
Can you rebase?
---------------------------------------------------------------------------
by fabpot at 2012-12-06T13:06:43Z
hmmm, now that I see the result, I'm not sure it is worth it as the Extension class in the DI component depends on the Config one.
---------------------------------------------------------------------------
by vicb at 2012-12-06T13:23:29Z
No pb, I can remove it, should I remove the `ContainerBuilder` altogether ?
---------------------------------------------------------------------------
by fabpot at 2012-12-06T13:37:18Z
I would keep everything that is strictly in the DI namespace in the DI extension class and everything else in the HttpKernel class as it is now.
---------------------------------------------------------------------------
by vicb at 2012-12-06T13:38:59Z
But this change is **great** if you need the DI without the full stack.
What about my other comment ?
---------------------------------------------------------------------------
by fabpot at 2012-12-06T13:55:30Z
Which other comment? This one? "should I remove the ContainerBuilder altogether?" In which case, I don't understand what it means.
What about adding 2 classes in the DI component: the base one and another one with the dependency on the config component? Is it overkill?
---------------------------------------------------------------------------
by vicb at 2012-12-06T14:06:43Z
> "should I remove the ContainerBuilder altogether?"
I mean that the **widely used** (ie loaders) `ContainerBuilder` also depends on Config - that was kind of a joke !
I was refering to my first comment here
> should we change addClassesToCompile & the likes (thinking of traits).
Overkill I don't know but useless for sure: the `ExtensionInterface` depends on `ContainerBuilder` which depends on `Config`.
This PR was merged into the master branch.
Commits
-------
cf63069 Fixed copy/paste mistake
Discussion
----------
Fixed copy/paste mistake in #6205
@fabpot Sorry, I had a little copy/paste mistake here #6205
This PR was squashed before being merged into the master branch (closes#5853).
Commits
-------
63b0059 [Process] Add ability to reset arguments on ProcessBuilder
Discussion
----------
[Process] Add ability to reset arguments on ProcessBuilder
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
This PR adds the ability to "reset" the arguments set on a `ProcessBuilder`. This allows the builder to be re-used without having to set things like custom environment variables, current working directory etc again.
This PR was squashed before being merged into the master branch (closes#5860).
Commits
-------
d0057d0 Added failure_path_parameter to mirror target_path_parameter
Discussion
----------
Added failure_path_parameter to mirror target_path_parameter
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
License of the code: MIT
Enable login failure redirect path can be assigned in a form field just like target path.
---------------------------------------------------------------------------
by stof at 2012-10-29T09:40:17Z
Please also open a PR to the doc repo to document this new feature
---------------------------------------------------------------------------
by leevigraham at 2012-10-29T09:56:29Z
@stof @fabpot Done.
This PR was merged into the master branch.
Commits
-------
7f16c1f [HttpKernel] Add DI extension configs as ressources when possible
Discussion
----------
[HttpKernel] Add DI extension configs as ressources when possible
/cc @rdohms @richardmiller
---------------------------------------------------------------------------
by vicb at 2012-11-30T11:57:48Z
btw @fabpot what about having a base class for `Extension` in the DI ? Would make it easier to re-use it when using standalone components, Di and (the suggested) Config as the greatest part of the class is not HttpKernel specific.
---------------------------------------------------------------------------
by fabpot at 2012-12-06T08:47:28Z
@vicb your suggestion makes sense.
Can you also explain the goal of this PR?
---------------------------------------------------------------------------
by vicb at 2012-12-06T09:01:58Z
The goal of this PR is to avoid having to sfcc when you modify a DI extension configuration. I think @rdohms got trapped.
---------------------------------------------------------------------------
by vicb at 2012-12-06T09:08:08Z
see https://twitter.com/rdohms/status/274059267428978688
---------------------------------------------------------------------------
by stof at 2012-12-06T09:20:54Z
I thought about it several times but never took time to implement it. It is annoying to have to clear the cache when you modify a default value in the Configuration class. So +1
This PR was merged into the master branch.
Commits
-------
d902e9d [FrameworkBundle] Added hostnamePattern to the router:debug command
Discussion
----------
[FrameworkBundle] Added hostnamePattern to the router:debug command
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
When you use the router:debug command with a specific route, the hostname is not show. I fix this with this PR. Also I make a little improvement to the requirements section.
This PR was merged into the 2.1 branch.
Commits
-------
10e5f3b Removed useless branch alias for dev-master in composer.json
Discussion
----------
[2.1] Fixed composer.json
---------------------------------------------------------------------------
by fabpot at 2012-12-06T08:23:35Z
Why is is useless?
---------------------------------------------------------------------------
by hason at 2012-12-06T08:30:58Z
Because the ``dev-master`` branch is alias for ``2.2-dev`` as mentioned @stof in https://github.com/symfony/symfony/pull/6196#discussion_r2320254.
---------------------------------------------------------------------------
by fabpot at 2012-12-06T08:33:42Z
got it now. Can you fix your PR as there are some unrelated commits? Thanks.
---------------------------------------------------------------------------
by hason at 2012-12-06T09:02:50Z
I backported some "unrelated" commits for better usage with composer. Should I remove these?
---------------------------------------------------------------------------
by fabpot at 2012-12-06T09:05:45Z
We do not backport things. So, please, remove them.
---------------------------------------------------------------------------
by hason at 2012-12-06T10:02:08Z
done
This PR was squashed before being merged into the master branch (closes#6083).
Commits
-------
6236c18 [FrameworkBundle] Added caching to TemplateController
Discussion
----------
[FrameworkBundle] Added caching to TemplateController
Because the main purpose for the `TemplateController` seems to be to render static pages like "disclaimer" and such, it seems useful to allow caching.
imprint:
pattern: /imprint
defaults:
_controller: SymfonyFrameworkBundle:Template:template
template: "::pages/imprint.html.twig"
maxAge: 86400
---------------------------------------------------------------------------
by pierredup at 2012-11-21T20:24:53Z
IMHO I think the caching should be allowed to be set optionally
---------------------------------------------------------------------------
by KingCrunch at 2012-11-21T20:38:54Z
I wrote it this way, because I assume, that it will cover more use-cases, than the other way round, but you are right, that this will change the current behaviour. Would like to hear other opinions, because I don't think one uses this action for anything else than fully-static content (means: The current behaviour doesn't feel very useful to me).
---------------------------------------------------------------------------
by pierredup at 2012-11-21T20:48:19Z
I totally agree, but I would then suggest keep the caching on by default, but have the option to turn it off if necessary
---------------------------------------------------------------------------
by pierredup at 2012-11-21T20:52:01Z
Actually I think to have caching permanently enabled for static content would probably be the best scenario, but I like to think in terms of flexibility and specific user requirements. It would be great to get some opinions from others on this
---------------------------------------------------------------------------
by KingCrunch at 2012-11-23T21:12:45Z
I thought about it and I come to the conclusion, that it is probably a not so good idea to enable caching by default, because ... well, it's not possible to disable it again. I guess something like this
{{ render '@AcmeBundle:ArticleController:latest' with {count: 1} }}
may be not so uncommon as I suggested in the first commit.
---------------------------------------------------------------------------
by fabpot at 2012-12-03T22:18:51Z
Can you make a PR for the docs? (symfony/symfony-docs). Thanks.
This PR was squashed before being merged into the master branch (closes#6173).
Commits
-------
4878ec0 [HttpKernel] [WebProfilerBundle] Better handling of deprecated methods
Discussion
----------
[HttpKernel] [WebProfilerBundle] Better handling of deprecated methods
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes, if you were expecting E_USER_DEPRECATED or E_DEPRECATED to throw an exception
Symfony2 tests pass: yes
Fixes the following tickets: #6139 partly, I'd go through and add the actual trigger_error() calls in another (or possibly one per component) PR
Todo: call trigger_error()
License of the code: MIT
Documentation PR: -
I added the deprecation count with the Exception icon in the Profiler Toolbar, and changed the color of it to be yellow for deprecations and red for exceptions (was yellow for exceptions).
---------------------------------------------------------------------------
by fabpot at 2012-12-03T09:43:09Z
Adding trigger_error calls should be done in one PR to ease the merging. thanks.
This PR was merged into the 2.0 branch.
Commits
-------
5fe58bf [Locale] fixed tests
500cc3c [Config] Fixed tests on Windows
Discussion
----------
[2.0] Fixed tests
---------------------------------------------------------------------------
by fabpot at 2012-12-05T15:25:23Z
Is it a backport of some commits that were merged in 2.1/master?
---------------------------------------------------------------------------
by hason at 2012-12-05T22:17:15Z
I backported 65281fb56c and modified 90d6dc3791
This PR was merged into the master branch.
Commits
-------
a3a832c Fix CS in the whole Propel1 bridge
86ab4b3 Add typehint to isInteger(), fix tests
a26a690 remove useless ColumnMap
ffd8759 fix some formatting issue
6fb9536 fix indentation problem
e5e3341 oups. It seems that here, we need \PDO
36d6c40 fix indentation problem
6f8cd9d Removed the PropelColumnTypes.php copy
8125163 removed the TODO mention. Will keep the Propel code here so it can work with older version of Propel
0e4419b I found the error in my latest commit. This pass the test suite.
cf8a6c0 Fix my code and also fix the test suite.
737b596 Merge remote-tracking branch 'upstream/master'
972e503 Fix problem when 1 column identifier in propel is a string.
Discussion
----------
Fix ModelChoiceList problem with string key
Replaces #6150
---------------------------------------------------------------------------
by willdurand at 2012-12-05T20:51:44Z
Note that 5f54ed1 is a "CS fix" commit. I don't want to open a PR just for that. Let me know if I should to remove it.
Also, I'm 👍 on this PR. Review has been made already, so it seems mergeable.
This PR was merged into the master branch.
Commits
-------
51223c0 added upgrade instructions
50e6259 adjusted tests
98f3ca8 [Routing] removed tree structure from RouteCollection
Discussion
----------
[Routing] removed tree structure from RouteCollection
BC break: yes (see below)
Deprecations: RouteCollection::getParent(); RouteCollection::getRoot()
tests pass: yes
The reason for this is so quite simple. The RouteCollection has been designed as a tree structure, but it cannot at all be used as one. There is no getter for a sub-collection at all. So you cannot access a sub-collection after you added it to the tree with `addCollection(new RouteCollection())`. In contrast to the form component, e.g. `$form->get('child')->get('grandchild')`.
So you can see the RouteCollection cannot be used as a tree and it should not, as the same can be achieved with a flat array!
Using a flat array removes all the need for recursive traversal and makes the code much faster, much lighter, less memory (big problem in CMS with many routes) and less error-prone.
BC break: there is only a BC break if somebody used the PHP API for defining RouteCollection and also added a Route to a collection after it has been added to another collection.
So
```
$rootCollection = new RouteCollection();
$subCollection = new RouteCollection();
$rootCollection->addCollection($subCollection);
$subCollection->add('foo', new Route('/foo'));
```
must be updated to the following (otherwise the 'foo' Route is not imported to the rootCollection)
```
$rootCollection = new RouteCollection();
$subCollection = new RouteCollection();
$subCollection->add('foo', new Route('/foo'));
$rootCollection->addCollection($subCollection);
```
Also one must call addCollection from the bottom to the top. So the correct sequence is the following (and not the reverse)
```
$childCollection->->addCollection($grandchildCollection);
$rootCollection->addCollection($childCollection);
```
Remeber, this is only needed when using PHP for defining routes and calling methods in a special order. There is no change required when using XML or YAML for definitions. Also, I'm pretty sure that neither the CMF, nor Drupal routing, nor Silex is relying on the tree stuff. So they should also still work.
cc @fabpot @crell @dbu
One more thing: RouteCollection wasn't an appropriate name for a tree anyway as a collection of routes (that it now is) is definitely not a tree.
Yet another point: The XML declaration of routes uses the `<import>` element, which is excatly what the new implementation of addCollection without the need of a tree does. So this is now also more analogous.
---------------------------------------------------------------------------
by Koc at 2012-11-26T17:34:15Z
What benefit of this?
---------------------------------------------------------------------------
by Tobion at 2012-11-26T17:56:53Z
@Koc Why did you not simply wait for the description? ^^
---------------------------------------------------------------------------
by dbu at 2012-11-26T18:33:09Z
i love PR that remove more code than they add whithout removing functionality.
---------------------------------------------------------------------------
by Crell at 2012-11-26T18:49:52Z
There's an issue somewhere in Drupal where we're trying to use addCollection() as a shorthand for iterating over one collection and calling add() on the other for each item. We can't do that, however, because the subcollections are not flattened properly when reading back and our current dumper can't cope with that. So this change would not harm Drupal at all, and would mean I don't have fix a bug in our dumper. :-) I cannot speak for any other projects, of course.
---------------------------------------------------------------------------
by Tobion at 2012-11-27T19:06:34Z
Ok, this is ready.
This PR was submitted for the 2.1 branch but it was merged into the master branch instead (closes#6086).
Commits
-------
d4a70e8 Implemented possibility to skip key normalization in config processing
Discussion
----------
Implemented possibility to skip key normalization in config processing
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: -
## Description
This PR implements the possibility to deactivate explicitly config keys normalization as it's sometimes annoying and unexpected to have `-` transformed in `_`. The default behavior is kept and deactivation is possible at the DI extension level (not possible to do it at the node level since the config processor does key normalization globally).
---------------------------------------------------------------------------
by lsmith77 at 2012-11-22T09:52:54Z
this is tricky since you might break some app config formats.
Semi related: I assume few people test their Bundles with anything but Yaml, but ATM the chances are quite good that it would also work with XML. then again many people are already not including the fix keys call so maybe we should also enable people to explicitly say which formats they support.
---------------------------------------------------------------------------
by stof at 2012-11-22T10:02:54Z
@lsmith77 you won't break anything as this PR is BC. You would brak it only if an existing bundle starts using it (and you are using XML)
---------------------------------------------------------------------------
by lsmith77 at 2012-11-22T10:14:55Z
I wasn't trying to imply it breaks BC but it would likely mean that Bundles using this would break the assumption that all formats are supported.
---------------------------------------------------------------------------
by stof at 2012-11-22T10:39:24Z
@lsmith77 The only difference is that a bundle using that would have an XML config using underscores instead of dashes (which are the XML convention).
And btw, as long as you don't provide an XSD, people can already use the underscored tags in their XML config...
---------------------------------------------------------------------------
by lsmith77 at 2012-11-22T11:49:50Z
right again. my point is that this feature breaks current assumptions. note I am not saying this should not be done either. just adding something to consider.
---------------------------------------------------------------------------
by lolautruche at 2012-11-22T16:30:20Z
Well, the real issue behind that is we currently don't know which format is used for application configuration, leading sometimes to unexpected issues. The problem is that the current *fix* is a bit brutal and magical, leading to headaches while debugging.
While a real way of dealing with config format should be the best way to fix this, allowing to throw an exception if the user format is inappropriate, this patch at least gives the opportunity to bypass this magical key normalization. A real solution should come with 2.2 or 2.3
---------------------------------------------------------------------------
by stof at 2012-11-22T17:07:17Z
Actually, this renaming of keys from dashes to underscores should probably be refactored to be aware of the tree. Because the only case where it causes some issues is for prototyped array nodes (using an associative array), as this is the only case where a key is defined by the user.
---------------------------------------------------------------------------
by lolautruche at 2012-11-23T07:30:57Z
@stof Exactly, and this is precisely where we have a problem, with prototyped array nodes. Having this key normalization aware of the tree is a nice option as well for a proper fix, but I think with this patch at least you can have *some* control 😃
---------------------------------------------------------------------------
by lolautruche at 2012-11-26T11:16:06Z
ping @fabpot
---------------------------------------------------------------------------
by lolautruche at 2012-12-05T09:41:49Z
Hello, any news on this ? Thanks
---------------------------------------------------------------------------
by fabpot at 2012-12-05T13:45:30Z
That can only be merged in master.
---------------------------------------------------------------------------
by lolautruche at 2012-12-05T13:47:01Z
@fabpot OK, should I open a new PR on master then ?
---------------------------------------------------------------------------
by fabpot at 2012-12-05T13:48:07Z
No, I'm going to switch the branch when merging, it was just to warn you about this.
---------------------------------------------------------------------------
by lolautruche at 2012-12-05T13:49:40Z
OK thanks 😃