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

paypal error after redirect to site. #258

Closed
papppeter opened this issue May 30, 2019 · 29 comments
Closed

paypal error after redirect to site. #258

papppeter opened this issue May 30, 2019 · 29 comments

Comments

@papppeter
Copy link

papppeter commented May 30, 2019

Twig token error after payment.

paypal was in sandbox mode. in paypal i could finalize the payment. after redirect back to cart i got this error.

payment was successfully registered.

Screenshot_21

@papppeter papppeter changed the title paypal error paypal error after redirect to site. May 30, 2019
@papppeter
Copy link
Author

I think the problem here is: DefaultMoney class. I am not sure about it but, if i am removing the render part than it starts working. so my thinking is: we are using inside mail generation the same twig instance which breaks template generation. (twig thinks the render is already started.)

i hope you can look into this. or at least you could use somehow an another twig instance. I see the boot that you create mall.twig.environment but i think this still crashes with the main twig render.

what do you think? what should i provide for recreating the problem?

@papppeter
Copy link
Author

one more info, if i remove all this type of methods {{ item.totalPostTaxes() | raw }} from mall.order.table than it starts working.

@tobias-kuendig
Copy link
Member

tobias-kuendig commented May 31, 2019 via email

@papppeter
Copy link
Author

Hi,

do you know the issue where octobercms team is working on this?

@papppeter
Copy link
Author

ok i did found it. octobercms/october#3341

but i am on the latest october version, or i hope at least. do you have any suggestion what should i check?

"name": "october/system",
"version": "v1.0.454",

@papppeter
Copy link
Author

Hi Tobias,

i found out a solution:

protected function render($contents, array $vars)
{
    return (new Twig)->parse($contents, $vars);
}

why are you using this differently? i saw in your code "paymentMethod.renderInstructions" i think that method is just working as expected.

what do you think? in this case you don't need twig booting process at all.

i did tested with paypal, and it was working for me, total table was displayed correctly.

@Eoler
Copy link
Contributor

Eoler commented Jun 10, 2019

Also got Call to a member function getCurrent() on null after successfully handling redirection from a custom paymentProvider, it indicates that it is in the middle of handling mailer.beforeAddContent event.

Using standard Twig in boot process makes Unknown "partial" tag in "__string_template exception reappear in other places. Maybe quickfix is not to use Mail queue?

@papppeter
Copy link
Author

hi,

@Eoler where does it reappear? for me it works as expected... but maybe i miss something

@Eoler
Copy link
Contributor

Eoler commented Jun 12, 2019

Your version of rendering DefaultMoney in Twig throws exception in new order checkout for offline payment providers.

@papppeter
Copy link
Author

papppeter commented Jun 13, 2019

i see...

can you suggest how i could create a test for this? it is hard to do it over and over.

another question, is it working for you at all? should i try a fresh install?

@papppeter
Copy link
Author

papppeter commented Jun 14, 2019

Hi,

i have another question, what happen is we are using Money class only one way. It should work as a filter in twig, and we should not give back formatted trough pricePostTaxes() would it maybe solve this issue? Or we should not use it as filter...

unfortuantely i don't see the difference between default money and paymentmethod twig usage.

@Eoler
Copy link
Contributor

Eoler commented Jun 14, 2019

Your solution works and solves exception on postback from remote payment provider, but it creates another one in offline payment provider (that probably IS a reason why Tobias implemented it differently).

My current workaround is setting up async queue (to be run from scheduler) and changing DefaultMoney method to:

    protected function render($contents, array $vars)
    {
        if (\App::runningInConsole()) {
            return (new \October\Rain\Parse\Twig)->parse($contents, $vars);
        }
        else {
            $template = $this->twig->createTemplate($contents);
            return $template->render($vars);
        }
    }

@papppeter
Copy link
Author

hi,

thanks for your code part! how do i set up async queue, do you have the document link for it? so than all the mails will go over queue?

@Eoler
Copy link
Contributor

Eoler commented Jun 14, 2019

It depends on your server: https://octobercms.com/docs/services/queues#running-the-queue-worker
probably the easiest way is to set it up as database and run it in your plugin's scheduler, rough example:

    public function registerSchedule($schedule)
    {
        $schedule->command('queue:work')->everyMinute()->withoutOverlapping();
    }

@tobias-kuendig
Copy link
Member

This is a nasty issue that I have already spent many hours on.

The solution provided by @papppeter was my initial solution that introduced the Unknown "partial" tag exception, hence I went on and registered a separate twig environment.

The problem should not appear if you are using a separate queue worker since twig is only loaded once. When the sync queue worker is used, Twig is loaded once by the CMS part of October and then the custom twig environment for Mall is not loaded completely because of how Twig caches things ‒ this leads to the getCurrent() on null error.

I'll have to further look into this... 🙄

@papppeter
Copy link
Author

could you build up some testing env? where it would be easier to recreate this problem? for me the hardest part was that i needed to go trough all the payment process to land on this page...

maybe the another sollution could be not using twig for money formatting. i don't think i would miss it if i could have same basic formatting. (currency prefix, postfix, how number function should called.)

@tobias-kuendig
Copy link
Member

could you build up some testing env? where it would be easier to recreate this problem? for me the hardest part was that i needed to go trough all the payment process to land on this page...

Yeah, that's what make this a time consuming problem for me as well. I tried to dig into the call stack and isolate the issue but the whole Twig/October boot cycle is a huge rabbit hole and not that easy to follow.

maybe the another sollution could be not using twig for money formatting. i don't think i would miss it if i could have same basic formatting. (currency prefix, postfix, how number function should called.)

I'd rather fix the issue upstream than re-inventing the wheel. Having the full feature set of Twig at hand provides many benefits.

@Eoler
Copy link
Contributor

Eoler commented Jun 17, 2019

Rabbit hole indeed, my workaround with async queued mails dropped me into one (templates are sent with EN translations): rainlab/translate-plugin#285

@papppeter
Copy link
Author

papppeter commented Jun 20, 2019

HI,

i just tried to long, now i removed that feature from my fork. Please write here if you could create a test area, or if you could fix the issue.

thanks!

my solution is:

protected function render($contents, array $vars)
{
        return number_format($vars['price'],$vars['currency']->decimals,  ',', ' ').' '.$vars['currency']->symbol;
}

@Dreanad
Copy link

Dreanad commented Nov 22, 2019

Hey there, same issue for me,
what do you recommand to do for ?

@tobias-kuendig
Copy link
Member

For the time being, you'd have to replace these lines and not rely on the twig environment:

https://github.com/OFFLINE-GmbH/oc-mall-plugin/blob/develop/classes/utils/DefaultMoney.php#L44

See papppeter's comment from above. If you don't need different formatting for different currencies this is very easy to implement.

@Dreanad
Copy link

Dreanad commented Nov 22, 2019

Thank you for your response,
how can i do this properly ? I dont want to modify the plugin directly

@tobias-kuendig
Copy link
Member

  1. Create your own plugin
  2. Create a class that extends Mall's DefaultMoney class
  3. Make changes to the render method
  4. In your own Plugin's boot method, register your own Money implementation like so:

https://github.com/OFFLINE-GmbH/oc-mall-plugin/blob/develop/classes/registration/BootServiceContainer.php#L39

@Zmove
Copy link

Zmove commented Jan 11, 2020

Can someone who has developed this custom module share it with the community?

@tobias-kuendig
Copy link
Member

I might be able to tackle this issue in the next few days. It's possible that we need to find an alternative to rendering the currency format using the Twig::parse method as it somehow runs in an "incomplete twig environment" when it is used in mail partials.

@tobias-kuendig
Copy link
Member

I did open a issue with a proof of this problem: https://github.com/octobercms/october/issues/4874

Let's see if someone from the community is able to help.

@marcomessa
Copy link
Contributor

Hello guys. I confirm the issue, and I confirm that the workaround explained up here is working flawlessly. Thank you for sharing, and thank you for your work!

@tobias-kuendig tobias-kuendig pinned this issue May 21, 2020
@zlobec
Copy link

zlobec commented Sep 12, 2020

Hey,
I get this error only when sending email to client, but not to the admin. The email to admin has correctly formatted |money format. Is this the same issue?

@tobias-kuendig
Copy link
Member

So we've got a great workaround from @jacobdekeizer over in https://github.com/octobercms/support/issues/35

See c5075fd, let me know if the problem persists!

@tobias-kuendig tobias-kuendig unpinned this issue Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants