Attempting Symfony 4 support

[weaverryan]

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]

Hi @weaverryan,

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]

@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]

Anything going on here?

[weaverryan]

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]

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

[weaverryan]

Comments addressed!

[weaverryan]

@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]

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]

Symfony 4.0 is here! Thanks guys :+1:

[stof]

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]

@weaverryan can we try to finish this one here?

[weaverryan]

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]

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]

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

[weaverryan]

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

[tobias-93]

@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]

@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]

@stof I think your concerns have been addressed

[tobias-93]

@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]

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!!!

[tobias-93]

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 :wink:. So no change is needed, really.

[weaverryan]

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

[tobias-93]

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

[tobias-93]

I've tagged the 2.1.0 release for your convenience

[weaverryan]

That's perfect! Thanks so much! :D

[lsmith77]

thank you @weaverryan !