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

Feat: support symfony6 #541

Merged
merged 26 commits into from
Jul 4, 2022
Merged

Conversation

Chris53897
Copy link
Contributor

Hi,

this PR will close #539

I applied some recommendations from PhpStorm-IDE.
Only the untitests are executed. i did not make a real test with an existing project.

Info: I marked some tests as incomplete. These are related to form.factory that needs a session under Symfony 6.
These tests need to be rewritten, but i do not know how. I guess it is related to the config-option csrf_protection: true.
But other tests need this config to be true.
Best way (i read about) is to rewrite the tests in a request-context. But i do not know how to do this.

Greetz

@Chris53897
Copy link
Contributor Author

I updated composer.json for outdated repos and not needed dependencies for tests.
The required php-ext are required by the packages themself.
Now we have no outdated repos composer outdated. Only psr/log 2.0.0 3.0.0 Common interface for logging libraries
but only as long as payum has a new release.

@ancient-spirit
Copy link

Thank you for your effort, I hope Payum will fully support symfony6

@Chris53897
Copy link
Contributor Author

Chris53897 commented Mar 31, 2022

@ancient-spirit I updated this comment.
The Symfony-Bridge in Payum/Core needs some work to support Symfony 6.
https://github.com/Payum/Core/tree/master/Bridge/Symfony

@pierredup Is this PR going in the right direction? Any suggestions to move this forward?

@Chris53897
Copy link
Contributor Author

Chris53897 commented Apr 4, 2022

I changed composer.json to replace payum/payum in favor of require single gateways (see Payum/Payum#912)

@pierredup
Copy link
Member

@Chris53897 Thanks for all the effort on this. I'm going to keep this open for a bit to see where Payum/Payum#928 goes. If we release a new major version of Payum/Payum, then the next major version of Payum/PayumBundle should support the Payum version (so there might be more changes required in this repo before we can tag a new major version)

@Fl0ux
Copy link

Fl0ux commented May 3, 2022

I would like to try your pull request, and maybe give some help if needed, but I cannot find a way to install it.
I tried this :

"require": {
    "payum/payum-bundle": "feature/support-symfony6"
}
"repositories": [
    {
        "type": "git",
        "url": "git@github.com:Chris53897/PayumBundle.git"
    }
]

But I get this error :
Could not parse version constraint feature/support-symfony6: Invalid version string "feature/support-symfony6"
Any idea how to fix it ?

@pierredup
Copy link
Member

@Fl0ux You can add the dev- prefix to the version constraint, which is what Composer uses to install branches

"require": {
    "payum/payum-bundle": "dev-feature/support-symfony6"
}

@Chris53897
Copy link
Contributor Author

Chris53897 commented May 3, 2022

@pierredup was faster with responding ;)
You will run maybe into 2 problems. One with psr/log version. (if you use it in a real project)
Second will be getMasterRequest() vs getMainRequest() in Symfony-Bridge from payum/core.
I will try to send PRs for this these week.

@Chris53897
Copy link
Contributor Author

PR to close the problem getMasterRequest() vs getMainRequest() Payum/Payum#934

@Chris53897
Copy link
Contributor Author

@pierredup It will take a while until Payum/Payum#928 will result in 2.x.

I would prefer to get symfony 6 up and running with this bundle and Payum/Core.
And after Payum/Core 2.x is finished we could create a new (major) version of this bundle according to changes in Payum/Core 2.x.

WDYT?

@Chris53897
Copy link
Contributor Author

@Fl0ux I made a change to composer.json to allow an install with dev-1.x branch. This will be reverted after 1.7.1 is released.

I did a quicktest and it looks like it works. But our application needs adjustments, so i can not verify at the moment.
Can you please help with testing?

@Chris53897
Copy link
Contributor Author

I did a test with onetime payment via saferpay-gateway and creditcard. It worked. Next step is to test a twotime payment.

@Fl0ux Did you found some time to make some tests?

@Chris53897
Copy link
Contributor Author

I did now test a recurrent payment (twotime payment) with saferpay-gateway and it worked.
This is the only live gateway i can test.
I hope some users can help with live-tests.

It would be great to have a 1.7.1 release of payum/payum, so this PR does not have to use 1.x-dev.

@Chris53897
Copy link
Contributor Author

@pierredup Sorry to nag you. Is there any update on this topic?

@Zales0123
Copy link

Hello @pierredup 👋 Do you have any perspective on when this change could be merged and released? :) We would definitely need it in Sylius (issue already linked) in the nearest future. Thanks in advance! 🚀 🖖

@pierredup
Copy link
Member

Sorry that it took this long, I contemplated the way forward with the various versions (for PHP and Symfony), and decided to not do a major release at the moment, but release a new 2.x version, which includes support for Symfony 6, as well as keeping support for Symfony 4 and 5.

The reasons for this, is because Symfony 4.4 is still in active maintenance, so I don't want to drop support for it at the moment, and also don't want to maintain 2 versions of the bundle (2.x for SF 4 and 5, and 3.0 for SF 6). Also, the changes required to support Symfony 4 - 6 is not a lot, so it should not be a big burden to maintain it all in a single version in the interim (until the next major version bump, which will definitely drop PHP < 8). Lastly, I think if we drop support for PHP 7 and support PHP 8 (or 8.1) only, then there are a lot more features that we can add and make more use of the new language feature. Which is why I want to work on Payum V2 first, which will support PHP 8.1+ only, but re-architect some of the internals as well, and provide a bit more features. After that, then PayumBundle 3.0 can come with support for Payum V2 only.

@Chris53897 Thanks for all the effort on this PR! I'm going to merge this now, but will hold off on a release for about a week, so that I can first test this with various projects to ensure nothing is broken. If there are no issues, then a 2.5 release with this changes can be expected any time next week.

@pierredup pierredup merged commit 4322a97 into Payum:master Jul 4, 2022
@Chris53897
Copy link
Contributor Author

@pierredup
Thanks for the explanation.
My employers main goal is to use it with symfony 6 in production.
After that is done, i will try to help with Payum V2. And PayumBundle V3.

Please do not forget to bump this version after payum has a new tag.
Otherwise the bug with psr/log is still there. (and can only be used if the project has psr/log version 1). So it will not work with Symfony 6 in my eyes.
https://github.com/Payum/PayumBundle/blob/master/composer.json#L41

Commit that fixes that. But i guess this commit is lost? Sorry i lost track.
Chris53897/Payum@46ed47d

https://github.com/Payum/Payum/blob/1.x/composer.json

@pierredup
Copy link
Member

@Chris53897 That commit has been picked to the 1.7.x branch (https://github.com/Payum/Payum/tree/1.7.x). I'm sure it should have been released with 1.7.2, but since I missed it. I'll release 1.7.3 then which will then include that change.

After that is done, i will try to help with Payum V2. And PayumBundle V3.

It would be awesome to get some assistance on this. I've started working on some improvements for V2 and will start creating a few draft PRs over the next couple of weeks

@Chris53897
Copy link
Contributor Author

Great. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Support for Symonfy 6
6 participants