Project

General

Profile

Actions

Anomalie #1538

closed

Manque d'initialisation sur $errors pour Contributions fait planter ->store() après appel du constructeur

Added by Mathieu Pellegrin about 3 years ago. Updated almost 3 years ago.

Status:
Fermé
Priority:
Normal
Assignee:
-
Category:
Core
Target version:
-
Start date:
02/07/2021
Due date:
% Done:

100%

Estimated time:
Version utilisée:

Description

Bonjour,

Je remonte ce qui est, je pense, une anomalie qui touche le plugin Paypal, mais qui je pense a sa source dans le Core.

Lorsque le plugin Paypal traite le /notify, il réalise les actions suivantes :

$contrib = new Contribution($this->zdb, $this->login, $args);
...
$store = $contrib->store();

Or, la fonction ->store() de Contributions commence par effectuer le test suivant :

if (count($this->errors) > 0) {

Et cela fait planter le programme (du moins sous PHP 7.2) :

stderr: Message: count(): Parameter must be an array or an object that implements Countable

Car à aucun moment ni dans l'initialisation de l'attribut ni dans le constructeur la valeur $this->errors est initialisée avec un tableau vide.

La propriété étant privée, il n'est pas possible de l'initialiser autrement qu'en passant par la méthode ->check(...) mais celle-ci s'attend à avoir un objet de formulaire, qu'il faut construire.

De mon côté je pense qu'il suffirait d'initialiser l'attribut $errors avec un tableau vide dans la déclaration de la classe pour régler le problème.

Actions #1

Updated by Johan Cwiklinski about 3 years ago

  • Status changed from Nouveau to In Progress

Il est possible de corriger le problème du côté du plugin aussi. Dans le coeur, sur les objets principaux (adhérents, dons/cotisations, ...) il y a une méthode check qui devrait normalement être exécutée dans tous les cas, même si ce n'est actuellement pas obligatoire dans les faits.

Je pense que le correctif suivant devrait corriger le problème dans le plugin (c'est du vite fait, je n'ai pas testé) :

diff --git a/lib/GalettePaypal/Controllers/PaypalController.php b/lib/GalettePaypal/Controllers/PaypalController.php
index 5b9662e..2be90a8 100644
--- a/lib/GalettePaypal/Controllers/PaypalController.php
+++ b/lib/GalettePaypal/Controllers/PaypalController.php
@@ -448,6 +448,18 @@ class PaypalController extends AbstractPluginController
                     }

                     if ($real_contrib) {
+                        $store = false;
+                        $valid = $contrib->check($post, [], []);
+                        if ($valid !== true) {
+                            Analog::log(
+                                'An error occurred while storing a new contribution from Paypal payment:' .
+                                implode("\n   ", $valid),
+                                Analog::ERROR
+                            );
+                            $ph->setState(PaypalHistory::STATE_ERROR);
+                            return $response->withStatus(500, 'Internal error');
+                        }
+
                         $store = $contrib->store();
                         if ($store === true) {
                             //contribution has been stored :)

Actions #2

Updated by Mathieu Pellegrin about 3 years ago

Merci pour ta réponse!

J'ai dût être un peu plus précis pour que ça passe, $post est issu du $request et ne contenait pas les bonnes clés :

index 5b9662e..a7ced54 100644
--- a/lib/GalettePaypal/Controllers/PaypalController.php
+++ b/lib/GalettePaypal/Controllers/PaypalController.php
@@ -39,6 +39,7 @@ namespace GalettePaypal\Controllers;
 use Analog\Analog;
 use Galette\Controllers\AbstractPluginController;
 use Galette\Entity\Contribution;
+use Galette\Entity\ContributionTypes;
 use Galette\Entity\PaymentType;
 use Galette\Filters\HistoryList;
 use GalettePaypal\Paypal;
@@ -448,6 +449,24 @@ class PaypalController extends AbstractPluginController
                     }

                     if ($real_contrib) {
+
+                        $fields = [
+                            ContributionsTypes::PK  => $post['item_number'],
+                            Adherent::PK            => $post['custom'],
+                            'type_paiement_cotis'   => PaymentType::PAYPAL,
+                            'montant_cotis'         => $post['mc_gross'],
+                        ];
+                        $valid = $contrib->check($fields, [], []);
+                        if ($valid !== true) {
+                            Analog::log(
+                                'An error occurred while storing a new contribution from Paypal payment:' .
+                                implode("\n   ", $valid),
+                                Analog::ERROR
+                            );
+                            $ph->setState(PaypalHistory::STATE_ERROR);
+                            return $response->withStatus(500, 'Internal error');
+                        }
+
                         $store = $contrib->store();
                         if ($store === true) {
                             //contribution has been stored :)

Du coup je ne suis pas sûr que le tableau $args serve à quelque chose, car check() initialise pas mal des paramètres de la contribution, mais je l'ai laissé par sécurité.

À noter que je n'ai pas essayé non plus, j'ai backporté les modifications que j'ai faites pour mon plugin Stripe à partir de ta réponse initiale.

Actions #3

Updated by Johan Cwiklinski about 3 years ago

Mathieu Pellegrin a écrit (#note-2):

Du coup je ne suis pas sûr que le tableau $args serve à quelque chose, car check() initialise pas mal des paramètres de la contribution, mais je l'ai laissé par sécurité.

Oui bien vu, ça me semble tout à fait pertinent ;)

À noter que je n'ai pas essayé non plus, j'ai backporté les modifications que j'ai faites pour mon plugin Stripe à partir de ta réponse initiale.

^^ pas de problème ;)

Du coup, j'ai regardé d'un peu plus près, si on utilise check, il n'est plus besoin de vérifier manuellement les chevauchements ; du coup, le patch devient :

diff --git a/lib/GalettePaypal/Controllers/PaypalController.php b/lib/GalettePaypal/Controllers/PaypalController.php
index 5b9662e..892921f 100644
--- a/lib/GalettePaypal/Controllers/PaypalController.php
+++ b/lib/GalettePaypal/Controllers/PaypalController.php
@@ -425,29 +425,27 @@ class PaypalController extends AbstractPluginController
                         $args['ext'] = $this->preferences->pref_membership_ext;
                     }
                     $contrib = new Contribution($this->zdb, $this->login, $args);
-                    $contrib->amount = $post['mc_gross'];
+                    $post = [
+                        ContributionsTypes::PK  => $post['item_number'],
+                        Adherent::PK            => $post['custom'],
+                        'type_paiement_cotis'   => PaymentType::PAYPAL,
+                        'montant_cotis'         => $post['mc_gross']
+                    ];

                     //all goes well, we can proceed
-                    if ($real_contrib && $contrib->isCotis()) {
-                        // Check that membership fees does not overlap
-                        $overlap = $contrib->checkOverlap();
-                        if ($overlap !== true) {
-                            if ($overlap === false) {
-                                Analog::log(
-                                    'An error occurred checking overlapping fees :(',
-                                    Analog::ERROR
-                                );
-                            } else {
-                                //method directly return error message
-                                Analog::log(
-                                    'Error while calculating overlapping fees from paypal payment: ' . $overlap,
-                                    Analog::ERROR
-                                );
-                            }
+                    if ($real_contrib) {
+                        $store = false;
+                        $valid = $contrib->check($post, [], []);
+                        if ($valid !== true) {
+                            Analog::log(
+                                'An error occurred while storing a new contribution from Paypal payment:' .
+                                implode("\n   ", $valid),
+                                Analog::ERROR
+                            );
+                            $ph->setState(PaypalHistory::STATE_ERROR);
+                            return $response->withStatus(500, 'Internal error');
                         }
-                    }

-                    if ($real_contrib) {
                         $store = $contrib->store();
                         if ($store === true) {
                             //contribution has been stored :)

J'ai ouvert une PR pour ce correctif : https://github.com/galette/plugin-paypal/pull/3

Actions #4

Updated by Johan Cwiklinski about 3 years ago

  • Status changed from In Progress to Résolu
  • % Done changed from 0 to 100
Actions #5

Updated by Johan Cwiklinski almost 3 years ago

  • Status changed from Résolu to Fermé

Corrigé dans le plugin Paypal

Actions

Also available in: Atom PDF