-
Notifications
You must be signed in to change notification settings - Fork 142
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
Version 3 #545
base: master
Are you sure you want to change the base?
Version 3 #545
Conversation
Updated Docs, Changelog, add PhpStan and fix some PhpStan Warnings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Chris53897 Thanks for this! I have done a quick review and have some comments.
I'm also wondering if the minimum PHP can be bumped to 8.1.
Since Symfony bumped the minimum supported PHP version to 8.1 in version 6.1, I'm happy to have 8.1 as the minimum for version 3.0 and drop Symfony < 6.1.
People that want to use PHP 8.0 or Symfony 5.4/6.0, can still use version 2.5 of this bundle, which I'll keep maintaining as an LTS for some time
Maybe a new branch from master for version 3?
I'll create a new branch for 2.5.x, which means master can then be used for all development towards 3.0
Tests/DependencyInjection/Factory/AbstractStorageFactoryTest.php
Outdated
Show resolved
Hide resolved
@@ -3,8 +3,6 @@ | |||
namespace Payum\Bundle\PayumBundle\Tests\DependencyInjection\Factory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but we dropped support for Propel entirely, so all these classes can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do it in this PR or in seperate later. Just let me know
} else { | ||
class AppKernel extends AppKernelShared | ||
|
||
public function handle(Request $request, int $type = HttpKernelInterface::MAIN_REQUEST, bool $catch = true): Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method needed if it just calls the parent method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is the $catch
parameter.
If i remove the function 7 tests will fail.
[error] Uncaught PHP Exception Symfony\Component\HttpKernel\Exception\NotFoundHttpException: "A token with hash
payum_token could not be found." at /Users/georg/PhpStorm/open-source/_backup/PayumBundle/vendor/payum/core/Payum/Core/Bridge/Symfony/Security/HttpRequestVerifier.php line 49
The tests have a reference to @ticket 507 I guess #507
I am not sure if this tests are useful anymore, and could mabye removed.
This needs a deeper look.
Co-authored-by: Pierre du Plessis <[email protected]>
Co-authored-by: Pierre du Plessis <[email protected]>
Co-authored-by: Pierre du Plessis <[email protected]>
…e deprecated functions
@pierredup Thanks for having a look and the good guidance. Of course i loved to upgrade to 8.1 and symfony 6. With "payum/core": "^1.7.3", as lowest version we can get rid of "psr/log": "^1 || ^2", I will try to have a look with sonata integration in an different PR. |
feat: drop support of symfony 4, drop support of php 7.4, remove deprecated functions
First step towards version 3.
Yes i know the plan is to update payum too, and integrate the changes here.
Maybe a new branch from master for version 3?