postnl / postnl-magento1-End-of-life

This is the official Magento 1 extension of PostNL.
12 stars 9 forks source link

v1.15.7 Buspakje error after upgrade, DeliveryDate not set #34

Closed seansan closed 5 years ago

seansan commented 5 years ago

Possible error? occurred shortly after upgrade other buspakjes work well only not 1

VIA GRID

[17-Dec-2018 16:49:06] WARNING: [pool shirts] child 6692 said into stderr: "#4 app/code/community/TIG/PostNL/Block/Adminhtml/Template.php(55): Mage_Core_Block_Template->_toHtml()" [17-Dec-2018 16:49:06] WARNING: [pool shirts] child 6692 said into stderr: "#5 /var/..." [17-Dec-2018 16:51:15] WARNING: [pool shirts] child 27192 said into stderr: "NOTICE: PHP message: PHP Fatal error: Uncaught Error: Call to a member function format() on boolean in app/code/community/TIG/PostNL/Model/Core/Shipment.php:4945" [17-Dec-2018 16:51:15] WARNING: [pool shirts] child 27192 said into stderr: "Stack trace:" [17-Dec-2018 16:51:15] WARNING: [pool shirts] child 27192 said into stderr: "#0 app/code/community/TIG/PostNL/Model/Core/Shipment.php(5065): TIG_PostNL_Model_Core_Shipment->_getIsBuspakje()" [17-Dec-2018 16:51:15] WARNING: [pool shirts] child 27192 said into stderr: "#1 app/code/community/TIG/PostNL/Model/Core/Shipment.php(5016): TIG_PostNL_Model_Core_Shipment->_extractProductCodeForType(Array)" [17-Dec-2018 16:51:15] WARNING: [pool shirts] child 27192 said into stderr: "#2 app/code/community/TIG/PostNL/Model/Core/Shipment.php(730): TIG_PostNL_Model_Core_Shipment->_getProductCode()" [17-Dec-2018 16:51:15] WARNING: [pool shirts] child 27192 said into stderr: "#3 app/code/community/TIG/PostNL/Model/Core/Shipment.php(2099): TIG_PostNL_Model_Core_Shipment->getProductCode()" [17-Dec-2018 16:51:15] WARNING: [pool shirts] child 27192 said into stderr: "#4 app/code/community/TIG/PostNL/Model/Core/Shipment.php(5576): TIG_PostNL_Model_Core_Shipment->hasCustomBarcode()" [17-Dec-2018 16:51:15] WARNING: [pool shirts] child 27192 said into stderr: "#5 app/code/core/Mage/Core/Mo..."

SIMILAR when manual shipping

[17-Dec-2018 16:55:29] WARNING: [pool shirts] child 26582 said into stderr: "#5 app/code/community/TIG/PostNL/Model/C..." [17-Dec-2018 16:55:31] WARNING: [pool shirts] child 27191 said into stderr: "NOTICE: PHP message: PHP Fatal error: Uncaught Error: Call to a member function setTimezone() on boolean in app/code/community/TIG/PostNL/Block/Adminhtml/Sales/Order/Shipment/Create/ShipmentOptions.php:301" [17-Dec-2018 16:55:31] WARNING: [pool shirts] child 27191 said into stderr: "Stack trace:" [17-Dec-2018 16:55:31] WARNING: [pool shirts] child 27191 said into stderr: "#0 app/design/adminhtml/default/default/template/TIG/PostNL/sales/order/shipment/create/shipment_options.phtml(72): TIG_PostNL_Block_Adminhtml_Sales_Order_Shipment_Create_ShipmentOptions->getFitsAsBuspakje()" [17-Dec-2018 16:55:31] WARNING: [pool shirts] child 27191 said into stderr: "#1 app/code/core/Mage/Core/Block/Template.php(241): include('/var/www/html/s...')" [17-Dec-2018 16:55:31] WARNING: [pool shirts] child 27191 said into stderr: "#2 app/code/core/Mage/Core/Block/Template.php(273): Mage_Core_Block_Template->fetchView('adminhtml/defau...')" [17-Dec-2018 16:55:31] WARNING: [pool shirts] child 27191 said into stderr: "#3 app/code/core/Mage/Core/Block/Template.php(287): Mage_Core_Block_Template->renderView()" [17-Dec-2018 16:55:31] WARNING: [pool shirts] child 27191 said into stderr: "#4 app/code/community/TIG/PostNL/Block/Adminhtml/Template.php(55): Mage_Core_Block_Template->_toHtml()" [17-Dec-2018 16:55:31] WARNING: [pool shirts] child 27191 said into stderr: "#5 /var/..."

seansan commented 5 years ago

Seems there is a logic error in the code both examples access a delivery date when there is no date: the customer selected "As soon as possible"

    /**
     * Buspakje shipments can't be delivered on a monday or tuesday.
     */
    $deliveryDate = DateTime::createFromFormat('Y-m-d H:i:s', $this->getDeliveryDate(), new DateTimeZone('UTC'));
    if ($deliveryDate->format('N') === '0' || $deliveryDate->format('N') === '1') {
        return false;
    }

somthing to do with this setting?

image

tig-dennisvanderhammen commented 5 years ago

Hi Sean,

Thank you for your submission.

First of all, I would like to ask you to make use of the issue template that is provided when you create an issue.

Regarding to your issue, I've created a backlog item for your issue. After shortly looking into the issue, I created the following fix. Please note that this is currently in internal review, and didn't go through QA yet, but I'd already like to share this fix with you. I advice you to first test it on a staging environment.

Replace app/code/community/TIG/PostNL/Model/Core/Shipment.php line 4945 with the following two lines: $date = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400); $deliveryDate = DateTime::createFromFormat('Y-m-d H:i:s', $date, new DateTimeZone('UTC'));

In the same file, replace line 4964 (4965 after the previous change) with the following line: $deliveryDate = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400);

Once this fix gets approved and is tested internally, we'll include this fix in the next release.

seansan commented 5 years ago

Will try

But do know this code piece can be found in multiple places throughout the extension

Fix it everywhere?

On Tue, Dec 18, 2018 at 1:03 PM Dennis van der Hammen < notifications@github.com> wrote:

Hi Sean,

Thank you for your submission.

First of all, I would like to ask you to make use of the issue template that is provided when you create an issue.

Regarding to your issue, I've created a backlog item for your issue. After shortly looking into the issue, I created the following fix. Please note that this is currently in internal review, and didn't go through QA yet, but I'd already like to share this fix with you. I advice you to first test it on a staging environment.

Replace app/code/community/TIG/PostNL/Model/Core/Shipment.php line 4945 with the following two lines: $date = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400); $deliveryDate = DateTime::createFromFormat('Y-m-d H:i:s', $date, new DateTimeZone('UTC'));

In the same file, replace line 4964 (4965 after the previous change) with the following line: $deliveryDate = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400);

Once this fix gets approved and is tested internally, we'll include this fix in the next release.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tig-nl/postnl-magento1/issues/34#issuecomment-448197941, or mute the thread https://github.com/notifications/unsubscribe-auth/AAn0a5ejwzLsflixu-JZ3MTYH7KDmG3uks5u6NmcgaJpZM4ZWoUf .

seansan commented 5 years ago

Uncaught Error: Call to a member function format() on string in /var/www/html/shirts/public_html/app/code/community/TIG/PostNL/Model/Core/Shipment.php:4969

On Tue, Dec 18, 2018 at 1:03 PM Dennis van der Hammen < notifications@github.com> wrote:

Hi Sean,

Thank you for your submission.

First of all, I would like to ask you to make use of the issue template that is provided when you create an issue.

Regarding to your issue, I've created a backlog item for your issue. After shortly looking into the issue, I created the following fix. Please note that this is currently in internal review, and didn't go through QA yet, but I'd already like to share this fix with you. I advice you to first test it on a staging environment.

Replace app/code/community/TIG/PostNL/Model/Core/Shipment.php line 4945 with the following two lines: $date = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400); $deliveryDate = DateTime::createFromFormat('Y-m-d H:i:s', $date, new DateTimeZone('UTC'));

In the same file, replace line 4964 (4965 after the previous change) with the following line: $deliveryDate = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400);

Once this fix gets approved and is tested internally, we'll include this fix in the next release.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tig-nl/postnl-magento1/issues/34#issuecomment-448197941, or mute the thread https://github.com/notifications/unsubscribe-auth/AAn0a5ejwzLsflixu-JZ3MTYH7KDmG3uks5u6NmcgaJpZM4ZWoUf .

seansan commented 5 years ago

Also would it be smarter to solve it in the getter?

public function getDeliveryDate()

On Tue, Dec 18, 2018 at 1:03 PM Dennis van der Hammen < notifications@github.com> wrote:

Hi Sean,

Thank you for your submission.

First of all, I would like to ask you to make use of the issue template that is provided when you create an issue.

Regarding to your issue, I've created a backlog item for your issue. After shortly looking into the issue, I created the following fix. Please note that this is currently in internal review, and didn't go through QA yet, but I'd already like to share this fix with you. I advice you to first test it on a staging environment.

Replace app/code/community/TIG/PostNL/Model/Core/Shipment.php line 4945 with the following two lines: $date = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400); $deliveryDate = DateTime::createFromFormat('Y-m-d H:i:s', $date, new DateTimeZone('UTC'));

In the same file, replace line 4964 (4965 after the previous change) with the following line: $deliveryDate = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400);

Once this fix gets approved and is tested internally, we'll include this fix in the next release.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tig-nl/postnl-magento1/issues/34#issuecomment-448197941, or mute the thread https://github.com/notifications/unsubscribe-auth/AAn0a5ejwzLsflixu-JZ3MTYH7KDmG3uks5u6NmcgaJpZM4ZWoUf .

tig-dennisvanderhammen commented 5 years ago

Hi Sean,

After your comment I decided to do a double check on where this might go wrong. Most cases were already caught on 1.15.7 but it seems like a few cases slipped past us.

Adding this to the getter is something that was considered, but this will provide unintended behaviour in if-statements where we might expect an empty value.

I found the following locations where this can be applied, this is including the ones I mentioned above. As mentioned above, this is still in internal review. Please test these changes before applying this to a live shop.

app/code/community/TIG/PostNL/Block/Adminhtml/Sales/Order/Shipment/Create/ShipmentOptions.php line 295: $date = $postnlOrder->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400); line 298: $date,

app/code/community/TIG/PostNL/Model/Core/Observer/Barcode.php line 114: $date = $postnlOrder->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400); $deliveryDate = new DateTime($date, new DateTimeZone('UTC'));

app/code/community/TIG/PostNL/Model/Core/Observer/Cron.php line 1422 & 1494: $confirmDate = $postnlShipment->getConfirmDate() ?: date('Y-m-d H:i:s', time() + 86400); line 1423 & 1495: $deliveryDate = $postnlShipment->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400);

app/code/community/TIG/PostNL/Model/Core/Shipment.php line 4945: $date = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400); $deliveryDate = DateTime::createFromFormat('Y-m-d H:i:s', $date, new DateTimeZone('UTC')); line 4965: $deliveryDate = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400);

seansan commented 5 years ago

Can you please create a branch for this?

Also: did you see my comment about object versus string?

On Tue, Dec 18, 2018 at 2:03 PM Dennis van der Hammen < notifications@github.com> wrote:

Hi Sean,

After your comment I decided to do a double check on where this might go wrong. Most cases were already caught on 1.15.7 but it seems like a few cases slipped past us.

Adding this to the getter is something that was considered, but this will provide unintended behaviour in if-statements where we might expect an empty value.

I found the following locations where this can be applied, this is including the ones I mentioned above. As mentioned above, this is still in internal review. Please test these changes before applying this to a live shop.

app/code/community/TIG/PostNL/Block/Adminhtml/Sales/Order/Shipment/Create/ShipmentOptions.php line 295: $date = $postnlOrder->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400); line 298: $date,

app/code/community/TIG/PostNL/Model/Core/Observer/Barcode.php line 114: $date = $postnlOrder->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400); $deliveryDate = new DateTime($date, new DateTimeZone('UTC'));

app/code/community/TIG/PostNL/Model/Core/Observer/Cron.php line 1422 & 1494: $confirmDate = $postnlShipment->getConfirmDate() ?: date('Y-m-d H:i:s', time() + 86400); line 1423 & 1495: $deliveryDate = $postnlShipment->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400);

app/code/community/TIG/PostNL/Model/Core/Shipment.php line 4945: $date = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400); $deliveryDate = DateTime::createFromFormat('Y-m-d H:i:s', $date, new DateTimeZone('UTC')); line 4965: $deliveryDate = $this->getDeliveryDate() ?: date('Y-m-d H:i:s', time() + 86400);

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tig-nl/postnl-magento1/issues/34#issuecomment-448213772, or mute the thread https://github.com/notifications/unsubscribe-auth/AAn0a9QKyGKJvSxuDFj5swXzGJcUGmcyks5u6Oe-gaJpZM4ZWoUf .

seansan commented 5 years ago

You can use an optional param to the function

/**
 * Gets the delivery date for this shipment.
 *
 * @return string
 */
public function getDeliveryDate($set = false)
{
    if ($this->hasDeliveryDate()) {
        return $this->_getData('delivery_date');
    }

    /**
     * Try to get the delivery date for a PostNL order.
     */
    $postnlOrder = $this->getPostnlOrder();
    if ($postnlOrder && $postnlOrder->hasDeliveryDate()) {
        $deliveryDate = $postnlOrder->getDeliveryDate();

        $this->setDeliveryDate($deliveryDate);
        return $deliveryDate;
    }

    /**
     * Else set first available shipping day being a Tuesday (Postnl does not deliver Sunday or Monday)
     */
    if ($set) {
        // DateTime without timezone is UTC
        $date = new DateTime('Tuesday');
        $deliveryDate = date_format($date,"Y-m-d H:i:s");
        $this->setDeliveryDate($deliveryDate);
        return $deliveryDate;
    }

    return null;
}
tig-dennisvanderhammen commented 5 years ago

Hi Sean,

I missed your comment about your error. I assumed this was related to the comment previous to that. I was not able to reproduce this after applying the changes that I mentioned.

I pushed the branch master_buspakje_fix, you should be able to download this one from the current repo.