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

Feature/update bundle to symfony 7 #38

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kurt-junker
Copy link

Hey @konandrum and @BeBlood

can you please check the Update of Symfony Components so we can use your Bundle with the LTS and Symfony 7.0

Thanks a lot.

PS: It would be nice to know how you handle Unit Tests? I didn't find any tests. If you prefer unit/integration tests please let me know and I will do it as soon as possible.

Copy link
Member

@konandrum konandrum left a comment

Choose a reason for hiding this comment

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

Thanks for this great job, some changes needed

{
if (null === $event->getToken() || null === $event->getToken()->getUser()) {
if (null === $event->getToken()?->getUser()) {
Copy link
Member

Choose a reason for hiding this comment

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

Not a good idea to use the null safe operator (uncompatible with < php8)

Copy link
Author

Choose a reason for hiding this comment

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

hmmm Symfony 6.4 requires >= PHP 8.1

composer.json Outdated
"symfony/routing": "^5.3|^6.0",
"symfony/security-bundle": "^5.3|^6.0",
"symfony/http-foundation": "^5.3|^6.0",
"php": ">=8.2",
Copy link
Member

Choose a reason for hiding this comment

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

https://symfony.com/releases/6.4 => requirement php8.1.0 or higher

@konandrum
Copy link
Member

If you wish to add unit tests, will accept contributions with pleasure ! Thanks
Missing time to write them ;)

downgrading phpunit to 10.5
@kurt-junker
Copy link
Author

If you wish to add unit tests, will accept contributions with pleasure ! Thanks Missing time to write them ;)

Hi @konandrum ,

I will create a new issue for the Unit Tests and write them in the next few days.

Have a nice day :)

@kurt-junker kurt-junker requested a review from konandrum March 18, 2024 19:54

/**
* @var string use to identify the "private"" way to call the auth server
*/
const MODE_PRIVATE = 'private';
private const MODE_PRIVATE = 'private';
Copy link

Choose a reason for hiding this comment

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

⚠️ This breaks the backward compatibility.

"symfony/stopwatch": "^6.4|^7.0",
"symfony/twig-bundle": "^6.4|^7.0",
"symfony/validator": "^6.4|^7.0",
"symfony/yaml": "^6.4|^7.0"
Copy link

Choose a reason for hiding this comment

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

Symfony 5.4 is still officially supported and for a while. I think using a BC Layer is not too expensive here. I have no idea what @konandrum thinks about this...

@@ -12,14 +12,14 @@ class ExceptionListener
/**
* @var UrlGeneratorInterface
*/
Copy link

Choose a reason for hiding this comment

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

This comment is now useless

@Nek- Nek- mentioned this pull request Apr 2, 2024
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.

3 participants