Commits
-------
11b6156 updated unittest
a931e21 get correct client IP from X-forwarded-for header
Discussion
----------
[HttpFoundation] Get correct client IP when using trusted proxy (Varnish)
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Note: This is reopened PR #2686 for 2.0 branch.
If using trusted proxy (Varnish, ...) the client IP must be identified from X-Forwarded-For header. The header has de-facto standard format:
X-Forwarded-For : client1, proxy1, proxy2,
where the value is a comma+space separated list of IP addresses, the left-most being the farthest downstream client, and each successive proxy that passed the request adding the IP address where it received the request from. See: http://en.wikipedia.org/wiki/X-Forwarded-For
Function getClientIp should return only one client IP, not a list of all nonimportant IPs as it's now. Similar example can be seen in Cake framework: http://api.cakephp.org/view_source/request-handler-component/#line-477
There are many ways how to chose the first IP from X-Forwarded-For header. Any other faster and more reliable way is welcome.
Commits
-------
0e51e32 point the status icon to 2.0
9d2bd1c tweaked travis configuration
f68c028 [CI] travis-ci.org integration
Discussion
----------
Add travis ci to 2.0
The original PR was send to master and not 2.0, so I cherry picked the two relevant commits.
Once this PR is merged, I will send another PR to master to set the status icon branch accordingly.
Commits
-------
b6bf018 tweaked error handling for the forward compatibility
dd606b5 added note about the purpose of this class
c1426ba added locale handling forward compatibility
10eed30 added MessageDataCollector forward compatibility
Discussion
----------
Forward compat
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2522
Commits
-------
78e9b2f [Form] Fixed textarea_widget (W3C standards)
Discussion
----------
[Form] Fixed textarea_widget (W3C standards)
Textarea widget included the "pattern" attribute but is not valid by W3C standards.
(See PR 2666 - New PR because rebase inside the 2.0 branch)
---------------------------------------------------------------------------
by fabpot at 2011/11/18 09:01:41 -0800
@hlecorche: Thanks for your work on this issue. Can you update the unit tests to be sure that this case is covered? If you're not comfortable with this, just tell me and I will do it myself
---------------------------------------------------------------------------
by hlecorche at 2011/11/19 02:51:06 -0800
@fabpot: I did'nt commited because I am not sure. I changed the "tests/Symfony/Tests/Component/Form/AbstractLayoutTest.php" file :
public function testTextarea()
{
$form = $this->factory->createNamed('textarea', 'na&me', 'foo&bar', array(
'property_path' => 'name',
'pattern' => 'foo',
));
$this->assertWidgetMatchesXpath($form->createView(), array(),
'/textarea
[@name="na&me"]
[not(@pattern)]
[.="foo&bar"]
'
);
}
Is it correct?
Commits
-------
36cebf0 Fix infinite loop on circullar reference in form factory
Discussion
----------
[BugFix][Form]Throw exception on form name circulal ref
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Closes: #2673
When FormType method `getName()` returns the same value as `getParent()` we're asking about trouble, and land into infinite loop.
Commits
-------
5b30812 See this issue : https://github.com/symfony/symfony/issues/2433
Discussion
----------
See this issue : https://github.com/symfony/symfony/issues/2433
I changed the access speficiers to `protected`, which makes easier to extend this class if one needs to like I did.
---------------------------------------------------------------------------
by greg0ire at 2011/11/10 06:55:12 -0800
Precision on the problem I had : I wanted to use a `CollectionType` and display a collection element attribute as the label for this element. I had no choice but to extend `ResizeFormListener` and `CollectionType`.
Commits
-------
57e1aeb Fixed undefined index notice in readProperty() method (PropertyPath)
Discussion
----------
Fixed undefined index notice in readProperty() method (PropertyPath)
Hi,
For some reasons, I get `notice` errors on `readProperty()` with Propel:
Notice: Undefined index: 0 in /Users/william/projects/Propel/testProjects/symfony2/vendor/symfony/src/Symfony/Component/Form/Util/PropertyPath.php line 284
The `PropelObjectCollection` implements `ArrayAccess`, the `readProperty()` method does not check if the given `index` exists so the `notice` error is thrown. I suppose to check whether the index exists or not has to be added.
Regards,
William
---------------------------------------------------------------------------
by fabpot at 2011/09/27 23:42:07 -0700
The patch is probably not what we want to do. First, I suppose that you are not creating the propertyPath by hand. If that is the case, we need to understand why the property path does not exist. Then, even if we might want to check the existence of the index, if it does not exist, we should probably throw an exception instead of just ignoring the problem.
---------------------------------------------------------------------------
by willdurand at 2011/09/28 01:14:49 -0700
My bad. This is a Propel bug due to `ArrayObject`. It throws a notice error if the index is not found in `offsetGet()` which is wrong according to the `ArrayAccess` interface. If the index is not found, we have to return `null`.
@fabpot Are you agree with that (for the `null` value) ?
---------------------------------------------------------------------------
by fabpot at 2011/09/28 01:17:09 -0700
My point is that it should never happen under normal circumstances.
---------------------------------------------------------------------------
by willdurand at 2011/09/28 01:23:55 -0700
@fabpot Not sure to get it.
The fact is that it tries to get the value (`getValue()`) of a fresh object, just added to the `collection` when I'm submitting a form with a `CollectionType` and a new entry in it.
I mean it tries to get this new object (not yet persisted, not yet in the collection) in the collection (`getValue()` -> `readProperty()`) which implements `ArrayAccess` but this object cannot be in the collection at this time.
Am I wrong ?
And, without this notice error thrown by Propel, I probably never opened this issue...
---------------------------------------------------------------------------
by willdurand at 2011/09/29 06:40:34 -0700
@fabpot: you can try this example: http://www.propelorm.org/cookbook/symfony2/mastering-symfony2-forms-with-propel.html#manytomany_relations in order to make your own tests. Will it be enough?
As I said, it throws a weird notice for the reasons above.
---------------------------------------------------------------------------
by jaugustin at 2011/10/04 12:58:10 -0700
any news on this ?
@fabpot did you have time to look at the test case ?
---------------------------------------------------------------------------
by cedriclombardot at 2011/11/09 14:29:42 -0800
@fabpot: can we have news about this ?
Commits
-------
47ebf08 Fix some bugs
fad825e Add DoctrineValidationPass to DoctrineBundle#buildContainer
a064acd Implement feature to add validations based on the Manager-Type (ORM, MongoDB, CouchDB)
Discussion
----------
[WIP] Validation on a Doctrine Manager Basis
Hello,
we have had problems before with validation that is "persistence" related. Unique-validators or any other validation that is based on services that depend on persistence.
The problem is two-fold:
1. In annotations you cannot define validators for all persistence layers you support, because then users need them all installed.
2. In XML/YAML the same is true, since there is only one validation.xml or validation.yml file looked for.
Now one solution is to have three model classes that extend from a base class to get around this (like FOSUserBundle does) but that is cumbersome. This PR provides a new solution that is Doctrine specific (and takes the responsibility out of the Core).
Each Doctrine Bundle (ORM, CouchDB, MongoDB, PHPCR) can add this compiler pass with a manager type name:
$container->addCompilerPass(new DoctrineValidationPass('orm'));
This leads to the compiler pass searching for additional validation files "Resources/config/validation.orm.yml" and "Resources/configvalidation.orm.xml".
My first idea was to put this into the Resources/config/doctrine folder as well, but then it is detected as mapping file of course.
Regarding tests, this is not easily testable without a full fledged bundle setup, i tested this inside Acme Demo Bundle, however for a good unit-test we probably need a filesystem abstraction testing layer. Has anyone a good idea how to test this without having to setup another test-bundle? I can't use the one from DoctrineBundle since this code is in the Bridge.
---------------------------------------------------------------------------
by fabpot at 2011/11/13 23:12:06 -0800
@beberlei: Is it still WIP?
---------------------------------------------------------------------------
by beberlei at 2011/11/15 10:47:49 -0800
@fabpot it is complete, but it has no tests, that was the WIP part. :-)
---------------------------------------------------------------------------
by mvrhov at 2011/11/15 23:56:11 -0800
I wanted to refactor how validation is managed today, so it could do one validation file per class, same as with Doctrine but @stof pointed me to this PR. I still find this a great idea as the validation is easier to find.
```php
foreach ($container->getParameter('kernel.bundles') as $bundle) {
$reflection = new \ReflectionClass($bundle);
$bundleDir = dirname($reflection->getFilename());
//check for per class validation files
if (is_dir($dir = $bundleDir . '/Resources/config/validation')) {
$finder = new Finder();
$finder
->name('*'.$extension)
->in($dir);
foreach ($finder as $file) {
$files[] = realpath($file);
$container->addResource(new FileResource($file));
}
}
//global validation file?
if (is_file($file = $bundleDir . '/Resources/config/validation'.$extension)) {
$files[] = realpath($file);
$container->addResource(new FileResource($file));
}
}
```
Commits
-------
fc4e628 [Config] added append to the node builder
Discussion
----------
[Config] added append to the node builder
Bug fix: no
Feature addition: yes
BC break: no
test pass: true
the problem is that i can only append a node if i have an array node, but if the current node is the node builder i can not append a node (and i think the node builder is like a array node).
---------------------------------------------------------------------------
by tecbot at 2011/11/16 23:23:16 -0800
@fabpot: any chance to merge this?
Commits
-------
fabe818 [EventDispatcher] Add reference to the EventDispatcher on the Event
Discussion
----------
[EventDispatcher] Add reference to the EventDispatcher on the Event
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
I don't like registering event listeners unless they are really used, it seems wasteful. So I tend to register listeners for the response event in other listeners, only when they will be required. @stof has [brought to my attention](fb243ace83 (commitcomment-696467)) that this may cause issues in Silex or any other situation where event listeners are not lazy loaded, since it creates a circular reference in that case.
With this PR, avoiding the circular reference is possible, without bloating the response listener with unnecessary "do I need to do anything?" code.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/06 05:28:39 -0800
Did you do any benchmarks? It's just a feeling, but registering a listener at runtime might be more expensive than just having it always executed.
Also, I find these dynamic listeners a bit of a code smell. They are not easily testable, and the control flow is harder to track. Besides, you do not take into account subrequests which might happen in between.
---------------------------------------------------------------------------
by Seldaek at 2011/11/06 09:34:27 -0800
I don't see why it would be slower, if it's a commonly fired event yes you blast away the `sorted` listener cache every time you add one, but most of the time those optional listeners are for the response, which is typically not sorted yet when you add the listener, so I don't think there is any overhead.
As for the code smell, of course it's a matter of preference, but I have the opposite view on control flow, I find it weird that listeners are registered when they are not used in the end, while doing it my way I think it's more clear what happens.
For sub-requests, I'm not sure what you mean. In this instance, and in most response listeners I have seen, the sub-requests are always ignored anyway by the listener.
---------------------------------------------------------------------------
by drak at 2011/11/10 06:07:45 -0800
I don't see how loading up the dispatcher with a bunch of callables can be expensive - it's just loading an array basically.
Wouldn't it be better to have a separate `DispatcherAwareEvent extends Event`
class DispatcherAwareEvent extends Event
{
protected $dispatcher;
public function setDispatcher(EventDispatcher $dispatcher)
{
$this->dispatcher = $dispatcher;
}
public function getDispatcher()
{
return $this->dispatcher;
}
This can then be used as a base class for what you need `MyEvent extends DispatcherAwareEvent`
$event = new MyEvent($dispatcher, $foo);
$dispatcher->dispatch($event);
---------------------------------------------------------------------------
by Seldaek at 2011/11/10 06:18:57 -0800
@drak: Every event is part the event dispatching system and therefore should be aware of the dispatcher imo. It's not like the ContainerAwareInterface which is gluing things that do not especially have to know about the DIC together.
If we do that, then we have to start arguing every time we need the dispatcher in a given event, because the original author did not think it was necessary, and then that will only make it into the next minor version, etc. Not fun at all.
---------------------------------------------------------------------------
by drak at 2011/11/10 06:36:26 -0800
By the way, the event dispatchers looks to be pretty well optimized given the fact that it only sorts listeners if they are called, and then only once.
---------------------------------------------------------------------------
by drak at 2011/11/10 12:33:28 -0800
It just seems weird. I mean, following on, why isn't the event name a compulsory parameter also? - again, you can say both ways, if you need it, make it part of your custom Event class, or since it's a required param to be able to dispatch an event in the first place, make it part of the base Event class. All I'm saying it it seems suspicious when it could be achieved a different way.
For example, you could inject the dispatcher into the listener itself and then the event handler could access the dispatcher if it needs:
class MyListener
{
public function __construct(EventDispatcher $eventDispatcher)
{
//...
}
public function someListener(Event $event)
{
//...
}
}
---------------------------------------------------------------------------
by stof at 2011/11/10 15:20:07 -0800
@drak The issue when injecting the dispatcher in the listener is described in the issue: circular dependency: you need to create the dispatcher before the listener (as it is injected in it) and when the listener is not lazy-loaded (in Silex for instance), you need to create it before the dispatcher.
---------------------------------------------------------------------------
by drak at 2011/11/10 21:15:45 -0800
Indeed, although it might not unreasonable to expect to create the dispatcher first... but anyway I'm convinced!
Injecting the dispatcher could lead to some __very interesting possibilities__ as standard. While we are at it though, we should have a getter and setter for `$name` in the `Event` class and `$event->setName($eventname)` in the `dispatch()` method. Allowing an event to know it's name is very useful. It allows a single listener to be registered for multiple names, and even makes the event object reusable. I don't know why $name was removed, it was in the Symfony 1 dispatcher and while the new dispatcher is brilliant from an OO point of view, missing the name as standard is a big shame.
+1 from me.
Commits
-------
cfc7be2 [CI] travis-ci.org integration
Discussion
----------
travis-ci.org integration
Travis CI is an open, distributed build system for the open source community.
Here the blog post for the PHP support announcement : http://about.travis-ci.org/blog/first_class_php_support_on_travis_ci/
Each commit in the repo will trigger travis-ci.org hook and will launch phpunit against multiple version of PHP (5.3.8, 5.4.0RC1 and soon 5.3.2, 5.3-snapshot and 5.4-snapshot)
For now we noticed almost no false-positive or downtime.
An icon to show the status of the repo (or a specific build) is included in my PR (change pborreli by symfony).
http://about.travis-ci.org/docs/user/languages/php/
This doesn't replace our jenkins platform in any case. it's another way to bulletproof our code and a great way for a contributor to check his Symfony fork before to submit any PR, like that :
[](http://travis-ci.org/pborreli/symfony)
---------------------------------------------------------------------------
by lsmith77 at 2011/11/17 07:55:57 -0800
eventually they will also add support to automatically run tests for PR's
---------------------------------------------------------------------------
by fabpot at 2011/11/17 08:02:29 -0800
I don't want to support more than one CI. Let's concentrate on Jenkins.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/17 08:04:10 -0800
i am sorry .. but this is a bad decision ... mostly because you don't know what travis CI can do compared to our own jenkins. this needs to be discussed.
---------------------------------------------------------------------------
by willdurand at 2011/11/17 08:04:59 -0800
yeah, travis-ci is really really fine to execute tests with different PHP versions, etc without any effort!
---------------------------------------------------------------------------
by stof at 2011/11/17 08:06:59 -0800
also our own Jenkins seems not to work. I cannot even access it on http://ci.symfony.com
---------------------------------------------------------------------------
by lsmith77 at 2011/11/17 08:10:36 -0800
jenkins atm is still necessary to tests on other OS and add artifacts like code sniffer etc.
however travis enables users to easily CI their feature branches, but to make this easy we have to provide the setup of the main repo. its simple and it also doesn't hurt to have another setup in case our setup is broken (like is atm) and it doesn't cost us anything.
---------------------------------------------------------------------------
by pborreli at 2011/11/17 08:10:59 -0800
our jenkins platform already execute tests with multiple version de PHP and OS (Windows, Linux)
Adding .travis.yml file to the official repo will not remove our work in Jenkins platform, but it does add a very good (and free) way for contributors to bulletproof their code before submit a PR.
I don't want to compare travis-ci and Jenkins, this is not the same architecture, Jenkins is configurable, Travis is distributed you don't have to install it you register, click and it's done.
---------------------------------------------------------------------------
by stof at 2011/11/17 08:53:31 -0800
@pborreli does the Jenkins CI works currently ? And if yes, did the url change ?
---------------------------------------------------------------------------
by stealth35 at 2011/11/17 09:00:26 -0800
👍 for Travis CI
---------------------------------------------------------------------------
by jmikola at 2011/11/17 09:59:28 -0800
I would urge a reconsideration here as well. If not now, then perhaps in the near future when Travis CI's anticipated features come to fruition. The dot-file addition seems even less invasive than adding `composer.json` files to our various components, and both are strong gestures of support for these community initiatives.
---------------------------------------------------------------------------
by hhamon at 2011/11/20 23:56:28 -0800
Travis CI only runs the tests suite if I'm not mistaking so it's a big -1 for me against Jenkins.
With Jenkins we can other analysis tools like PMD, PDepend, Docblox, PHPLOC, PHPCPD...
---------------------------------------------------------------------------
by willdurand at 2011/11/21 00:03:17 -0800
This is not the same usage sir!
Without any effort you'll be able to test all branches on different PHP version, and to provide a visual feedback.
There is nothing to do... I don't know why you're against.
Le 21 nov. 2011 à 08:56, Hugo Hamon<reply@reply.github.com> a écrit :
> Travis CI only runs the tests suite if I'm not mistaking so it's a big -1 for me against Jenkins.
>
> With Jenkins we can other analysis tools like PMD, PDepend, Docblox, PHPLOC, PHPCPD...
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2664#issuecomment-2812352
---------------------------------------------------------------------------
by lsmith77 at 2011/11/21 00:08:01 -0800
did you read up on what travis actually offers? did you read my RFC on the mailinglist?
if did you would realize that your answer misses the point entirely.
it would be like me saying big -1 for jenkins since we do not test feature branches and PRs.
again the topic isnt if travis replaces jenkins, the two are simply complementary services and travis requires the addition of a single file in order to do its magic. thats it.
On 21.11.2011, at 08:56, Hugo Hamon <reply@reply.github.com> wrote:
> Travis CI only runs the tests suite if I'm not mistaking so it's a big -1 for me against Jenkins.
>
> With Jenkins we can other analysis tools like PMD, PDepend, Docblox, PHPLOC, PHPCPD...
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2664#issuecomment-2812352
---------------------------------------------------------------------------
by pborreli at 2011/11/21 00:08:06 -0800
@hhamon test execution and code analysis are two different parts of continuous integration, travis-ci only focuses (for now) on test execution and does it nicely, and this is what you are looking for as contributor when you want to work in Symfony code to provide a bug fix.
---------------------------------------------------------------------------
by hhamon at 2011/11/21 00:33:22 -0800
For me Travis-CI is just a new fashion and fancy tool. I agree with you it's nice because you can have CI with minimum effort but this tool also has drawbacks:
* Limited features
* This is an hosted platform so if Travis is down, we won't have CI
Jenkins is made for that and it's a proven solution. Why would you like to add or change to a distributed fancy tool instead of using a standard and proven tool.
Jenkins does the job well, I'm for having only one CI tool that does not rely on an external service.
My 2 cents.
---------------------------------------------------------------------------
by pborreli at 2011/11/21 00:38:07 -0800
This is not about Symfony having a CI or not, this is about contributors able to test their commits against multiple version of PHP before making a PR.
---------------------------------------------------------------------------
by hhamon at 2011/11/21 00:42:08 -0800
So you mean the CI of the main repository of symfony will remain in Jenkins but you want the contributors to integrate their branches in Travis before asking for a pull request in symfony/symfony?
---------------------------------------------------------------------------
by lsmith77 at 2011/11/21 00:46:40 -0800
@hhamon come on .. please read before posting.
here is my related RFC http://groups.google.com/group/symfony-devs/browse_frm/thread/723964b675c698dd
---------------------------------------------------------------------------
by pborreli at 2011/11/21 00:53:10 -0800
Symfony is already under Jenkins, this will not change. (server will be back public this week)
Adding .travis.yml to the main repo will provide :
- ability for any contributor to monitor any branch before PR (they can't add it without exclude it from PR) easily with travis-ci.org
- an aditional tool to monitor test execution of the main repository.
---------------------------------------------------------------------------
by hhamon at 2011/11/21 01:04:13 -0800
I'm semi convinced by testing PRs. At first it looks an interesting idea in the fact that we could check the PR before integration. But, anyway, when a PR is integrated by Fabien, the tests suite is run one more time and the code pushed to the master is rechecked just after with our Jenkins.
Double check looks interesting but do we really need it? I'm not convinced, especially if we have a well configured custom CI server based on Jenkins.
---------------------------------------------------------------------------
by pborreli at 2011/11/21 01:10:52 -0800
i said **before** PR, means you can play, fix, test your commit on your own fork easily.
---------------------------------------------------------------------------
by willdurand at 2011/11/21 01:14:06 -0800
@hhamon that way you'll be able to configure your fork and test your branches before to PR them...
It's really useful.
Le 21 nov. 2011 à 10:10, Pascal Borreli<reply@reply.github.com> a écrit :
> i said **before** PR, means you can play, fix, test your commit on your own fork easily.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/symfony/symfony/pull/2664#issuecomment-2812921
---------------------------------------------------------------------------
by hhamon at 2011/11/21 01:16:19 -0800
Ok so I'm still unconvinced.
* How many contributors do really write unit tests when they submit a patch?
* How many of them do really launch the tests suite before committing?
* How many contributors do really know what unit tests are and what CI is?
I'm sure they are just a few...
So, asking them to CI and monitor their code before sending a PR is not so simple.
---------------------------------------------------------------------------
by schmittjoh at 2011/11/21 01:18:22 -0800
👍 this is only viable if Travis CI automatically does that on PRs and posts the result as comment.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/21 01:25:31 -0800
Most contributors do know about unit tests and CI, since most PR's are send by a small group of people. Those that do not yet know about unit tests and CI should learn about that. Furthermore the point is that thx to travis setting up automated testing of all branches in your symfony fork requires going to a page and clicking on one button.
And yes fairly soon travis will automatically monitor and test PR's even if the person who send the PR has no clue what travis, unit tests or CI servers are.
But for me as a contributor its a huge advantage already in its current form. This opposition is extremely frustrating because you are complaining about what amounts to a freaking single file called ".travis.yml" that provides real value to several frequent contributors. I am sorry but there is nothing to discuss here but to simply merge.
---------------------------------------------------------------------------
by stloyd at 2011/11/21 01:28:24 -0800
@hhamon All your question is answered [here](http://symfony.com/contributors), check commits from top 20-30 contributors. Check how many commit were done by this amount of contributors. Most of them test PR before commiting or even better they add/fix existing tests.
If I can test my code automaticlly and I don't need to run my own CI or _whatever_ (yes, I'm lazy, as most of us, programmers, thats why we develop automatic jobs and other cool stuff), I will love it, I will use it, I will propagate it to others.
So I'm totally 👍.
---------------------------------------------------------------------------
by hhamon at 2011/11/21 01:34:54 -0800
I totally agree on the fact that all contributors should be aware / learn what unit tests are and what CI is. I also agree on the fact that most important symfony contributors are quite few people that know these subjects. But as a contributor, I don't care about monitoring my own commit as my fork only contains my current commits before submitting a patch I just want to make sure the tests suite passes on my computer.
The core team will then integrate my code, relaunch the tests suite, push to the master that is continuously integrated by Jenkins. I think they are enough security checks on the code base with our current workflow.
Anyway, I'm not against Travis or any other CI tool. I just find the proposed CI process overweight.
---------------------------------------------------------------------------
by lsmith77 at 2011/11/21 01:37:30 -0800
"overweight" there is no weight at all .. there is nothing of any significants to do .. the proposed solution is "weightless" but provides real world benefits. not sure why you are not getting that.
---------------------------------------------------------------------------
by pierrejoye at 2011/11/21 01:37:34 -0800
@fabpot
Bonjour :)
I'd to suggest to reconsider this proposal.
CI is not the business of a single (and limited) tool like Jenkins. What jenkins provides is a small part of what a project need. Travis provides other features and possibilities for the non "core" developers or for forks, read: Get contributors test their stuff in a very easy and efficient way, without having to worry about CI or having much clue about it. Only a matter of adding a note to your "how to contribute" howto.
I'm also sure other tools may make their way to the PHP world in a near future. Focuson on Jenkins only only because it has a hype is a mistake imo. Just like what some do in php.net, it only does 10% of what we actually need there, for example.
---------------------------------------------------------------------------
by hhamon at 2011/11/21 01:42:18 -0800
Anyway, I'm certainly wrong and it's just my point of view at the moment. I first need to experiment the whole worklow to be convinced and give a +1.
---------------------------------------------------------------------------
by pierrejoye at 2011/11/21 01:46:15 -0800
@hhamon also it is one single file (or so), don't see how it can hurt or bloat sf2 repo :)
---------------------------------------------------------------------------
by videlalvaro at 2011/11/21 01:46:27 -0800
I don't understand what's the opposition against travis-ci.
a) The discussion is not about Jenkins vs. Travis as some guys seem to put it. No one is talking of replacing Jenkins.
b) For me using travis is about transparency. It's about letting the community, both contributors and users, see what's going on with the new additions to the framework, both in master branch and in PRs. Yes you can do that if the Symfony CI, but that means we need to know the CI URL of every OS project we use. With travis you get all that in a central place. Before cloning a repo on github I can already see their build status and decide on based on it's build status.
Also as @jmikola said… this is as intrusive as adding the `composer.json` to our packages. Is just **one** invisible file in the repo source tree.
My 2.-
---------------------------------------------------------------------------
by Palleas at 2011/11/21 01:47:05 -0800
A discussion about adding or not a single (!) yml file to the main symfony repo and integrating a tool that would provide nothing but awesomeness to this community. Interesting.
Big 👍 on this.
---------------------------------------------------------------------------
by bergie at 2011/11/21 01:50:54 -0800
+1 from me for Travis CI integration. While it is great that Symfony2 has its own dedicated CI environment, having Travis _in addition to that_ only brings benefits:
* Easy for contributors to test their own stuff before sending pull requests
* More visibility to the fact that Symfony2 is a high-quality, tested PHP framework, especially outside traditional PHP circles
* Handy fallback for when the official CI environment is down
I wrote some notes on how Travis is already helping us in Midgard development: http://lists.midgard-project.org/pipermail/dev/2011-November/003111.html
---------------------------------------------------------------------------
by naholyr at 2011/11/21 04:47:46 -0800
How can one be against adding a 5-lines dot-file in a project and let it work alone forever, without configuring anything or even consuming any resource of yours ?
Can anyone provide an easier way for contributors to test their changes ? The "forker" just has to log in travis-ci.org and click "on", and his changes are tested, no need to install anything.
If that still matters, I highly 👍 Travis integration for all those reasons.
---------------------------------------------------------------------------
by digitalkaoz at 2011/11/21 06:31:35 -0800
simply +1 for integrating travis...
but for every component, there will be more than just one `.travis.yml` ;)
---------------------------------------------------------------------------
by stloyd at 2011/11/21 06:36:05 -0800
@digitalkaoz Travis should be added only to main folder, just like in this PR, nowhere else. Components are __not__ disbrubuted with tests, so there is no need for adding there `.travis.yml`.
---------------------------------------------------------------------------
by kertz at 2011/11/21 06:46:45 -0800
👍 for Travis
---------------------------------------------------------------------------
by cordoval at 2011/11/21 07:14:02 -0800
👍 for travis
---------------------------------------------------------------------------
by simensen at 2011/11/21 09:13:40 -0800
+1 for Travis.
As far as I can tell, it is virtually zero work to support Travis. Given that it would provide us with a way to ensure that the code being submitted at leasts passes the basic `phpunit` tests I think it is well worth the amount of work (zero) it will take to support it.
If down the line it becomes a pain (for example, maybe certain test need to be rewritten to work correctly on Travis...) we can revisit it. But for now it seems like all pros and very little cons to use Travis *in addition to* Jenkins.
---------------------------------------------------------------------------
by ivanrey at 2011/11/21 20:48:16 -0800
+1 for travis
Commits
-------
413756c [BC break][SecurityBundle] Changed the way to register factories
Discussion
----------
[BC break][SecurityBundle] Changed the way to register factories
As discussed in #2454, this changes the way to register the factories to let each bundles register the factories it provides.
Commits
-------
2adc36c [Security] renamed security option to erase_credentials
104b697 [Security] added configurable option security.erase_credentials_from_token
ede55d2 [Security] added configuration parameter for AuthorizationManagerProvider
Discussion
----------
[Security] added configuration parameter to AuthorizationManagerProvider
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: 2657
Todo: -
---------------------------------------------------------------------------
by fabpot at 2011/11/16 10:30:34 -0800
You should also add a configurable option under the `security` entry to ease the configurability.
Commits
-------
4d1e352 To avoid having 2 different default values for env.
Discussion
----------
To avoid having 2 different default values for env.
- 1 come from app/console
- application default env value ignore kernel env.
Commits
-------
5e4b7cb Renamed Propel Bridge: Propel => Propel1
cdad7ab Introducing the Propel Bridge
Discussion
----------
Introducing the Propel Bridge
Basic bridge with stable components for Propel.
This should not affect anything except the need to maintain this code (even if the most part is safe thanks to `@api` tag). As Propel future is linked to Symfony2, to maintain this bridge should not be a problem.
Don't flame me for this proposal, I do that "knowingly".
Regards,
William.
---------------------------------------------------------------------------
by stof at 2011/09/15 12:22:12 -0700
IMO, it would be better to put this code in a repo owned by the Propel organization than in the core.
---------------------------------------------------------------------------
by Richtermeister at 2011/09/15 15:33:02 -0700
Yay, great to see this, very much looking forward to using Propel again.
---------------------------------------------------------------------------
by GromNaN at 2011/09/16 01:37:53 -0700
+1 for @stof proposition.The Silex Core should stay lightweight.
Silex definitely need a site or at least a page in the doc to list all the community extensions.
---------------------------------------------------------------------------
by odino at 2011/09/16 01:39:40 -0700
Silex? :)
---------------------------------------------------------------------------
by GromNaN at 2011/09/16 01:43:20 -0700
Oups, I was looking at Silex PR and got this PR.
---------------------------------------------------------------------------
by lsmith77 at 2011/10/08 01:01:56 -0700
@willdurand we should maybe make this a topic for the next IRC meeting.
---------------------------------------------------------------------------
by willdurand at 2011/10/09 10:26:22 -0700
Agreed :)
---------------------------------------------------------------------------
by willdurand at 2011/11/03 14:18:02 -0700
Good to go ?
---------------------------------------------------------------------------
by willdurand at 2011/11/04 03:34:37 -0700
Just removed the `Query` class. /cc @Stof
Anything else?
---------------------------------------------------------------------------
by willdurand at 2011/11/05 08:45:34 -0700
@fabpot : good to merge ?
The PropelBundle has a `bridge` branch. I'm ready, I'm just waiting you merge this PR to tag the PropelBundle for Symfony 2.0, and after that I'll merge the `bridge` branch into the `master`.
---------------------------------------------------------------------------
by fabpot at 2011/11/16 22:32:42 -0800
What about renaming the directory from `Propel` to `Propel1`? That way, we will be able to have a `Propel` bridge for Propel 2.0.
---------------------------------------------------------------------------
by willdurand at 2011/11/16 23:18:56 -0800
It won't be consistent with the PropelBundle but I guess we don't have any other choice.. So I'm +1 for that.
If it's ok, I'll update this PR, just tell me.
---------------------------------------------------------------------------
by fabpot at 2011/11/17 07:09:58 -0800
yes, +1 for renaming
---------------------------------------------------------------------------
by lsmith77 at 2011/11/17 07:11:45 -0800
Why rename it to ``Propel1``? I think its enough to eventually add a ``Propel2``.
---------------------------------------------------------------------------
by willdurand at 2011/11/17 07:14:05 -0800
`Propel1` is for BC.
`Propel` will be the Propel's future :)
---------------------------------------------------------------------------
by lsmith77 at 2011/11/17 07:17:02 -0800
sounds like a bad idea .. and what happens when you come out with ``Propel3`` ?
---------------------------------------------------------------------------
by stof at 2011/11/17 07:17:31 -0800
@willdurand Maybe the bundle should renamed the same way, for consistency and to let ``PropelBundle`` for the Propel 2 one ? (but this should probably be discussed in another issue tracker)
---------------------------------------------------------------------------
by willdurand at 2011/11/17 07:30:21 -0800
That way we'll be able to handle both Propel 1 & 2 without BC break. You may want to upgrade Symfony2 but not Propel nor PropelBundle. Propel1 bridge has a limited lifetime.
@stof : the PropelBundle will be tagged and a branch will probably appear for Propel1 compatibility.
---------------------------------------------------------------------------
by stof at 2011/11/17 07:34:10 -0800
@willdurand if Symfony provides a Propel bridge using the same namespace for Propel2 and then Propel3, this means that the Sf2 update changing the bridge to use the Propel3 code will make Sf2 incompatible with Propel2 even if you have a tag for Propel2 in the PropelBundle (as you will need to downgrade Symfony to the older tag too). As long as bridges are in the main Symfony repo, they are upgraded the same time Symfony is upgraded and they can bump the requirements.
---------------------------------------------------------------------------
by willdurand at 2011/11/17 07:37:13 -0800
Yes but Propel 1 is frozen, almost dead as we won't add any new features.
Propel2 is the future and there is no plan for a Propel3 which will break BC.
---------------------------------------------------------------------------
by willdurand at 2011/11/17 07:57:05 -0800
Updated!
Commits
-------
c89d45b Fix cs
Discussion
----------
Fix cs
---------------------------------------------------------------------------
by rouffj at 2011/11/17 23:30:29 -0800
lsmith77: ok, so i revert my change only on RequestMatcher file ?
Commits
-------
6d7e6a8 [DoctrineBundle] Enhance error reporting during mapping validation when nested exceptions occur.
Discussion
----------
Doctrine info mapping exception
Better error handling when nested exceptions occur (which is commonly possible with reflection errors).
---------------------------------------------------------------------------
by fabpot at 2011/11/16 02:10:13 -0800
It should probably be done on the 2.0 branch, no?
---------------------------------------------------------------------------
by beberlei at 2011/11/18 02:11:06 -0800
Its not necessarily a bugfix but an enhancement.
Commits
-------
d2195cc Fixed phpdoc and updated the changelog
9e41ff4 [SecurityBundle] Added a validation rule
b107a3f [SecurityBundle] Refactored the configuration
633f0e9 [DoctrineBundle] Moved the entity provider service to DoctrineBundle
74732dc [SecurityBundle] Added a way to extend the providers section of the config
Discussion
----------
[WIP][SecurityBundle] Added a way to extend the providers section of the config
Bug fix: no
Feature addition: yes
BC break: <del>no (for now)</del> yes
Tests pass: yes
This adds a way to extend the ``providers`` section of the security config so that other bundles can hook their stuff into it. An example is available in DoctrineBundle which is now responsible to handle the entity provider (<del>needs some cleanup as the service definition is still in SecurityBundle currently</del>). This will allow PropelBundle to provide a ``propel:`` provider for instance.
In order to keep BC with the existing configuration for the in-memory and the chain providers, I had to allow using a prototyped node instead of forcing using an array node with childrens. This introduces some issues:
- impossible to validate easily that a provider uses only one setup as prototyped node always have a default value (the empty array)
- the ``getFixableKey`` method is needed in the interface to support the XML format by pluralizing the name.
Here is my non-BC proposal for the configuration to clean this:
```yaml
security:
providers:
first:
memory: # BC break here by adding a level before the users
users:
joe: { password: foobar, roles: ROLE_USER }
john: { password: foobarbaz, roles: ROLE_USER }
second:
entity: # this one is BC
class: Acme\DemoBundle\Entity\User
third:
id: my_custom_provider # also BC
fourth:
chain: # BC break by adding a level before the providers
providers: [first, second, third]
```
What do you think about it ? Do we need to keep the BC in the config of the bundle or no ?
Btw note that the way to register the factories used by the firewall section should be refactored using the new way to provide extension points in the extensions (as done here) instead of relying on the end user to register factories, which would probably mean a BC break anyway.
---------------------------------------------------------------------------
by lsmith77 at 2011/10/23 09:19:23 -0700
i don't think we should keep BC. the security config is complex as is .. having BC stuff in there will just make it even harder and confusing.
---------------------------------------------------------------------------
by willdurand at 2011/10/23 09:41:25 -0700
Is the security component tagged with `@api` ?
So basically, we just have to create a factory (`ModelFactory` for instance) and to register it in the `security` extension, right ? Seems quite simple to extend and much better than the hardcoded version…
Why did you call the method to pluralize a key `getFixableKey` ?
---------------------------------------------------------------------------
by beberlei at 2011/10/23 14:48:26 -0700
Changing security config will introduce risk for users. We should avoid that
---------------------------------------------------------------------------
by stof at 2011/10/23 15:34:47 -0700
@beberlei as the config is validated, it will simply give them an exception during the loading of the config if they don't update their config.
---------------------------------------------------------------------------
by stof at 2011/10/24 01:01:42 -0700
@schmittjoh @fabpot Could you give your mind about it ?
---------------------------------------------------------------------------
by stof at 2011/10/31 17:08:12 -0700
@fabpot @schmittjoh ping
---------------------------------------------------------------------------
by stof at 2011/11/11 14:08:18 -0800
I updated the PR by implementing my proposal as the latest IRC meeting agreed that we don't need to keep the BC for this change. This allows to add the validation rule now.
---------------------------------------------------------------------------
by stof at 2011/11/16 11:16:06 -0800
@fabpot ping
---------------------------------------------------------------------------
by fabpot at 2011/11/16 22:29:05 -0800
@stof: Before merging, you must also add information about how to upgrade in the CHANGELOG-2.1.md file.
---------------------------------------------------------------------------
by stof at 2011/11/17 00:01:23 -0800
@fabpot done
