[Extended] Remove deprecated code and add new implementation for Symfony 3.0 #371

luispabon · 2015-12-16T10:30:05Z

This is meant to supersede #358 as @ghostika seems to have gone away. I believe it now addresses all the issues raised, please code review.

@sekl, it has your patch in it, although not entirely sure why the link to your github user got lost on my cherry pick.

Closes #366

luispabon · 2015-12-16T10:50:34Z

I think Travis is playing up, some of the containers running the tests are missing the mongo extension.

luispabon · 2015-12-16T11:10:35Z

It's definitely a travis issue, they've borked their containers.

Modified travis config to test stable symfony 2.8/3 on php 5.6/7.

luispabon · 2015-12-16T11:13:36Z

@stof any thoughts on this PR?

stof · 2015-12-16T12:09:09Z

Controller/AuthorizeController.php

 {
+    use ContainerAwareTrait;


not good as the bundle supports PHP 5.3. Implement the interface without using the trait

Same as per php 5.4... it's not supported any longer. Do you want to burden yourself with it?

well, I'm quite sure that some people still run it when they run the Symfony 2.3 LTS. And it would not be good to force to run an unmaintained version of the bundle for something related to security.
And given that this bundle is low maintenance, I don't think maintaining 2 branches just to avoid 4 small lines of codes here. There is no burden here.

I think dropping support for them might be done at the same time we drop support for Symfony 2.3 (which reaches end of maintenance in 6 months)

luispabon · 2015-12-16T12:45:14Z

All concerns addressed except for the one regarding TokenStorageCompilerPass, please see comment above @stof .

… support the new mongodb php driver extension - they're still stuck on legacy mongo extension

luispabon · 2015-12-21T11:14:22Z

Okay, this bundle cannot support PHP7 for the time being as doctrine/mongo-odm and dependencies depend on the legacy mongo extension, instead of the new mongodb extension.

oauth2-php needs updating also to symfony 3 as it depends on httpfoundation 2.0

I'm trying to fix the latter.

luispabon · 2015-12-21T11:20:06Z

FriendsOfSymfony/oauth2-php#88

stof · 2015-12-21T11:23:54Z

We can totally support PHP 7, as there is no hard dependency on the ODM. It is an optional dependency.

stof · 2015-12-21T11:24:47Z

Controller/AuthorizeController.php

@@ -127,10 +146,15 @@ protected function getRedirectionUrl(UserInterface $user)
    protected function getClient()
    {
        if (null === $this->client) {
-            if (null === $clientId = $this->container->get('request')->get('client_id')) {
+            try {
+                $request = $this->container->get('request_stack')->getCurrentRequest();


you need to handle the case of $request being null here to be safe

luispabon · 2015-12-21T11:25:09Z

Understood, but you must realise builds will fail, and I imagine tests also, on PHP7 until doctrine/mongo upgrades to the new extension.

stof · 2015-12-21T11:25:15Z

Form/Handler/AuthorizeFormHandler.php

@@ -41,13 +59,19 @@ public function isRejected()

    public function process()
    {
+        try {
+            $request = $this->container->get('request_stack')->getCurrentRequest();


same here about handling of null

done on both accounts

stof · 2015-12-21T11:26:47Z

@luispabon no. tests relying on mongo should simply be skipped if the mongo extension is not available

luispabon · 2015-12-21T11:28:22Z

Fair do's, although that's out of scope for this particular PR. This is symfony 3.0 compat only. Builds will still fail due to composer not being able to resolve dependencies. I've moved php7 to allow_failures on travis.

stof · 2015-12-21T11:31:03Z

.travis.yml

-      env: SYMFONY_VERSION=2.8.*@dev
+      env: SYMFONY_VERSION=2.8.*
+    - php: 7.0
+      env: SYMFONY_VERSION=2.8.*


there is no reason to run tests for Symfony 2.8 twice, on 5.6 and 7.0. The bundle is already tested on both different versions of PHP, and Symfony itself too.

luispabon · 2015-12-21T11:31:24Z

Also @stof, if you have any hand on the FOS oauth2-php, we need this FriendsOfSymfony/oauth2-php#88 merged and a new release cut. Otherwise composer won't be able to resolve working dependencies when using this alongside symfony 3.

stof · 2015-12-21T11:31:56Z

Fair do's, although that's out of scope for this particular PR. This is symfony 3.0 compat only. Builds will still fail due to composer not being able to resolve dependencies. I've moved php7 to allow_failures on travis.

Given that running tests on PHP 7 is something you added, it is in the scope. Otherwise, you should not add PHP 7 at all in this PR, and do a separate PR for it (which might indeed be a good idea though)

luispabon · 2015-12-21T11:36:29Z

Fair enough, php7 was on the original PR and I expanded travis config for it. I've removed it entirely, best left for a different PR. I've also bumped all symfony components to allow for 3.0. Will have a look at the token compiler pass when I have some time.

We're blocked by oauth2-php atm.

stof · 2015-12-21T11:37:07Z

We're blocked by oauth2-php atm.

The PR has been merged, even though it is not released yet

luispabon · 2015-12-21T11:38:10Z

👍

stof · 2015-12-21T11:38:11Z

.travis.yml

@@ -20,17 +20,14 @@ matrix:
    - php: 5.6
      env: SYMFONY_VERSION=2.6.*


I suggest removing 2.5 and 2.6 here, as they reached end of maintenance. This will avoid making the Travis build too big.

Will do. The travis config is also asking for php 5.4 and 5.5, although they're not being tested. 5.3 is tested however. Anything you'd like to do about this?

PHP 5.4 and 5.5 are tested. They are running on builds which are not modifying the Symfony requirement

luispabon · 2015-12-22T13:49:02Z

Addressed the remaining concerns. Still blocked on oauth2-php, unless we switch to dev_master.

luispabon · 2015-12-22T15:30:52Z

I won't be near a computer for the next 10 days, please feel free to complete this as needed.

GuilhemN · 2016-01-13T14:53:36Z

Closed in favor of #374

ghostika and others added 6 commits October 20, 2015 15:22

Remove deprecated code and add new implementation for Symfony 3.0

5277ced

Keep BC Layer

549d281

Fix test, remove passed request variable

777b004

Fix test

cc114ab

Cleanup

80c7f23

FormInterface::submit no longer accepts a Request object

2fc2498

luispabon mentioned this pull request Dec 16, 2015

Remove deprecated code and add new implementation for Symfony 3.0 #358

Closed

luispabon closed this Dec 16, 2015

luispabon reopened this Dec 16, 2015

fix missing namespace, ContainerAwareTrait usage for sf3

7f6dcec

Update travis config to test with symfony 2.8/3.0 on php 5.6/7.0

29f4843

stof reviewed Dec 16, 2015
View reviewed changes

luispabon closed this Dec 17, 2015

luispabon reopened this Dec 17, 2015

luispabon closed this Dec 18, 2015

luispabon reopened this Dec 18, 2015

Luis Pabon added 2 commits December 18, 2015 16:25

Ensure PHP 5.3 and Symfony 2.3 compat

0fcaa5f

Allow PHP7 failure as doctrine/mongo-*** libs we depend on do not yet…

4ad271d

… support the new mongodb php driver extension - they're still stuck on legacy mongo extension

stof reviewed Dec 21, 2015
View reviewed changes

Luis Pabon added 2 commits December 21, 2015 11:34

Remove php 7 from travis build

4056762

Allow 3.0 symfony components we depend on

0418d93

stof reviewed Dec 21, 2015
View reviewed changes

Luis Pabon added 3 commits December 21, 2015 11:43

Remove travis tests for symfony 2.5/6 due to EOL reached

5c61688

Handle cases in which Request is null

cbcc860

Handle lack of service security.token_storage on compiler pass only

6969bc9

j0k3r mentioned this pull request Dec 27, 2015

Support Symfony dependencies 3.0 #373

Closed

GuilhemN mentioned this pull request Jan 1, 2016

[Extended] Add symfony 3.0 support #374

Merged

lsmith77 assigned GuilhemN Jan 1, 2016

GuilhemN closed this Jan 13, 2016

		@@ -20,17 +20,14 @@ matrix:
		- php: 5.6
		env: SYMFONY_VERSION=2.6.*

[Extended] Remove deprecated code and add new implementation for Symfony 3.0 #371

[Extended] Remove deprecated code and add new implementation for Symfony 3.0 #371

Uh oh!

Conversation

luispabon commented Dec 16, 2015

Uh oh!

luispabon commented Dec 16, 2015

Uh oh!

luispabon commented Dec 16, 2015

Uh oh!

luispabon commented Dec 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luispabon commented Dec 16, 2015

Uh oh!

luispabon commented Dec 21, 2015

Uh oh!

luispabon commented Dec 21, 2015

Uh oh!

stof commented Dec 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luispabon commented Dec 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Dec 21, 2015

Uh oh!

luispabon commented Dec 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luispabon commented Dec 21, 2015

Uh oh!

stof commented Dec 21, 2015

Uh oh!

luispabon commented Dec 21, 2015

Uh oh!

stof commented Dec 21, 2015

Uh oh!

luispabon commented Dec 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luispabon commented Dec 22, 2015

Uh oh!

luispabon commented Dec 22, 2015

Uh oh!

GuilhemN commented Jan 13, 2016

Uh oh!

Uh oh!