New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to Symfony #895

Merged
merged 1 commit into from Dec 17, 2017

Conversation

@derrabus
Copy link
Contributor

derrabus commented Dec 7, 2017

This PR switches the application to Symfony.

  • Upgrade to Symfony 3.4.
  • Eliminate container-awareness.
  • Convert middlewares to event subscribers.
  • Configure a Symfony kernel for OpenCFP.
  • Switch front controller to the Symfony kernel.
  • Switch console to the Symfony kernel.
  • Switch integration tests to the Symfony kernel.
  • Remove the Application and all providers.
  • Remove silex/silex.
  • Raise php requirement to 7.1.
  • Upgrade to Symfony 4.0.

This PR switches OpenCFP to a new Symfony kernel and removes all dependencies to the Silex package.

Fixes #618.

Some notes:

  • I've placed all Symfony configuration in /resources/config because Symfony's default /config is already used for config files that the user is supposed to edit. If you know a better place, please tell me.
  • The routing configuration looks ugly because I use a script to regenerate it every time I rebase against master. We can split that file after the PR has been merged – or switch to annotations if you like.
  • We still depend on a couple of libraries that are not compatible with Symfony 4. I suggest to remain on Symfony 3.4 for the time being.
  • I know the diff is large, but I don't see much that I could pull out of the PR. The biggest part is removing the Silex bootstrap and replacing it with a Symfony one.
  • I did not include Symfony Flex because it enforces a very opinionated repository layout that would clash with the current structure of this application.
}
}
private function loadConfiguration(): array

This comment has been minimized.

@derrabus

derrabus Dec 7, 2017

Contributor

Maybe we should use the Config component to parse the file.

@derrabus derrabus referenced this pull request Dec 7, 2017

Closed

Migration to Symfony 4 #618

@derrabus derrabus force-pushed the derrabus:symfony branch 7 times, most recently from 99650bf to 678d425 Dec 7, 2017

"symfony/security": "^3.4",
"symfony/security-csrf": "^3.4",
"symfony/swiftmailer-bundle": "^2.6",

This comment has been minimized.

@derrabus

derrabus Dec 8, 2017

Contributor

This is going to block Symfony 4 as well. But we need to upgrade to Swiftmailer 6 to use a newer Swiftmailer bundle.

@derrabus derrabus force-pushed the derrabus:symfony branch 20 times, most recently from 7c12099 to 9da22df Dec 11, 2017

@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Dec 17, 2017

About the doc block comments: They were meant to be a documentation for the team. But I can remove them, if you understand what I'm doing there.

@derrabus derrabus force-pushed the derrabus:symfony branch 2 times, most recently from 58c53d9 to b380de2 Dec 17, 2017

@localheinz

This comment has been minimized.

Copy link
Collaborator

localheinz commented Dec 17, 2017

@derrabus

Ha, no problem! 👍

@localheinz
Copy link
Collaborator

localheinz left a comment

👍

@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Dec 17, 2017

@localheinz I've addressed all of your comments and added your test.

@localheinz

This comment has been minimized.

Copy link
Collaborator

localheinz commented Dec 17, 2017

@BackEndTea @chartjes @mdwheele

Would you like to take a look as well?

@chartjes

This comment has been minimized.

Copy link
Contributor

chartjes commented Dec 17, 2017

@mdwheele

This comment has been minimized.

Copy link
Contributor

mdwheele commented Dec 17, 2017

I'd like to take a look but it will be later tonight.

@derrabus derrabus force-pushed the derrabus:symfony branch 2 times, most recently from d7fb287 to 98060b7 Dec 17, 2017

@@ -16,6 +16,7 @@
<php>
<!-- https://github.com/symfony/console/blob/v3.3.12/Terminal.php#L24-L36 -->
<env name="COLUMNS" value="120"/>
<env name="SYMFONY_DEPRECATIONS_HELPER" value="35"/>

This comment has been minimized.

@derrabus

derrabus Dec 17, 2017

Contributor

This setting allows up to 35 deprecation warnings to be triggered during test execution. I have opened #972 and #973 to address the deprecation warnings, so we can lower this to zero.

This comment has been minimized.

@derrabus

derrabus Dec 17, 2017

Contributor

Both PRs have been merged, so I've removed that line again.

@derrabus derrabus force-pushed the derrabus:symfony branch from 98060b7 to 4ccb9a3 Dec 17, 2017

@derrabus derrabus force-pushed the derrabus:symfony branch from 4ccb9a3 to 08308d6 Dec 17, 2017

@mdwheele

This comment has been minimized.

Copy link
Contributor

mdwheele commented Dec 17, 2017

Big 👍 from me. Looks great. I want to revisit the idea of moving away from our historic config/production.dist.yml et al. towards use of .env, but that can be done in a separate PR.

$container->getAlias(Capsule::class)->setPublic(true);
$container->getDefinition(CallForPapers::class)->setPublic(true);
$container->getDefinition(Sentinel::class)->setPublic(true);

This comment has been minimized.

@derrabus

derrabus Dec 17, 2017

Contributor

To explain myself here: This pass sets some services to public to make them accessible for the integration tests. This is not best practise, but keeps the diff small for now. I will clean this up with a later PR.

Appears concerns have been addressed

@chartjes
Copy link
Contributor

chartjes left a comment

This has come through way faster than I thought it would, and I cannot thank all the contributors enough for their hard work

@chartjes chartjes merged commit a7811fb into opencfp:master Dec 17, 2017

3 of 4 checks passed

codeclimate/diff-coverage 8% (80% threshold)
Details
codeclimate 56 fixed issues
Details
codeclimate/total-coverage 32% (1.2% change)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@derrabus derrabus deleted the derrabus:symfony branch Dec 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment