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

Allow Symfony 6 and resolve depreciations #299

Merged
merged 2 commits into from
Dec 19, 2021

Conversation

GromNaN
Copy link
Contributor

@GromNaN GromNaN commented Dec 17, 2021

First, thank you for maintaining this library. I work on a project that uses it and that needs to be upgraded to Symfony 6.0.

I had to make a lot of updates in order to have tests passing. Please tell me if you prefers separated pull requests, or anything that I should change.

  • Remove hirak/prestissimo, not necessary since Composer 2 supports parallel downloads.
  • Switch to symfony/simple-phpunit to get polyfills for PHPUnit versions.
  • Require PHP 7.1+ (void return type)
  • Allow Symfony 6.0 and add return types.
  • Replace Mockery with PHPUnit Mock in Cocur\Slugify\Tests\Bridge\Symfony\CocurSlugifyExtensionTest, to solve an error ParseError: syntax error, unexpected token "static", expecting identifier

@GromNaN GromNaN force-pushed the feature/symfony6 branch 7 times, most recently from 4bda1a1 to f1ac25b Compare December 17, 2021 21:20
Copy link
Member

@florianeckerstorfer florianeckerstorfer left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR, looks good. There is just one misplaced parenthesis that makes one test fail.

tests/Bridge/Symfony/CocurSlugifyExtensionTest.php Outdated Show resolved Hide resolved
Co-authored-by: Florian Eckerstorfer <florian@eckerstorfer.net>
@GromNaN
Copy link
Contributor Author

GromNaN commented Dec 18, 2021

Good catch. There is issue with scrutinizer. Not sure if it is correctly configured.

@florianeckerstorfer florianeckerstorfer merged commit 4330c46 into cocur:master Dec 19, 2021
@GromNaN GromNaN deleted the feature/symfony6 branch December 19, 2021 16:47
@GromNaN GromNaN mentioned this pull request Dec 19, 2021
@@ -19,7 +19,7 @@
}
],
"require": {
"php": ">=7.0",
"php": ">=7.1",

Choose a reason for hiding this comment

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

Should probably be ^7.1 || ~8.0.0 || ~8.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really? Isn't PHP 8.2 supposed to be compatible with PHP 8.1?

Choose a reason for hiding this comment

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

PHP doesn't (can't) follow SemVer, but in general, the current range selector also allows for PHP 9, which is a problem.

^7.1 || ^8.0 is also acceptable, if you feel like you aren't worried about breakages in 8.2+

Choose a reason for hiding this comment

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

I started a PR and would appreciate your input on this: #301

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