Attempting Symfony 4 support #300

weaverryan · 2017-11-07T15:06:01Z

Hi guys!

This adds Symfony 4 support. I'm not sure if any changes to the code are needed, let's run the tests and find out!

Cheers!

tobias-93 · 2017-11-07T15:22:32Z

It would certainly be nice if we can support SF4!
Could you add Travis config to test this?
I also see that some tests are failing due to some JS error, will have a look at that later on.

tobias-93 · 2017-11-08T21:02:35Z

@weaverryan I have fixed the tests for the current master, so you should be able to see what's going on if you add the required Travis config.

ElectricMaxxx · 2017-11-15T23:31:35Z

Anything going on here?

weaverryan · 2017-11-16T05:49:41Z

Haha, yep. I forgot about this for a few days! But I (probably) just finished it:

composer.json now supports Symfony 4
tests now run Symfony 4
Symfony 2.3 support was dropped, which made fixing the 1 test failure on Symfony 4 quite easy
Any public services were explicitly marked as public="true" so that they remain public in Symfony 4

So, no behavior changes or BC breaks, and the bundle (assuming the tests do pass) is now Symfony 4 compat!

weaverryan · 2017-11-16T05:51:17Z

Command/RouterDebugExposedCommand.php

@@ -23,18 +26,23 @@
 *
 * @author      William DURAND <william.durand1@gmail.com>
 */
-class RouterDebugExposedCommand extends RouterDebugCommand
+class RouterDebugExposedCommand extends ContainerAwareCommand


RouterDebugCommand became final in Symfony 3.4. But we only extended it for Symfony 2.3 support. So, it was an easy win to stop extending it.

weaverryan · 2017-11-16T05:51:33Z

Command/RouterDebugExposedCommand.php

+                new InputOption('show-controllers', null, InputOption::VALUE_NONE, 'Show assigned controllers in overview'),
+                new InputOption('format', null, InputOption::VALUE_REQUIRED, 'The output format (txt, xml, json, or md)', 'txt'),
+                new InputOption('raw', null, InputOption::VALUE_NONE, 'To output raw route(s)'),
+            ))


These are a copy of the options that the previous parent class added for us.

weaverryan · 2017-11-16T05:52:12Z

Tests pass! This should be ready for merge and a new release to make it available :)

ElectricMaxxx · 2017-11-16T07:00:01Z

.travis.yml

@@ -17,6 +19,7 @@ cache:
    - $HOME/.composer/cache/files

 before_install:
+  - if [ "$DEPENDENCIES" = "beta" ]; then composer config minimum-stability beta; fi;


General question: what is better? require ^4.0@dev and doing a minimum-stability: dev here, or going explicit on beta to catch releases only?

It's a great question :). I chose to do beta because I don't think it makes sense to always test against the latest (effectively dev-master) version of Symfony. We don't really care of the bundle is compatible with a new Symfony version until it becomes beta at least. So I think this is the best config.

But we would have to change twice:

from beta to RC

from RC to stable
having dev for that period of try and fail would cause only one change then: from dev -> stable.
But i think the point of having released versions only sounds valid.

I don't think we would need to change this again. This is the minimum stability, so I think when 4.0.0 stable is released, this line would cause 4.0.0 to be downloaded. This should say "use the latest beta, RC or stable version".

ElectricMaxxx

Just a minor issue.

ElectricMaxxx · 2017-11-16T20:31:38Z

composer.json

-        "symfony/serializer": "~2.3||~3.0",
-        "symfony/console": "~2.3||~3.0",
+        "php": ">=5.3.9",
+        "symfony/framework-bundle": "~2.7||~3.0|^4.0",


why ~ 2.7 and not 2.8 as the LTS?

2.7 is LTS
ref: https://symfony.com/roadmap?version=2.7#checker

weaverryan · 2017-11-18T09:43:29Z

Comments addressed!

weaverryan · 2017-11-18T10:21:53Z

@tobias-93 This should be ready. Someone at the hack day has been using/testing this bundle on Symfony 4 with no issues. So beyond the tests passing, I think it should be good :). I'm told this is blocking doctrine/phpcr-bundle Symfony 4 support - the merge+tag will fix that.

Thanks!

tobias-93 · 2017-11-18T13:55:06Z

Hi @weaverryan,

It looks good to me, thank you for your work!
However, I do not agree with dropping support for SF 2.7.
As you can see on http://symfony.com/roadmap?version=2.7#checker, it will still be supported for more than 1 year and I think we should do so too, especially because we're not running into any BC conflicts with it.
Could you revert your latest commit?

Thanks!

alexchuin · 2017-12-01T07:47:36Z

Symfony 4.0 is here! Thanks guys 👍

stof · 2017-12-01T08:45:14Z

.travis.yml

@@ -8,6 +8,8 @@ matrix:
    - php: 7.1
    - php: 7.1
      env: SYMFONY_VERSION='2.8.*'


Please do 2 additional things:

add testing on PHP 7.2

add a job forcing testing on the 3.4 LTS, similar to this 2.8 job

stof · 2017-12-01T08:47:41Z

Command/RouterDebugExposedCommand.php

@@ -23,18 +26,23 @@
 *
 * @author      William DURAND <william.durand1@gmail.com>
 */
-class RouterDebugExposedCommand extends RouterDebugCommand
+class RouterDebugExposedCommand extends ContainerAwareCommand


This command has to be defined as a service, otherwise it won't be registered in Symfony 4.

And once defining it as a service, we can use DI in it instead of making it ContainerAware, allowing to avoid making the extractor service public (it is public only because of this command being ContainerAware)

and commands as services were supported in 2.7 already https://symfony.com/doc/2.7/console/commands_as_services.html

stof · 2017-12-01T08:49:03Z

and I agree with @tobias-93 about keeping support for the 2.7 LTS, giving that there is no cost for it at all (we don't have any BC layer for 2.7 support)

ElectricMaxxx · 2017-12-01T23:07:34Z

@weaverryan can we try to finish this one here?

lsmith77 · 2017-12-02T10:04:40Z

composer.json

-        "symfony/framework-bundle": "~2.3||~3.0",
-        "symfony/serializer": "~2.3||~3.0",
-        "symfony/console": "~2.3||~3.0",
+        "php": ">=5.3.9",


should we not also fix the php dep to state the major version explicitly?

lsmith77 · 2017-12-02T10:05:00Z

composer.json

-        "symfony/serializer": "~2.3||~3.0",
-        "symfony/console": "~2.3||~3.0",
+        "php": ">=5.3.9",
+        "symfony/framework-bundle": "~2.8||~3.0|^4.0",


as stated else where .. imho we should keep 2.7 allowed

RouterDebugCommand RouterDebugCommand became final in Symfony 3.4. But it was only extended for 2.3 support anyways.

... so that they remain public in Symfony 4.0

weaverryan · 2017-12-02T18:25:50Z

Thanks for the feedback guys! And sorry for the delay - I lost track of this PR.

I've just made the updates... but my final update caused a conflict. I'll have to look later - the baby is awakening!

;)

weaverryan · 2017-12-02T18:29:16Z

Ha! Nevermind, I finished just in time! Feedback warmly welcome - I'm taking care of everything I'm know of, but I don't know the internals of the bundle super well :).

lsmith77 · 2017-12-03T07:57:44Z

home stretch! DumpCommandTest::testExecuteFormatOption() still needs to be refactored to remove the container

weaverryan · 2017-12-03T18:26:54Z

I don't know how I missed that :). Let's try one more time

tobias-93 · 2017-12-04T08:58:19Z

@weaverryan good work! I see the tests fail because of a deprecation notice existing in PHP 7.2. The deprecated code seems to be in the source of PHPUnit itself (although I cannot test this myself because I have no machine with PHP 7.2 available) and was fixed in a later version. Could you update the version of PHPUnit in the phpunit-script from 6.0 to 6.5? That should include the non-deprecated code.

tobias-93 · 2017-12-04T16:09:30Z

@weaverryan I like skipping the local phpunit script and just using the Symfony PHPunit-bridge script (which was used anyway with some specific configuration).
Could you adjust the docs and remove the phpunit script?
Then I think this is ready to be merged!

lsmith77 · 2017-12-04T16:26:43Z

@stof I think your concerns have been addressed

tobias-93 · 2017-12-07T20:08:12Z

@weaverryan In the end I think the directory configuration was made for a reason, and it's nice that an error message is shown if someone didn't run composer update. I therefore reverted your last commit and fixed the phpunit script. I think this is ready to be merged now.

weaverryan · 2017-12-08T01:43:43Z

I'm fine with that - we can discuss it in another PR. simple-phpunit has a few advantages (like reporting deprecations), but it's certainly out of scope.

So, let's merge!!!

lsmith77 · 2017-12-08T07:15:48Z

composer.json

-        "symfony/framework-bundle": "~2.3||~3.0",
-        "symfony/serializer": "~2.3||~3.0",
-        "symfony/console": "~2.3||~3.0",
+        "php": "^5.3.9|^7.0",


not sure if we still want to support versions below 5.6 .. but if its no extra effort, I guess its fine.

if it works, let's keep it and once it causes an issue, one can increase the min version?

It is in line with the rest of our dependencies, so it makes sense to only increase this if we want to use features present in PHP >5.3.

tobias-93 · 2017-12-08T09:55:27Z

simple-phpunit has a few advantages (like reporting deprecations)

If you look carefully, our own phpunit-script does nothing more than check if simple-phpunit is present and set the working directory, before starting simple-phpunit to do the 'real' work 😉. So no change is needed, really.

tifabien · 2017-12-12T16:19:37Z

Command/DumpCommand.php

+    private $targetPath;
+
+    /**
+     * @var \FOS\JsRoutingBundle\Extractor\ExposedRoutesExtractorInterface


@var ExposedRoutesExtractorInterface is enough as you imported the namespace?

tifabien · 2017-12-12T16:20:36Z

Command/DumpCommand.php

+    private $extractor;
+
+    /**
+     * @var \Symfony\Component\Serializer\SerializerInterface


@var SerializerInterface is enough as you imported the namespace?

tifabien · 2017-12-12T16:24:16Z

Command/RouterDebugExposedCommand.php

 {
+    protected static $defaultName = 'fos:js-routing:debug';
+
+    private $extractor;


Maybe add PHPDocs

weaverryan · 2017-12-13T02:45:05Z

Ping! Who can merge this and tag a release. Is it you @tobias-93? Any issues left?

tobias-93 · 2017-12-13T08:22:33Z

@weaverryan Thank you for your effort, I've merged the changes!

tobias-93 · 2017-12-13T08:28:23Z

I've tagged the 2.1.0 release for your convenience

weaverryan · 2017-12-13T13:26:03Z

That's perfect! Thanks so much! :D

lsmith77 · 2017-12-13T14:21:00Z

thank you @weaverryan !

weaverryan force-pushed the patch-1 branch 2 times, most recently from d3b19a1 to c856552 Compare November 15, 2017 13:56

ElectricMaxxx mentioned this pull request Nov 15, 2017

start on working with symfony 4.0 symfony-cmf/Testing#155

Merged

weaverryan commented Nov 16, 2017

View reviewed changes

ElectricMaxxx reviewed Nov 16, 2017

View reviewed changes

ElectricMaxxx suggested changes Nov 16, 2017

View reviewed changes

stof requested changes Dec 1, 2017

View reviewed changes

ElectricMaxxx mentioned this pull request Dec 1, 2017

Allow Symfony 4.0 symfony-cmf/Routing#213

Merged

lsmith77 reviewed Dec 2, 2017

View reviewed changes

weaverryan and others added 6 commits December 2, 2017 13:24

Attempting Symfony 4 support

c8eabc8

testing beta versions

837743f

Removing Symfony 2.3 support and not extending the base

b2a4249

RouterDebugCommand RouterDebugCommand became final in Symfony 3.4. But it was only extended for 2.3 support anyways.

Explicitly making public services public

0c37caf

... so that they remain public in Symfony 4.0

using latest LTS

727eb40

requested changes

8e57491

registering commands as services and using DI instead of container

d48769b

re-allowing Symfony 2.7 and making sure we don't allow PHP version 8 yet

16ec0e4

;)

weaverryan force-pushed the patch-1 branch from 95ff91f to 16ec0e4 Compare December 2, 2017 18:28

updating last test

a84013d

trying to test with simple-phpunit

53c757d

lsmith77 self-requested a review December 4, 2017 16:26

lsmith77 approved these changes Dec 4, 2017

View reviewed changes

tobias-93 mentioned this pull request Dec 5, 2017

Issue with flex #304

Closed

Fix PHPUnit configuration

a4957a9

Revert execute permissions on phpunit command

ad09f26

lsmith77 reviewed Dec 8, 2017

View reviewed changes

tifabien approved these changes Dec 12, 2017

View reviewed changes

shortening phpdoc

8860848

tobias-93 merged commit 9334e8f into FriendsOfSymfony:master Dec 13, 2017

weaverryan deleted the patch-1 branch December 13, 2017 13:25

Attempting Symfony 4 support #300

Attempting Symfony 4 support #300

Conversation

weaverryan commented Nov 7, 2017

tobias-93 commented Nov 7, 2017

tobias-93 commented Nov 8, 2017

ElectricMaxxx commented Nov 15, 2017

weaverryan commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElectricMaxxx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Nov 18, 2017

weaverryan commented Nov 18, 2017

tobias-93 commented Nov 18, 2017

alexchuin commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Dec 1, 2017

ElectricMaxxx commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Dec 2, 2017

weaverryan commented Dec 2, 2017

lsmith77 commented Dec 3, 2017

weaverryan commented Dec 3, 2017

tobias-93 commented Dec 4, 2017 • edited Loading

tobias-93 commented Dec 4, 2017

lsmith77 commented Dec 4, 2017

tobias-93 commented Dec 7, 2017

weaverryan commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobias-93 Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

tobias-93 commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Dec 13, 2017

tobias-93 commented Dec 13, 2017

tobias-93 commented Dec 13, 2017

weaverryan commented Dec 13, 2017

lsmith77 commented Dec 13, 2017

tobias-93 commented Dec 4, 2017 •

edited

Loading

tobias-93 Dec 8, 2017 •

edited

Loading