Does not report changes in parent constructor

[norgeindian]

We have a preference on class X in which we extend the overridden class and call the constructor of the parent. During the latest update, the constructor, which we call, has changed. One more parameter has been added. This issue has not been found by the helper, but leads to a critical error. It would be awesome to find a way to catch these kind of problems as well.

[convenient]

One more parameter has been added. This issue has not been found by the helper, but leads to a critical error.

Internally we rely a lot on using this tool alongside phpcs and phpstan, just curious if it was picked up by any other means?

I am surprised the change to the constructor on the parent class didnt flag this as a change to preference in general, as any changes to a thing you have a preference on, should be flagged 🤔

Could you give a general example with a few more details?

[norgeindian]

@convenient , I actually ran into it before it came into our automated testing process. But I'm with you, this would have surely been caught by some other tool. Nevertheless, I would say it would be great if it could be included in the helper as well.

The change came from https://github.com/mollie/magento2. We updated from version 2.27.0 to 2.29.0. Our preference is defined in the following way:

<preference for="Mollie\Payment\Model\Client\Orders" type="CustomGento\Mollie\Model\Client\Orders"/>

Our constructor was the following:

    public function __construct(
        OrderLines $orderLines,
        InvoiceSender $invoiceSender,
        OrderRepository $orderRepository,
        CheckoutSession $checkoutSession,
        ManagerInterface $messageManager,
        Registry $registry,
        MollieHelper $mollieHelper,
        ProcessAdjustmentFee $adjustmentFee,
        OrderCommentHistory $orderCommentHistory,
        PartialInvoice $partialInvoice,
        StoreCredit $storeCredit,
        RefundUsingPayment $refundUsingPayment,
        Expires $expires,
        State $orderState,
        Transaction $transaction,
        BuildTransaction $buildTransaction,
        PaymentTokenForOrder $paymentTokenForOrder,
        ProcessTransaction $processTransaction,
        \Mollie\Payment\Service\Mollie\MollieApiClient $mollieApiClient,
        Config $config,
        EventManager $eventManager,
        LinkTransactionToOrder $linkTransactionToOrder,
        OrderLockService $orderLockService
    ) {
        parent::__construct(
            $orderLines,
            $invoiceSender,
            $orderRepository,
            $checkoutSession,
            $messageManager,
            $registry,
            $mollieHelper,
            $adjustmentFee,
            $orderCommentHistory,
            $partialInvoice,
            $storeCredit,
            $refundUsingPayment,
            $expires,
            $orderState,
            $transaction,
            $buildTransaction,
            $paymentTokenForOrder,
            $processTransaction,
            $mollieApiClient,
            $config,
            $eventManager,
            $linkTransactionToOrder,
            $orderLockService
        );

This was fine for 2.27.0, but must include $shouldEmailInvoice in the new version, as the constructor of the original class has been changed to this:

    public function __construct(
        OrderLines $orderLines,
        InvoiceSender $invoiceSender,
        OrderRepository $orderRepository,
        CheckoutSession $checkoutSession,
        ManagerInterface $messageManager,
        Registry $registry,
        MollieHelper $mollieHelper,
        ProcessAdjustmentFee $adjustmentFee,
        OrderCommentHistory $orderCommentHistory,
        PartialInvoice $partialInvoice,
        StoreCredit $storeCredit,
        RefundUsingPayment $refundUsingPayment,
        Expires $expires,
        State $orderState,
        Transaction $transaction,
        BuildTransaction $buildTransaction,
        PaymentTokenForOrder $paymentTokenForOrder,
        ProcessTransaction $processTransaction,
        \Mollie\Payment\Service\Mollie\MollieApiClient $mollieApiClient,
        Config $config,
        EventManager $eventManager,
        LinkTransactionToOrder $linkTransactionToOrder,
        OrderLockService $orderLockService,
        ShouldEmailInvoice $shouldEmailInvoice
    ) {
        $this->orderLines = $orderLines;
        $this->invoiceSender = $invoiceSender;
        $this->orderRepository = $orderRepository;
        $this->checkoutSession = $checkoutSession;
        $this->messageManager = $messageManager;
        $this->registry = $registry;
        $this->mollieHelper = $mollieHelper;
        $this->adjustmentFee = $adjustmentFee;
        $this->storeCredit = $storeCredit;
        $this->refundUsingPayment = $refundUsingPayment;
        $this->orderCommentHistory = $orderCommentHistory;
        $this->partialInvoice = $partialInvoice;
        $this->expires = $expires;
        $this->orderState = $orderState;
        $this->transaction = $transaction;
        $this->buildTransaction = $buildTransaction;
        $this->eventManager = $eventManager;
        $this->paymentTokenForOrder = $paymentTokenForOrder;
        $this->processTransaction = $processTransaction;
        $this->mollieApiClient = $mollieApiClient;
        $this->config = $config;
        $this->linkTransactionToOrder = $linkTransactionToOrder;
        $this->orderLockService = $orderLockService;
        $this->shouldEmailInvoice = $shouldEmailInvoice;
    }

Hope, I could make it obvious, what I mean.

[convenient]

Interesting i believe this should have flagged definitely an issue to review

[convenient]

Hey @norgeindian

So I've updated the phpunit test for the preference and it works just fine for me https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/pull/107

The preferences logic doesn't do any deep analysis of what has changed, only that its a PHP file and it is has a preference attached. I think that even a whitespace will be enough to create a diff, which will be picked up by the tool which will scan the filepath and pass it through the following code

https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/master/src/Ampersand/PatchHelper/Checks/ClassPreferencePhp.php#L50-L86

I would have expected you to have some output like

+-------+------------+---------------------------------------------------------------------------------------+---------------------------------------------------+
| Level | Type       | File                                                                                  | To Check                                          |
+-------+------------+---------------------------------------------------------------------------------------+---------------------------------------------------+
| WARN  | Preference | vendor/mollie/module-payment-or-whatever/Model/Client/Orders.php                      | CustomGento\Mollie\Model\Client\Orders            |
+-------+------------+---------------------------------------------------------------------------------------+---------------------------------------------------+

If you have time I'd recommend following the steps on https://github.com/AmpersandHQ/ampersand-magento2-upgrade-patch-helper/blob/master/docs/TROUBLESHOOTING.md#debug-a-specific-file to debug this specific file and maybe run through xdebug to see why it was not being reported?

[tr33m4n]

Hi @norgeindian, is this still an issue? Just reviewing open issues and seeing where we're at 😄