Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

khiminrm created an issue. See original summary.

khiminrm’s picture

Assigned: Unassigned » khiminrm
Issue summary: View changes

jsacksick’s picture

@khiminrm: Could you post the final patch to the issue, so we can go ahead and commit this?

khiminrm’s picture

StatusFileSize
new52.81 KB

@jsacksick the patch is there

jsacksick’s picture

Status: Active » Needs review
jsacksick’s picture

Status: Needs review » Needs work

@khiminrm: Could you run phpcs locally and fix the following violations?

FILE: /Users/jonathan/projects/d8/www/web/modules/contrib/commerce_paypal/src/PluginForm/PayflowLinkForm.php
------------------------------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
------------------------------------------------------------------------------------------------------------
 57 | ERROR | [x] Line indented incorrectly; expected 10 spaces, found 11
 58 | ERROR | [x] Line indented incorrectly; expected 10 spaces, found 11
 59 | ERROR | [x] Line indented incorrectly; expected 12 spaces, found 13
 60 | ERROR | [x] Line indented incorrectly; expected 10 spaces, found 11
 61 | ERROR | [x] Line indented incorrectly; expected 10 spaces, found 11
 62 | ERROR | [x] Line indented incorrectly; expected 10 spaces, found 11
------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------


FILE: /Users/jonathan/projects/d8/www/web/modules/contrib/commerce_paypal/src/PluginForm/PaymentReferenceForm.php
-----------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------
 12 | ERROR | [x] Expected 1 space before implements keyword; 2 found
-----------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------


FILE: /Users/jonathan/projects/d8/www/web/modules/contrib/commerce_paypal/src/Plugin/Commerce/PaymentGateway/PayflowLink.php
--------------------------------------------------------------------------------------------------------------------------------
FOUND 29 ERRORS AND 1 WARNING AFFECTING 28 LINES
--------------------------------------------------------------------------------------------------------------------------------
  161 | ERROR   | [x] Expected 1 space between comma and type hint "MessengerInterface"; 0 found
  161 | ERROR   | [x] Expected one space after the comma, 0 found
  489 | ERROR   | [x] Expected 1 space after "=>"; 2 found
  503 | WARNING | [ ] Code after the THROW statement on line 502 cannot be executed
  503 | ERROR   | [x] Line indented incorrectly; expected 6 spaces, found 8
  744 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
  783 | ERROR   | [x] Inline comments must end in full-stops, exclamation marks, question marks, colons, or closing parentheses
  816 | ERROR   | [x] Expected 1 space after "=>"; 2 found
  832 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  850 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  885 | ERROR   | [ ] Missing parameter type
  894 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  896 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  899 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  902 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  904 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  907 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  914 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  920 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  926 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  962 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  965 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  967 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  969 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  974 | ERROR   | [x] Case breaking statements must be followed by a single blank line
  998 | ERROR   | [x] Expected 1 space after "=>"; 2 found
 1003 | ERROR   | [x] Array indentation error, expected 6 spaces but found 8
 1004 | ERROR   | [x] Array indentation error, expected 6 spaces but found 8
 1005 | ERROR   | [x] Array closing indentation error, expected 4 spaces but found 6
 1099 | ERROR   | [ ] Description for the @return value is missing
--------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 27 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------------------------------------


FILE: ...Users/jonathan/projects/d8/www/web/modules/contrib/commerce_paypal/src/Plugin/Commerce/PaymentGateway/PayflowLinkInterface.php
------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------------
 26 | ERROR | [ ] Only 1 @return tag is allowed in a function comment
 27 | ERROR | [x] Separate the @return and @throws sections by a blank line.
------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------

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::createSecureToken is 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') { to if ($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::buildConfigurationForm has 2 #descriptions attributes.

Also... I'm wondering if the code in itemizeOrder()is actually correct? Especially the following:

    $i = 0;
    /** @var \Drupal\aggregator\ItemInterface $item */
    foreach ($order->getItems() as $item) {
      $item_amount = $this->rounder->round($item->getUnitPrice());

      // Add the line item to the return array.
      $nvp += [
        'L_NAME' . $i => $item->getTitle(),
        'L_COST' . $i => $item_amount->getNumber(),
        'L_QTY' . $i => $item->getQuantity(),
      ];

      // Add the SKU.
      /** @var \Drupal\commerce\PurchasableEntityInterface $purchased_entity */
      $purchased_entity = $item->getPurchasedEntity();
      if ($purchased_entity) {
        $nvp += [
          'L_SKU' . $i => $purchased_entity->getSku(),
        ];
      }

      $items_total = $items_total->add($item->getTotalPrice());
    }

First of all /** @var \Drupal\aggregator\ItemInterface $item */ is wrong, but... $i is never incremented which technically means you're only appending the first order item to the $nvp array. Are you sure all the items are passed correctly to PayPal??

jsacksick’s picture

Also, 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:

Calculator::trim($order_item->getUnitPrice()->getNumber())

.

jsacksick’s picture

      /** @var \Drupal\commerce\PurchasableEntityInterface $purchased_entity */
      $purchased_entity = $item->getPurchasedEntity();
      if ($purchased_entity) {
        $nvp += [
          'L_SKU' . $i => $purchased_entity->getSku(),
        ];
      }

PurchasableEntityInterface 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:

canReferencePayment has some useless checks that won't ever return TRUE?

    // Return FALSE if the transaction was a failure.
    if ($payment->getState()->getId() == 'failure') {
      return FALSE;
    }

Payment transactions don't define a "failure" state.

      $new_payment = $this->entityTypeManager->getStorage('commerce_payment')
        ->create([
          'state' => 'success',
          'amount' => $amount,
          'payment_gateway' => $payment->getPaymentGatewayId(),
          'order_id' => $order->id(),
          'remote_id' => $response['PNREF'] ?? '',
          'remote_state' => 'S',
        ]);
      $new_payment->save();

Again, the 'success' state doesn't actually exist (See commerce_payment.workflows.yml).

    $state = '';

    // Set the transaction status based on the type of transaction this was.
    if (intval($received_parameters['RESULT']) == 0) {
      switch ($trxtype) {
        case 'S':
          $state = 'success';
          break;

        case 'A':
        default:
          $state = 'pending';
          break;
      }
    }
    elseif (intval($received_parameters['RESULT']) == 126) {
      $state = 'pending';
    }

    $commerce_payment = $payment_storage->create([
      'state' => $state,
      'amount' => $order->getBalance(),
      'payment_gateway' => $this->parentEntity->id(),
      'order_id' => $order->id(),
      'remote_id' => $received_parameters['PNREF'],
      'remote_state' => $trxtype,
    ]);

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.

khiminrm’s picture

@jsacksick, I've pushed most of fixes.
I saw that rounder is used in Payflow and ExpressCheckout gateways. 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 ExpressCheckout gateway. Should I still replace code by using AdjustmentTransformeris?

I've fixed 'success' state. But not 'pending' yet. Still researching...

Thanks for your help!

khiminrm’s picture

StatusFileSize
new0 bytes
khiminrm’s picture

khiminrm’s picture

StatusFileSize
new52.35 KB
khiminrm’s picture

StatusFileSize
new52.34 KB
jsacksick’s picture

  1. +++ b/src/Plugin/Commerce/PaymentGateway/PayflowLink.php
    @@ -0,0 +1,1187 @@
    +    $this->settings = $settings;
     +   $this->datetimeTime = $datetime_time;
    

    $this->settings, this->datetimeTime, and this->dateFormatter are not used anywhere.

The constructor should say Constructs a new PayflowLink object.

khiminrm’s picture

StatusFileSize
new51.35 KB

  • jsacksick committed 7db072d on 8.x-1.x authored by khiminrm
    Issue #3161220 by khiminrm, jsacksick: Port Payflow Link to D8.
    
jsacksick’s picture

Status: Needs work » Fixed
StatusFileSize
new51.11 KB

Made minor changes prior to commit and Committed! Thanks Roman!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.