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

[PAYSHIP-3149] PrestaShop 9 compatibility #1304

Merged
merged 52 commits into from
Feb 25, 2025
Merged

Conversation

L3RAZ
Copy link
Collaborator

@L3RAZ L3RAZ commented Dec 13, 2024

No description provided.

@L3RAZ L3RAZ force-pushed the feat/PAYSHIP-3149 branch from b887836 to 1a1d7f0 Compare December 13, 2024 15:15
@L3RAZ L3RAZ marked this pull request as draft December 30, 2024 14:01
@L3RAZ L3RAZ force-pushed the feat/PAYSHIP-3149 branch from 14ae80f to 67e5f16 Compare January 2, 2025 15:54
@L3RAZ L3RAZ marked this pull request as ready for review January 13, 2025 13:59
Copy link

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @L3RAZ for this huge work

I have a few questions, sorry if they don't make sens it's probably because I lack knowledge on this module's code 😅

$this->fundingSourceTranslationProvider = $fundingSourceTranslationProvider;
$this->configuration = $configuration;
}

public function __invoke(AddOrderPaymentCommand $command) {

Choose a reason for hiding this comment

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

By the way, if you use the AsCommandHandler you shouldn't need to add this __invoke method, but again I'm really aware of this module's architecture and context If the command/query buses are used in FO/legacy context this auto-configuration will probably not work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will check if this works on front controllers

Copy link
Contributor

Choose a reason for hiding this comment

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

You added this file in the gitignore, don't forget to remove it. 😉

ps_checkout.php Outdated
@@ -101,8 +101,8 @@ class Ps_checkout extends PaymentModule
* @var \PrestaShop\ModuleLibServiceContainer\DependencyInjection\ServiceContainer
*/
private $serviceContainer;
private static $merchantIsValid;
private static $currencyIsAllowed;
private ?bool $merchantIsValid;
Copy link
Contributor

Choose a reason for hiding this comment

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

That's pretty weird to define boolean property which can be nullable. In my opinion, we should set this property as false in the __construct() method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The whole point is to not check if merchant is valid multiple times. If it's null - we set the value. If it's bool - we know if it's valid or not

@@ -746,13 +746,13 @@ public function hookActionAdminControllerSetMedia()
*/
public function merchantIsValid()
{
if (static::$merchantIsValid === null) {
if ($this->merchantIsValid === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we set the value as false in the __construct() method, it should be :

Suggested change
if ($this->merchantIsValid === null) {
if (!$this->merchantIsValid) {

@L3RAZ L3RAZ requested a review from seiwan February 6, 2025 15:13
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

I approve commits

But as I'm not a checkout expert I prefer to wait for a 2nd review ;)

@L3RAZ L3RAZ merged commit f0fe087 into prestashop/9.x Feb 25, 2025
17 of 19 checks passed
@L3RAZ L3RAZ deleted the feat/PAYSHIP-3149 branch February 25, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants