Skip to content
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.

Sign up for GitHub

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

Add support for Symfony 6 #179

Merged
merged 1 commit into from
Jan 10, 2022
Merged

Add support for Symfony 6 #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/.idea/
/vendor/
composer.lock
tests/Fixtures/Symfony/cache
tests/Fixtures/Symfony/logs
tests/Fixtures/Symfony/var
tests/Fixtures/Symfony/var
.phpunit.result.cache
10 changes: 5 additions & 5 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,14 @@
"license": "MIT",
"require": {
"php-pm/php-pm": "^2.0",
"symfony/http-foundation": "^3.4|^4.2.12|^5.0",
"symfony/http-kernel": "^3.4|^4.0|^5.0",
"symfony/http-foundation": "^4.2.12|^5.0|^6.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 4.2.12 specifically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure - it was there before, so I left it "as-is". Maybe there is a security issue with versions below 4.2.12?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just notified of GHSA-754h-5r27-7x3r. We should upgrade to ^4.1.13|^5.1.5. I'm not sure if this version constraint can properly be handled?

I'd be happy for follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I'll make a new PR with just that suggested change and see if it works out. Technically SF <5.3 is no longer supported and 5.3 drops out the end of this month. I am not sure how much of an issue that restraint is in the grand scheme of things based on that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then lets just upgrade to a then-supported version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#180 - with a comment about suggested versions.

"symfony/http-kernel": "^4.0|^5.0|^6.0",
"guzzlehttp/psr7": "^1.5"
},
"require-dev": {
"phpunit/phpunit": "^7.0",
"symfony/framework-bundle": "^3.4|^4.1.12|^5.0",
"symfony/yaml": "^3.4|^4.0|^5.0",
"phpunit/phpunit": "^9.5",
"symfony/framework-bundle": "^4.1.12|^5.0|^6.0",
"symfony/yaml": "^4.0|^5.0|^6.0",
"doctrine/annotations": "^1.6"
},
"autoload": {
Expand Down
10 changes: 5 additions & 5 deletions tests/Fixtures/Symfony/Kernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@
use Symfony\Component\Config\Loader\LoaderInterface;
use Symfony\Component\Config\Resource\FileResource;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\Routing\RouteCollectionBuilder;
use Symfony\Component\Routing\Loader\Configurator\RoutingConfigurator;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work for all Symfony versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From 4.X onward, yes I believe so. I figured since this is only used in the tests it would be OK to update it.


class Kernel extends \Symfony\Component\HttpKernel\Kernel
{
use MicroKernelTrait;

const CONFIG_EXTS = '.{php,xml,yaml,yml}';

public function registerBundles()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change the php version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The neyt line‘s signature, sorry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think these would need PHP 7.4? Again: as these were in the tests I worked on the basis that this should be OK. I was running against PHP 8 at the time. I can revert the changes but it's becoming increasingly difficult to have PHP <7.4 and PHP8 code in the same lib. SF6 adds return types to many classes which would then break these anyway so??? What would you prefer to do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't know. But I agree- as these are only the tests it should be ok. Upgrading minimum php version would be a separate PR then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would personally recommend the minimum PHP version be 7.4 since 7.3 is unsupported and does not receive security updates.

I do have some additional updates for ppm just not got around to making PRs for them yet. I don't mind adding that if it works for you guys?

public function registerBundles(): iterable
{
$contents = require $this->getProjectDir() . '/config/bundles.php';
foreach ($contents as $class => $envs) {
Expand All @@ -36,14 +36,14 @@ protected function configureContainer(ContainerBuilder $container, LoaderInterfa
$loader->load($confDir . '/{services}' . self::CONFIG_EXTS, 'glob');
}

protected function configureRoutes(RouteCollectionBuilder $routes)
protected function configureRoutes(RoutingConfigurator $routes)
{
$confDir = $this->getProjectDir() . '/config';

$routes->import($confDir . '/{routes}' . self::CONFIG_EXTS, '/', 'glob');
$routes->import($confDir . '/{routes}' . self::CONFIG_EXTS);
}

public function getProjectDir()
public function getProjectDir(): string
{
return __DIR__;
}
Expand Down
4 changes: 2 additions & 2 deletions tests/SymfonyBootstrapTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@

class SymfonyBootstrapTest extends TestCase
{
public function setUp()
public function setUp(): void
{
ProcessSlave::$slave = new ProcessSlaveDouble();
}

public static function tearDownAfterClass()
public static function tearDownAfterClass(): void
{
$fs = new Filesystem();
$fs->remove(__DIR__.'/Fixtures/Symfony/var');
Expand Down