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 5 #266

Merged
merged 21 commits into from Dec 11, 2019
Merged

Add support for Symfony 5 #266

merged 21 commits into from Dec 11, 2019

Conversation

Big-Shark
Copy link
Contributor

No description provided.

Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Since it's coming in the next 30 days, this is probably a good time to do it.

Please add an entry in the changelog!

@Big-Shark
Copy link
Contributor Author

@Jean85 Done

@Big-Shark Big-Shark requested a review from Jean85 October 28, 2019 21:05
@Big-Shark
Copy link
Contributor Author

@Jean85 Your move )

@Jean85
Copy link
Collaborator

Jean85 commented Nov 22, 2019

Sorry I'm at SymfonyCon so I didn't had the time to check it 😄 But I saw the release live 🎉

I'm going to re-start the CI to see if everything is ok....

This was referenced Nov 22, 2019
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

PR seems fine, but unfortunately it seems that we're blocked upstream: getsentry/sentry-php#925 (review)

@rvanlaak
Copy link
Contributor

We had the following error after installing it on a Symfony 5 project, so an additional forward-compatibility fix seems needed:

image

@Jean85
Copy link
Collaborator

Jean85 commented Nov 25, 2019

We're missing an integration test, that's why we missed this 😢

I'll need to setup a test like that before going forward.

@Jean85
Copy link
Collaborator

Jean85 commented Dec 3, 2019

Symfony 5 is still not getting installed 😢 There's something amiss with the CI config. I'll try to force it somehow.

@Jean85
Copy link
Collaborator

Jean85 commented Dec 5, 2019

Conflict solved, build is failing :(

@DavidBadura
Copy link

Hey, thank you for trying to push this PR forward! <3 I'm trying to help:

  1. You have requested a non-existent parameter "kernel.root_dir". Did you mean one of these: "kernel.project_dir", "kernel.cache_dir", "kernel.logs_dir"?

You need to change kernel.root_dir to kernel.project_dir. kernel.root_dir is deprecated since SF4.2 and removed in SF5.0. kernel.project_dir exists since SF3.3.

  1. Could not reflect class Symfony\Component\Console\Event\ConsoleCommandEvent as it is marked final.

You do not have to mock value objects. You can just instantiate them. These include:

  • Request
  • Token
  • *Event like ConsoleCommandEvent

Of course, you can mock the service away.

If you want, I can try to finish this PR on the weekend.

@Big-Shark
Copy link
Contributor Author

@DavidBadura yes please, do it.

@Jean85
Copy link
Collaborator

Jean85 commented Dec 6, 2019

Thanks @DavidBadura for the suggestions, I've implemented it. I've also removed some pieces of code that were needed for Symfony < 3.3.

@Jean85
Copy link
Collaborator

Jean85 commented Dec 6, 2019

I should've fixed all the tests. Now onto fixing PHPStan.

@Vincz
Copy link

Vincz commented Dec 9, 2019

@Jean85 Hey! The remaining errors seems related to the PHP Unit version. The current version is 8.5 while the bundle uses the 7.5

@Jean85
Copy link
Collaborator

Jean85 commented Dec 9, 2019

In reality I did a mistake with the class aliases. I fear that something else is still missing, let's see the CI after this fix...

@cleentfaar
Copy link

@Jean85 need help?

@Jean85
Copy link
Collaborator

Jean85 commented Dec 9, 2019

I'm currently getting deprecation errors under PHP 7.4, probably from PHPUnit. I'll try to use the PHPUnit bridge to allow PHPUnit 8

@Jean85
Copy link
Collaborator

Jean85 commented Dec 9, 2019

Ok two other issues:

@garak
Copy link
Contributor

garak commented Dec 9, 2019

@Jean85 I see that phpunit-bridge has "normal" versions, why do you want to ignore it in sed replace?

@garak
Copy link
Contributor

garak commented Dec 9, 2019

@Jean85 deprecation could be probably solved by allowing phpunit 8

Copy link

@jderusse jderusse left a comment

Choose a reason for hiding this comment

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

to fix phpunit deprecation, I use rector rector process --set phpunit8.0 .

"symfony/browser-kit": "^3.4||^4.0",
"symfony/expression-language": "^3.4||^4.0",
"symfony/framework-bundle": "^3.4||^4.0",
"phpunit/phpunit": "^7.5||^8.5",
Copy link

Choose a reason for hiding this comment

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

remove this, and let symfony/phpunit-bridge pick the right version ./vendor/bin/simple-phpunit

private $currentScope;
private $currentHub;
private $options;

protected function setUp()
protected function doSetUp(): void
Copy link

Choose a reason for hiding this comment

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

not needed, because the projet require php ^7.1n you can saftly add the typehint here setUp(): void

@Jean85
Copy link
Collaborator

Jean85 commented Dec 9, 2019

Because the trait is missing. But as per #266 (comment), I may not need it. BTW the PHPUnit bridge has no requirements, so you do not need to downgrade it.

@Jean85
Copy link
Collaborator

Jean85 commented Dec 9, 2019

PHP 7.4 deprecations will be fixed by getsentry/sentry-php#930 upstream.

Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

It finally works!

Remaining deprecations will be fixed when getsentry/sentry-php#930 will be tagged.

@Jean85 Jean85 merged commit 7532496 into getsentry:master Dec 11, 2019
@rvanlaak
Copy link
Contributor

rvanlaak commented Dec 11, 2019

@Jean85 great job everyone! 👏 Planning on releasing soon? If not I'll verify by requiring master locally.

@Jean85
Copy link
Collaborator

Jean85 commented Dec 11, 2019

Normally @HazAT is in charge of releasing. Feel free to test it out!

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.

None yet

9 participants