Closed (fixed)
Project:
Commerce PayPal
Version:
8.x-1.x-dev
Component:
Payflow Gateway
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
24 Jul 2020 at 10:59 UTC
Updated:
7 Oct 2020 at 14:59 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
khiminrm commentedComment #4
jsacksick commented@khiminrm: Could you post the final patch to the issue, so we can go ahead and commit this?
Comment #5
khiminrm commented@jsacksick the patch is there
Comment #6
jsacksick commentedComment #7
jsacksick commented@khiminrm: Could you run phpcs locally and fix the following violations?
Also, noticed
createSecureToken()has 2 return statements (and both look wrong):* @return bool|mixed
* The Secure Token if successfully retrieved or FALSE on failure.
*
* @return mixed|null
I think we return a string, or null, correct?
Also
PayflowLink::createSecureTokenis missing the typehint(OrderInterface $order)instead of just $order.Also the switch in
PayflowLinkForm::buildConfigurationForm()is missing the "break" statements (Phpstorm noticed that).Also, please prefer strict type check (e.g
if ($mode == 'test') {toif ($mode === 'test').These are small things I've noticed myself after applying the patch and reading the code quickly, but I'm sure there's more.
The "password" field in
PayflowLink::buildConfigurationFormhas 2 #descriptions attributes.Also... I'm wondering if the code in
itemizeOrder()is actually correct? Especially the following:First of all /** @var \Drupal\aggregator\ItemInterface $item */ is wrong, but...
$iis never incremented which technically means you're only appending the first order item to the$nvparray. Are you sure all the items are passed correctly to PayPal??Comment #8
jsacksick commentedAlso, you shouldn't have to call the rounder to round prices (in itemizeOrder()), and perhaps in other places.
Instead you should be sending prices the following way:
.
Comment #9
jsacksick commentedPurchasableEntityInterface doesn't define a
getSku()method (you should check if the purchased entity is an instead of ProductVariationInterface before passing the SKU.Also, what you'e doing in the adjustments loop really looks similar to what the
AdjustmentTransformeris doing, you should probably reuse that instead of duplicating the logic?Also:
canReferencePaymenthas some useless checks that won't ever return TRUE?Payment transactions don't define a "failure" state.
Again, the 'success' state doesn't actually exist (See commerce_payment.workflows.yml).
There are no 'success' and 'pending' states. You should either define a new payment type and a new payment workflow if you really need those, or use existing states.
Comment #10
khiminrm commented@jsacksick, I've pushed most of fixes.
I saw that rounder is used in
PayflowandExpressCheckoutgateways. That's why I decided to use similar approach. I've replaced with Calculator::trim in my code.Regarding adjustments. I've re-used similar code as in
ExpressCheckoutgateway. Should I still replace code by usingAdjustmentTransformeris?I've fixed 'success' state. But not 'pending' yet. Still researching...
Thanks for your help!
Comment #11
khiminrm commentedComment #12
khiminrm commentedComment #13
khiminrm commentedComment #14
khiminrm commentedComment #15
jsacksick commented$this->settings, this->datetimeTime, and this->dateFormatterare not used anywhere.The constructor should say Constructs a new PayflowLink object.
Comment #16
khiminrm commentedComment #18
jsacksick commentedMade minor changes prior to commit and Committed! Thanks Roman!