[ECP-9489] Implement PHPStan and fix relevant issues #2843
Conversation
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
candemiralp
had a problem deploying
to
internal
GitHub Actions
Failure
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
candemiralp
had a problem deploying
to
internal
GitHub Actions
Failure
candemiralp
had a problem deploying
to
internal
GitHub Actions
Failure
|
|
|
||
| if (isset($paymentFormFields) && isset($paymentFormFields | ||
| [AdyenPayByLinkDataAssignObserver::PBL_EXPIRY_DATE])) { | ||
| if (isset($paymentFormFields[AdyenPayByLinkDataAssignObserver::PBL_EXPIRY_DATE])) { |
There was a problem hiding this comment.
Does this work as expected if $paymentFormFields is not defined?
There was a problem hiding this comment.
Yes, the first isset() is redundant. The result will be same even if $paymentFormFields is not defined. isset() first checks the existence of the variable and then checks (multi-dimensional) array keys.
| /** @var AdyenCreditmemoModel $currAdyenCreditmemo */ | ||
| $currAdyenCreditmemo = $adyenCreditmemoLoader->load( | ||
| $adyenCreditmemo[CreditmemoInterface::ENTITY_ID], | ||
| CreditmemoInterface::ENTITY_ID | ||
| ); | ||
|
|
||
| if ($currAdyenCreditmemo->getCreditmemoId() !== null) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Can't see why is this being removed?
There was a problem hiding this comment.
Hey @acampos1916! Thanks for the heads-up!
The beginning of the loop is redundant now. $currAdyenCreditmemo was used to store AdyenCreditMemo object generated by the factory $adyenCreditmemoLoader with the entityId. It became redundant as adyenCreditmemoRepository already returns an AdyenCreditMemo object. We don't need to use object factory to build the entity anymore.
However, the following code snippet shouldn't have been removed. This must be used to identify and skip the already linked Adyen creditmemos. I will re-introduce this snippet.
if ($currAdyenCreditmemo->getCreditmemoId() !== null) {
continue;
}
| @@ -146,7 +140,7 @@ public function createInvoice(Order $order, Notification $notification, bool $is | |||
| throw $e; | |||
| } | |||
|
|
|||
| $invoiceAutoMail = (bool)$this->scopeConfig->isSetFlag( | |||
There was a problem hiding this comment.
Is it safe to remove (bool) here? Will isSetFlag always return a boolean?
There was a problem hiding this comment.
isSetFlag() method returns bool. It looks like type casting is redundant here.
|
|
||
| public function getOrderPaymentStatus(string $orderId, string $cartId): string | ||
| { | ||
| $quoteIdMask = $this->quoteIdMaskFactory->create()->load($cartId, 'masked_id'); | ||
| $quoteId = $quoteIdMask->getQuoteId(); | ||
| $quoteId = $this->maskedQuoteIdToQuoteId->execute($cartId); |
There was a problem hiding this comment.
Will this always be an INT? So that intval below works as expected
There was a problem hiding this comment.
execute() method always return int if there is an associated cart. If the cart can't be found, it throws NoSuchEntityException.
I believe the type int is safe. But, your question raised another point in my mind. As NoSuchEntityException is not caught here, the application will break unexpectedly. Formerly, quoteIdMaskFactory used to return null which throws __("The entity that was requested doesn't exist. Verify the entity and try again.") safely in the if condition.
Wrapping the execute() method with a try-catch block and catching NoSuchEntityException exception would be nice in favour of throwing the same exception as before.
throw new NotFoundException(
__("The entity that was requested doesn't exist. Verify the entity and try again.")
);
What would you think about this conversion?
| /* | ||
| * Load resource model Adyen\Payment\Model\ResourceModel\Notification manually | ||
| * as Magento\Framework\Model\AbstractModel::_getResource() method has been deprecated. | ||
| * ObjectManager has been used due to dependencies not being loaded timely manner on entity classes. |
There was a problem hiding this comment.
Is it strictly necessary? Let's review this one
There was a problem hiding this comment.
Hello @acampos1916, thank you for the review!
It created a circular dependency if DI patter is used. I would be happy if we can revisit that approach together.
Description
This PR implements automated PHPStan analysis as a part of the workflows. As Magento 2 has its own limitations, we have decided to use bitexpert/phpstan-magento extension to provide platform specific requirements.
Besides implementing PHPStan to the workflows, major issues having
Level 0severity have been fixed. Mainly,Tested scenarios