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.

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

merged 1 commit into from
Jan 10, 2022

Conversation

dave-redfern
Copy link
Contributor

Remove SF 3.4 as it is EOL'd as of November 2021 and no longer receives updates (https://symfony.com/releases/3.4)

Remove SF 3.4 as it is EOL'd as of November 2021 and no longer receives updates (https://symfony.com/releases/3.4)
@dave-redfern
Copy link
Contributor Author

@andig Any chance of getting this PR in for SF6 support too? 🙏

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

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

@andig andig merged commit 1125ab4 into php-pm:master Jan 10, 2022
@dave-redfern dave-redfern deleted the support-sf6 branch January 10, 2022 15:24
dave-redfern added a commit to dave-redfern/php-pm-httpkernel that referenced this pull request Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants