Commits
-------
0776b50 removed supports(De)Serializiation()
72b9083 SerializerAwareNormalizer now only implements SerializerAwareInterface
97389fa use Serializer specific RuntimeException
cb495fd added additional unit tests for deserialization
967531f fixed various typos from the refactoring
067242d updated serializer tests to use the new interfaces
d811e29 CS fix
351eaa8 require a (de)normalizer inside the (de)normalizable interfaces instead of a serializer
c3d6123 re-added supports(de)normalization()
078f7f3 more typo fixes
c3a711d abstract class children should also implement dernormalization
2a6741c typo fix
d021dc8 refactored encoder handling to use the supports*() methods to determine which encoder handles what format
f8e2787 refactored Normalizer interfaces
58bd0f5 refactored the EncoderInterface
b0daf35 split off an EncoderInterface and NormalizerInterface from SerializerInterface
Discussion
----------
[Serializer] split off an EncoderInterface and NormalizerInterface from SerializerInte
Bug fix: no
Feature addition: no
Backwards compatibility break: yes (but not inside a stable API)
Symfony2 tests pass: 
Fixes the following tickets: #2153
The purpose is to make it easier for other implementations that only implement parts of the interface due to different underlying approaches like the JMSSerializerBundle.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/01 03:36:17 -0700
Actually, you can keep the current interface and I will just provide an adapter, sth like the following:
```php
<?php
class SymfonyAdapter implements SymfonyInterface
{
public function __construct(BundleInterface $serializer) { /* ... */ }
// symfony serializer methods mapped to bundle methods
}
```
I like to provide an adapter instead of implementing the interface directly since the bundle can be used standalone right now, and I don't want to add a dependency on the component just for the sake of the interface.
However, I do not completely see the purpose of the component. When would someone be recommended to use it? Everything the component does, the bundles does at the same level with the same complexity or simplicity (however you want to view that).
---------------------------------------------------------------------------
by lsmith77 at 2011/11/01 03:40:55 -0700
standalone in what way? you mean even out of the context of Symfony? In that context imho you should ship that code outside of a Bundle.
Regardless, how will that adaptor work? How would you implement methods like ``getEncoder()``? Afaik you can't and this is what this PR is about, splitting the interface to enable people to more finely specify what they provide.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/01 04:03:56 -0700
I would just throw exceptions when something is not supported.
The more important question though is what is the goal of the component in the long-term, i.e. what problems is it supposed to solve, or in which cases should it be used?
Because right now it seems to me - correct me if I'm wrong - that the only purpose is that people don't have to install an extra library. However, that might even be frustrating for users because they need to migrate their code to the bundle as soon as they need to customize the serialization process which you need in 99% of the cases. For deserialization, the situation in the component is even worse. So, if my assessment is correct here (i.e. component to get started fast, if you need more migrate to the bundle), I think it would be better and less painful to have them start with the bundle right away.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/01 04:15:10 -0700
Well then imho it would be better to split the interface.
I think the serializer component is sufficient for many situations and imho its easier to grok. Furthermore the normalizer/encoder concept it can be used in situations where JMSSerializerBundle cannot be used.
And splitting up the interfaces has exactly the goal of reducing the "frustrations" caused by out growing the the component.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/01 04:29:39 -0700
I don't agree, but it's a subjective thing anyway.
So, whatever interface you come up with (preferably as few methods as possible), I will provide an adapter for it.
---------------------------------------------------------------------------
by fabpot at 2011/11/07 08:45:25 -0800
What's the status of this PR?
---------------------------------------------------------------------------
by lsmith77 at 2011/11/07 10:28:14 -0800
from my POV its good to go. but would like a nod from someone else in terms of the naming of the new interfaces
On 07.11.2011, at 17:45, Fabien Potencier <reply@reply.github.com> wrote:
> What's the status of this PR?
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2530#issuecomment-2655889
---------------------------------------------------------------------------
by stof at 2011/11/08 11:37:40 -0800
@lsmith77 what about doing the same for the ``NormalizerInterface`` instead of adding a new interface with a confusing name ? The Serializer class could implement ``Normalizer\NormalizerInterface`` by adding the 2 needed methods instead of duplicating part of the interface.
The next step is to refactor the Serializer class so that it choose the encoder and the decoder based on the ``support*`` methods. But this could probably be done in a separate PR.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/08 11:51:27 -0800
yeah .. i wanted to do that once we are in agreement on the encoder stuff. question then is if we should again split off Denormalization. i guess yes.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/08 12:06:34 -0800
ok done ..
---------------------------------------------------------------------------
by lsmith77 at 2011/11/08 12:59:51 -0800
i guess the next big task is to add more tests .. had to fix way too few unit tests with all this shuffling around .. will also help validating the concept. i should also test this out in a production application.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/14 13:27:48 -0800
@ericclemmons can you also have a look at this PR and potentially help me adding tests?
---------------------------------------------------------------------------
by fabpot at 2011/12/07 07:32:06 -0800
@lsmith77: Is it ready to be merged? Should I wait for more unit tests?
---------------------------------------------------------------------------
by lsmith77 at 2011/12/07 07:34:56 -0800
If you merge it I am afraid I might get lazy and not write tests. This is why I changed the topic to WIP. I promise to finish this on the weekend.
Note however I was planning to write the tests for 2.0 and send them via a separate PR.
Once that PR is merged into 2.0 and master. I would then refactor them to work for this PR.
This way both 2.0 and master will have tests.
---------------------------------------------------------------------------
by fabpot at 2011/12/07 07:42:15 -0800
@lsmith77: sounds good. Thanks.
---------------------------------------------------------------------------
by lsmith77 at 2011/12/11 12:02:12 -0800
@fabpot ok i am done from my end.
@schmittjoh would be great if you could look over the final interfaces one time and give your blessing that you will indeed be able to provide implementations for these interfaces inside JMSSerializerBundle (even if just via an adapter)
---------------------------------------------------------------------------
by stof at 2011/12/12 12:43:49 -0800
@schmittjoh can you take a look as requested by @lsmith77 ?
---------------------------------------------------------------------------
by schmittjoh at 2011/12/13 03:33:23 -0800
Are the supports methods necessary? This is what I'm using in the bundle:
https://github.com/schmittjoh/JMSSerializerBundle/blob/master/Serializer/SerializerInterface.php
---------------------------------------------------------------------------
by lsmith77 at 2011/12/13 04:08:49 -0800
@schmittjoh without them determining if something is supported will always require an exception, which is pretty expensive. especially if one iterates over a data structure this can cause a lot of overhead.
---------------------------------------------------------------------------
by schmittjoh at 2011/12/13 04:24:18 -0800
my question was more if you have a real-world use case where this is useful?
On Tue, Dec 13, 2011 at 1:08 PM, Lukas Kahwe Smith <
reply@reply.github.com
> wrote:
> @schmittjoh without them determining if something is supported will always
> require an exception, which is pretty expensive. especially if one iterates
> over a data structure this can cause a lot of overhead.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2530#issuecomment-3122157
>
---------------------------------------------------------------------------
by lsmith77 at 2011/12/13 04:28:08 -0800
yes .. this serializer .. since it traverses the tree and decides what is the current normalizer one by one (aka not via visitors as in your implementation). without the supports*() methods it would need to have the normalizer throw exceptions, but this is not exceptional, its the normal code flow to have to iterate to find the correct normalizer.
---------------------------------------------------------------------------
by schmittjoh at 2011/12/13 04:30:36 -0800
can we split it off into a second interface?
On Tue, Dec 13, 2011 at 1:28 PM, Lukas Kahwe Smith <
reply@reply.github.com
> wrote:
> yes .. this serializer .. since it traverses the tree and decides what is
> the current normalizer one by one (aka not via visitors as in your
> implementation). without the supports*() methods it would need to have the
> normalizer throw exceptions, but this is not exceptional, its the normal
> code flow to have to iterate to find the correct normalizer.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2530#issuecomment-3122315
>
---------------------------------------------------------------------------
by lsmith77 at 2011/12/13 04:33:27 -0800
hmm .. i guess we could .. these methods in a way are implementation specific and are mainly public because its different objects interacting with each other, though for users of the lib they can also be convenient at times.
---------------------------------------------------------------------------
by lsmith77 at 2011/12/14 09:13:53 -0800
ok i reviewed things again and just removed those two methods, since the possibility for these methods to be feasible is too tied to the implementation and for this particular implementation supportsEncoding() and supportsDecoding() are still available.
so all ready to be merged ..
---------------------------------------------------------------------------
by lsmith77 at 2011/12/14 09:15:44 -0800
hmm i realized one thing just now:
cb495fd7a3
that commit should also be included in 2.0 .. i am not sure what the most elegant way is to make that happen ..
---------------------------------------------------------------------------
by fabpot at 2011/12/14 10:10:16 -0800
@lsmith77: commit cb495fd7a3 cannot be cherry picked in 2.0 as is as the tests do not pass: "Fatal error: Call to undefined method Symfony\Component\Serializer\Serializer::supportsDenormalization() in tests/Symfony/Tests/Component/Serializer/SerializerTest.php on line 150"
---------------------------------------------------------------------------
by lsmith77 at 2011/12/14 10:11:55 -0800
ah of course .. i just removed that method :) .. then never mind .. all is well.
Commits
-------
4d64d90 Allow empty result; change default *choices* value to **null** instead of **array()**. - added *testEmptyChoicesAreManaged* test - `null` as default value for choices. - is_array() used to test if choices are user-defined. - `null` as default value in __construct too. - `null` as default value for choices in EntityType.
Discussion
----------
[Doctrine][Bridge] EntityType: Allow empty result; default `choices` value changed to null
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
References the following tickets: #2504
- added *testEmptyChoicesAreManaged* test
- `null` as default value for choices.
- is_array() used to test if choices are user-defined.
- `null` as default value in __construct too.
- `null` as default value for choices in EntityType.
I squashed commits from PR #2504 as requested.
- added *testEmptyChoicesAreManaged* test
- `null` as default value for choices.
- is_array() used to test if choices are user-defined.
- `null` as default value in __construct too.
- `null` as default value for choices in EntityType.
Commits
-------
600066e [Templating] fixed 'scheme://' not detected as absolute path
e6f2687 [HttpKernel] fixed 'scheme://' not detected as absolute path
b50ac5b [Config] fixed 'scheme://' not detected as absolute path
Discussion
----------
[Config][HttpKernel][Templating] 'scheme://' paths not detected as absolute
Bug fix: yes
Feature addition: no
Backwards compatibility break: no (99%)
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
The method ```isAbsolutePath``` does not detect URL schemes as absolute. This makes imposible the use of wrappers to access remote files or the use of files (mostly configuration or templates) stored on phar archives (uses the scheme ```phar://``` in the path).
Three classes implement this methods: ```Symfony\Component\Config\FileLocator```, ```Symfony\Component\HttpKernel\Util\Filesystem``` and ```Symfony\Component\Templating\Loader\FilesytemLoader```. All are updated. Also includes a new check on all related tests (```Symfony\Component\HttpKernel\Util\Filesystem``` lacks of test).
Commits
-------
cd24fb8 change explode's limit parameter based on known variable content
b3cc270 minor optimalisations for explode
Discussion
----------
[FrameworkBundle][CssSelector][HttpFoundation][HttpKernel] [Security][Validator] Minor optimizations for "explode" function
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
I added limit parameter in some places, where it may be usefull. I did not check the context of what values may have been exploded. So to not break anything, I added +1 to limit parameter.
If you find out that in some places limit (or limit+1) is not important or meaningless, write a comment please and I will fix it.
---------------------------------------------------------------------------
by fabpot at 2011/12/07 06:56:49 -0800
Adding +1 just to be sure to not break anything is clearly something we won't do. What is the benefit of doing that anyway?
---------------------------------------------------------------------------
by pulzarraider at 2011/12/07 13:50:24 -0800
The main idea of making this PR was to notify about some places that may run faster with just adding one parameter to explode function.
If in code is someting like: ```list($a, $b) = explode(':', $s);```
Function ```explode``` will create n-items (depends on ```$s```), but we need in code only the first two items. There is no reason to let ```explode``` create more items in memory that are NEVER used in our code. The limit parameter is there for these situations, so let's use it.
I know that it is microoptimization and may look unimportant, but we are writing a framework - so people expect that code will be as fast as possible without this kind of mistakes.
As I've noticed above, I know that +1 is not ideal solution, but the fastest without debugging the code. I expect that someone (with good knowledge of that code) will look at it and write in comments if variable may contain 1 comma (dot or someting on what is doing the explode) or maybe 2 in some situations or more.
Anyway, +1 will not break anything, because same items are created as it is now, but no unnecessary item is created.
---------------------------------------------------------------------------
by fabpot at 2011/12/07 23:14:59 -0800
I'm +1 for adding the number to avoid problems but I'm -1 on the optimization side of things as it won't optimize anything.
---------------------------------------------------------------------------
by helmer at 2011/12/08 12:46:49 -0800
*.. The main idea of making this PR was to notify about some places that **may** run faster ..*
I am also unsure the optimization is really an optimization, care to benchmark (with meaningful inputs)? As for the limit+1 thing, why would you want to +1 it? The number of ``list`` arguments should always reflect the ``limit`` parameter, no?
---------------------------------------------------------------------------
by pulzarraider at 2011/12/08 23:11:34 -0800
@helmer please try this simple benchmark:
```
<?php
header('Content-Type: text/plain; charset=UTF-8');
define('COUNT', 10000);
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc:dddddddddddddddddddddd:eeeeeeeeeeeeeeeeeeeeeeeee:fffffffffffffffffffffffffff';
$start = microtime(true);
for ($i = 0; $i < COUNT; $i++) {
list($a, $b) = explode(':', $source_string);
}
$end = microtime(true)-$start;
echo 'without limit: '.$end."\n";
$start = microtime(true);
for ($i = 0; $i < COUNT; $i++) {
list($a, $b) = explode(':', $source_string, 2);
}
$end = microtime(true)-$start;
echo 'with limit: '.$end."\n";
```
My results are:
```
without limit: 0.057228803634644
with limit: 0.028676986694336
```
That is 50% difference (with APC enabled). Of course the result depends on the length of source string and if it's too short, the difference may be none or very very small. That's why I said, that it **may** run faster and is just a micro optimization.
---------------------------------------------------------------------------
by pulzarraider at 2011/12/08 23:18:12 -0800
@helmer And why +1? It depends on a code:
```
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc';
list($a, $b) = explode(':', $source_string, 2);
var_dump($a, $b);
```
and
```
$source_string = 'aaaaaaaaaaaaaaaaaaaa:bbbbbbbbbbbbbbbbbbbbb:cccccccccccccccccccccccc';
list($a, $b) = explode(':', $source_string, 3);
var_dump($a, $b);
```
gives different results. That's why the content of the variable must be known.
---------------------------------------------------------------------------
by helmer at 2011/12/09 00:08:28 -0800
@pulzarraider Thanks for the benchmark, seems like a gain enough. Although, we are more likely having a scenario of:
``explode(':', 'a🅱️c')`` vs ``explode(':', 'a🅱️c', 3)`` with a ``COUNT`` of 10, where the difference is not even in microseconds anymore :)
The limit addition alters the behaviour though, ie suddenly you can define a controller [logical name](http://symfony.com/doc/current/book/routing.html#controller-string-syntax) as ´´AcmeBlogBundle:Blog:show:something``, and things go downhill from there on.
All that aside, I'm +1 for setting the limit to the exact number of ``list`` parameters, but certainly not number+1, this is just too wtfy (as you said, this was a safety thing, but I reckon for this PR to be merged it needs to be +0).
---------------------------------------------------------------------------
by drak at 2011/12/09 08:28:58 -0800
Overall `list()` is ugly as it's not very explicit. Even though it would mean extra lines, it's better to `explode()` then explicitly assign variables:
```
$parts = explode(':', $foo);
$name = $parts[0];
$tel = $parts[1];
```
`list()` is one of those bad relics from the PHP past...
---------------------------------------------------------------------------
by fabpot at 2011/12/11 10:07:47 -0800
@drak: why is `list` not explicit? It is in fact as explicit as the more verbose syntax you propose.
---------------------------------------------------------------------------
by pulzarraider at 2011/12/11 13:08:50 -0800
@drak: I agree with @fabpot. In speech of benchmarks ```list``` is faster then using a helper variable.
@fabpot, @helmer I've changed explode's limit to be correct (without +1) and removed some changes from this PR, where I can't find out what the content of variable may be. Unit tests pass, so I think it's ready for merge.
Commits
-------
f44bcaf fix error due to merging 2.0 into master
Discussion
----------
(WebProfilerBundle) fix error resulting from merging 2.0 into master
Commits
-------
5f22268 [Profiler] Sync with master
1aef4e8 Adds collecting info about request method and allowing searching by it
Discussion
----------
[WebProfiler] Add ability to filter data by request method
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #1515
For discussion & description checkout: #1515 & #2279
---------------------------------------------------------------------------
by fabpot at 2011/12/11 10:02:41 -0800
After merging this PR, the toolbar is not displayed anymore for me.
---------------------------------------------------------------------------
by stof at 2011/12/12 14:18:20 -0800
@fabpot the toolbar works for me using this branch
Commits
-------
6548354 fixed data-url
Discussion
----------
[WebProfilerBundle] fixed and adjusted HTML5 markup
I corrected some markup errors that I found when validating the pages of the WebProfilerBundle.
Along the way I also improved the semantic structure of HTML5 like table header and body, lang attribute.
Removed type="text/css" that is the default with rel="stylesheet". Also no need for media="screen"!? Otherwise style does not apply when debugging with handheld device or when printing.
---------------------------------------------------------------------------
by fabpot at 2011/12/12 23:37:15 -0800
@Tobion: Can you squash your commit before I merge your PR? Thanks.
---------------------------------------------------------------------------
by Tobion at 2011/12/13 03:14:51 -0800
@fabpot I would appreciate if you could do this.
I see two problems with pull requests on @github that occur again and again. It's pretty annoying compared to the otherwise very user-friendly Github.
1. Squashing commits of a pull request: If you've already pushed commits to GitHub, and then squash them locally, you will not be able to push that same branch to GitHub again. So you need to create a new branch and a new pull request.
So there should be a button on Github that simply squashes all commits and allows you to enter a new commit message.
2. Opening a pull request based on the master branch instead of the 2.0 branch where bug fixes should be made. So people must rebase their stuff and open a new pull request again. All this back and forth is taking time unnecessarily (both for admins and contributors) and cluttering Githubs news feed.
There should be the possibility to allow switching the pull request base branch. Or at least give the users a configurable hint about the best practice of contributing to a specific repo when they open a pull request.
---------------------------------------------------------------------------
by henrikbjorn at 2011/12/13 03:16:10 -0800
@Tobion
1. Solved by doing a git push -f remote_name branch_name
2. Yes here you need to open a new PR
---------------------------------------------------------------------------
by fabpot at 2011/12/13 03:21:47 -0800
@Tobion: I'm more than aware of these issues but unfortunately, there is nothing I can do if we want to continue using the Github PRs (and automatic closing).
---------------------------------------------------------------------------
by Tobion at 2011/12/13 03:51:47 -0800
That's why I hope that @github will provide a convenient solution to these issues.
---------------------------------------------------------------------------
by stof at 2011/12/13 04:08:07 -0800
@Tobion send a feature request to github. Commenting here will not make them implement it
---------------------------------------------------------------------------
by Tobion at 2011/12/13 04:18:31 -0800
@fabpot I squashed commits.
@stof I will do it. But there is no public issue tracker for the Github software, is there? So need to use the contact form I suppose.
fixed markup: <pre> not valid inside <p>
adjusted base html structure for HTML5
improved table markup in bag.html.twig
improved table markup in results.html.twig
update exception.html.twig
Commits
-------
d97d7e9 Added a check to see if the type is a string if it's not a FormTypeInterface
Discussion
----------
Add exception on wrong type
When you forget to extend AbstractType in your form type, and then try to create a named builder from it, the error message is quite confusing:
Expected argument of type "string", "Samson\InvoiceBundle\Form\Type\PaymentTermsType" given (from the getType() method)
This PR checks for the right type at the relevant place
---------------------------------------------------------------------------
by stloyd at 2011/12/13 03:00:29 -0800
IMO you should add an test for this.
---------------------------------------------------------------------------
by Burgov at 2011/12/13 03:11:50 -0800
@stloyd done
---------------------------------------------------------------------------
by fabpot at 2011/12/13 03:27:08 -0800
@Burgov: Looks good to me. Can you squash your commits before I merge this PR? Thanks.
---------------------------------------------------------------------------
by Burgov at 2011/12/13 03:29:00 -0800
@fabpot done!
Commits
-------
73ac773 [Config] added ability to set info message and example to node definition
Discussion
----------
[2.1] [Config] added ability to set info message to node definition
The configuration TreeBuilder lends itself to be hooked into for reference documentation generation. This ``setInfo()`` method allows the addition of a message to a node for use in doc generation.
Example (``Symfony/Bundle/WebProfilerBundle/DependencyInjection/Configuration.php``):
```php
<?php
// ...
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root('web_profiler');
$rootNode
->children()
->booleanNode('verbose')->defaultTrue()
->setInfo('Setting to false hides some secondary information to make the toolbar shorter.')->end()
->booleanNode('toolbar')->defaultFalse()
->setInfo('Enable/Disable the display of the web debug toolbar containing a summary of the profiling data.')->end()
->booleanNode('intercept_redirects')->defaultFalse()
->setInfo('Intercepts the redirects and gives you the opportunity to look at the collected data before following the redirect.')->end()
->end()
;
return $treeBuilder;
// ...
```
I think a core console command would be great (ie: ``config:reference:dump web_profiler``) or even a frame in the profiler. The way bundle configuration processing is not enforced makes this difficult currently. Perhaps adding a ``getConfiguration()`` method to ``ExtensionInterface``. Thoughts?
Certainly this change would allow for a third party bundles or sites (ie symfony.com) to generate bundle reference docs.
---------------------------------------------------------------------------
by Seldaek at 2011/05/25 10:36:10 -0700
👍 finally some effort in this direction, although as @schmittjoh said without generation this won't bring us very far. But if it allows everyone to document their stuff already, I think it's still a plus, otherwise we'll have generation without content. And content without generation is still useful for people looking at the sources.
---------------------------------------------------------------------------
by Problematic at 2011/05/25 10:52:42 -0700
+1, even if the only thing it does is save me some WTFs later looking through my own code.
---------------------------------------------------------------------------
by weaverryan at 2011/05/25 10:59:25 -0700
@kbond and I talked about this a bit over the weekend and decided that he should at least get this first step going. It *is* of limited use in its current state (thought @Problematic and @Seldaek bring up a good points), but we need some ideas on where to go next.
As @kbond says, nothing really ties the DI extension to a config class now. Should we add a `getConfiguration()` or `getTreeBuilder()`? How can we make it so that we *know* which `TreeBuilder` to use for each DI extension config alias?
---------------------------------------------------------------------------
by lsmith77 at 2011/05/25 11:21:05 -0700
I think this is great and I dont think we need to require a generator if the API makes sense.
One thing the API should also cover is setting a custom error message in case of a validation error. Just something to keep in mind.
---------------------------------------------------------------------------
by stof at 2011/05/25 11:26:05 -0700
@weaverryan I think that ``getConfiguration`` would be better than ``getTreeBuilder`` as this is still the method implemented by the ``ConfigurationInterface``. Of course this method should be optionnal (defaulting to return ``null``)
---------------------------------------------------------------------------
by kbond at 2011/05/25 13:09:26 -0700
The ``ConfigurationInterface`` API would need to be locked down more. For example, FrameworkBundle's ``Configuration`` class has a constructor. I am afraid if we lock it down too much we could lose its flexibility.
---------------------------------------------------------------------------
by stof at 2011/05/25 13:24:51 -0700
@kbond the constructor **cannot** be enforced by an interface. This is why I was talking about adding a ``getConfiguration`` method in the extension. This way, the extension can do whatever it wants to instantiate the Configuration, passing whatever param you want.
---------------------------------------------------------------------------
by kbond at 2011/05/25 14:12:03 -0700
Sorry meant the ``ExtensionInterface``. I would like the ``getConfiguration`` method enforced. How would we tackle the FrameworkBundle? To retrieve it's configuration you need the ``ContainerBuilder``.
---------------------------------------------------------------------------
by kbond at 2011/05/25 14:38:02 -0700
The only way I can see the bundle method working is if the method was manually overridden for each bundle (returns null by default). The fact that ``Configuration`` classes may or may not have constructors prevent it from being automated.
---------------------------------------------------------------------------
by kbond at 2011/05/26 06:24:37 -0700
I think we should avoid *searching* if at all possible.
The only real thing you might need to build your configuration is the ``ContainerBuilder``. What about passing it to the ``Extension`` class constructor? Then it could be accessed in a ``getConfiguration()`` method. Looking at the internals, I can't see how I could do that but thought I would throw it out there.
---------------------------------------------------------------------------
by stof at 2011/05/26 06:31:46 -0700
The constructor is not a good idea IMO as the ContainerBuilder passed to an extension is not the main one when using the ``load`` method. So passing it in the constructor is bad IMO. Passing it to the ``getConfiguration`` method would be better IMO.
---------------------------------------------------------------------------
by kbond at 2011/05/26 08:04:08 -0700
Based on the comments I am seeing 2 ways of implementing a ``getConfiguration()`` method. I have started branches for both ways:
1. ``getConfiguration()`` in Bundle class: 3fb1c889af
2. ``getConfiguration()`` in Extension class: cf05ec20fc
I have updated the ``FrameworkBundle`` in both.
On another note, I think one ``Configuration`` class per Bundle should be enforced. Any reason why we can't do this? The only core bundle that has more than one is ``SecurityBundle`` and I don't get why it needs two (not saying it shouldn't, I just don't know why).
---------------------------------------------------------------------------
by stof at 2011/05/26 08:15:52 -0700
One configuration class per bundle does not make any sense as each configuration class is responsible to merge an array of configurations. So a bundle providing several extension (not the common use case at all but supported) would need several configuration classes as each extension would get a different set of configuration.
and if you look at what a Configuration class do, it merges several configurations in an intelligent way and normalizes it. Look at why a second one is used in the SecurityBundle and you will see that this is a perfectly valid use of it. The Config component is not limited to DI extensions configuration.
And for the 2 implementations, I prefer the one putting it in the extension. Here are my reasons:
- a bundle supports having several extensions so returning only one Configuration class for the bundle is broken in this case (for which extension is it ? and what about the others ?)
- the extension is the class using the Configuration, not the bundle. So it is logical to put it in it
- this removes code duplication to instantiate the Configuration as the extension can use the method.
---------------------------------------------------------------------------
by kbond at 2011/05/27 08:41:58 -0700
Created a *rough* initial ``config:dump`` console command. Works for all core bundles. Still needs some work for the more complex Node types.
Had some issues getting it to work with ``AsseticBundle`` and ``SecurityBundle``. There is some code duplication I couldn't figure out how to avoid.
@weaverryan I tried to mimic the YAML format you have in the reference docs.
---------------------------------------------------------------------------
by kbond at 2011/06/02 08:09:57 -0700
I reduced the scope of this PR to just the API to document configuration nodes and access the configuration object from an extension. Like stated initially, if it makes sense we should add it and worry about the generation later.
I am still going to work on a console ``config:dump`` command but in another branch.
---------------------------------------------------------------------------
by stof at 2011/09/04 05:36:09 -0700
@kbond could you rebase your PR ? It conflicts with the current master branch.
Apart from that, the way to add info on the node seems good to me. @schmittjoh what do you think ?
---------------------------------------------------------------------------
by kbond at 2011/09/04 06:59:29 -0700
Will do, give me a few days (on vacation)
---------------------------------------------------------------------------
by kbond at 2011/09/06 08:48:55 -0700
@stof should i squash the commits into 1?
---------------------------------------------------------------------------
by stof at 2011/09/06 09:00:46 -0700
If you want, but it is not necessary here IMO. the history is not a mess as in some other PR with lots of changes.
---------------------------------------------------------------------------
by kbond at 2011/09/06 10:14:32 -0700
Ok, rebased.
---------------------------------------------------------------------------
by lsmith77 at 2011/09/15 07:49:17 -0700
@kbond will you be around for the IRC meeting in 10mins?
---------------------------------------------------------------------------
by kbond at 2011/09/22 08:13:37 -0700
There is a rudimentary config dump command based on this PR available for testing here: https://github.com/kbond/symfony/tree/config_dump_command
---------------------------------------------------------------------------
by stof at 2011/10/16 11:03:57 -0700
@fabpot @schmittjoh ping
---------------------------------------------------------------------------
by stof at 2011/11/11 07:01:12 -0800
@fabpot ping
---------------------------------------------------------------------------
by kbond at 2011/11/22 07:14:58 -0800
I added a new interface as discussed in the irc meeting. Is this ok?
---------------------------------------------------------------------------
by stof at 2011/12/12 12:38:38 -0800
@fabpot this PR conflicts with master once again, needing to rebase it. It would be good to review it so that we don't need to keep it pending for each further change in master
@kbond can you do the rebase ?
---------------------------------------------------------------------------
by kbond at 2011/12/12 19:35:05 -0800
rebased.
---------------------------------------------------------------------------
by fabpot at 2011/12/13 02:56:02 -0800
@kbond: Can you squash your commits before I merge this PR? Thanks.
---------------------------------------------------------------------------
by kbond at 2011/12/13 03:09:09 -0800
@fabpot done.
Commits
-------
7827f72Fixes#2817: ensure that the base loader is correctly initialised
Discussion
----------
[TwigBundle] Ensure base Filesystem loader paths are initialised
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Fixes the following tickets: #2817
Originated from #2817.
Commits
-------
7fadd08 static::$privateField is an OOP non-sense (extending the class is not possible)
Discussion
----------
static::$privateField is an OOP non-sense (extending the class is not possible)
static::$privateField is an OOP non-sense (extending the class is not possible)
Commits
-------
9c1fbb8 [DoctrineBridge] fixed the refreshing of the user for invalid users
Discussion
----------
[DoctrineBridge] fixed the refreshing of the user for invalid users
A user provider is not allowed to return ``null`` when the user is not found.
This bug is the reason why #2845 has been submitted
---------------------------------------------------------------------------
by stof at 2011/12/12 04:47:04 -0800
it closes#2822 btw
Commits
-------
171f2d5 fixed failing test in windows because
Discussion
----------
fixed failing test in windows
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
fixed failing test in windows because
1) PHP_BINDIR is not secure to rely on
2) the assertion doesn't actually check for the suffix as the test implies
Commits
-------
45bba7b Added a hint about a possible cause for why no mime type guesser is be available
Discussion
----------
Added a hint about a possible cause for why no mime type guesser is be available
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
