Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

allows overriding the controller names for which the stripe headers a… #3

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

jagui
Copy link
Contributor

@jagui jagui commented Jan 13, 2020

allows overriding the controller names for which the stripe headers are sent.

This eases integrating stripe with 3rd party one page checkout modules.

@clotairer clotairer self-requested a review May 3, 2020 08:14
@clotairer
Copy link
Collaborator

Thank you for your PR and suggestion.

To be in the same "spirit" of the module, I'd rather a declaration like "public $controllers" or hooks "public $hooks".
But, we are not comfortable with overrides and perhaps exec an hook should be better.

$orderPageNames = ['order', 'order-opc'];
Hook::exec('actionStripeDefineOrderPageNames', &$orderPageNames);

What do you think about it ?

@jagui
Copy link
Contributor Author

jagui commented May 4, 2020

@clotaire202 yeah, I like the idea, please check the new commit. Please note that I'm using (min) php7.0, therefore call time pass by reference is not allowed. Hence I'm merging the result of the hook with the base array.

I'll squash the changes once we reach an agreement.
Thanks, Juan

@clotairer clotairer mentioned this pull request May 18, 2020
13 tasks
@jagui
Copy link
Contributor Author

jagui commented May 18, 2020

@clotaire202 After thinking this through I've realized we can the array by reference by means of an intermediate array. I've updated the PR accordingly.

Copy link
Collaborator

@clotairer clotairer left a comment

Choose a reason for hiding this comment

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

thank you

@@ -1058,7 +1058,12 @@ public function hookDisplayAdminOrderContentOrder($params)
*/
public function hookHeader()
{
if (!in_array($this->context->controller->php_self, ['order', 'order-opc'])) {
$orderPages = ['order', 'order-opc'];
$extraOrderPages = Hook::exec('actionStripeDefineOrderPageNames');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see two theoretical problems:

  • if you have two modules hook the last will override previous values
  • if someone want to unset an orderPage previously added, the hook cannot manage this case
        $orderPages = [
            'order',
            'order-opc',
        ];
        Hook::exec('actionStripeDefineOrderPageNames', [ 'orderPages' => &$orderPages ]);

And to call this hook :

    public function hookActionStripeDefineOrderPageNames($params)
    {
        $extraOrderPages = [
            'my-order',
        ];
        $params['orderPages'] = array_merge($params['orderPages'], $extraOrderPages);
    }

@clotairer clotairer added the enhancement New feature or request label May 18, 2020
…re sent.

This eases integrating stripe with 3rd party one page checkout modules.
@jagui
Copy link
Contributor Author

jagui commented May 18, 2020

I've squashed the changes a single commit.

@clotairer clotairer merged commit 177449e into 202-ecommerce:develop Jun 2, 2020
@clotairer clotairer added this to the 2.1.1 milestone Jun 2, 2020
@clotairer
Copy link
Collaborator

Thank you for your contribution. I've just merged your PR in develop branch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants