Contrib.social

Stop unsetting the order ID reference from the order refresh

Open on Drupal.org →
Created on 25 January 2024, 11 months ago

Describe your bug or feature request.

The following line of code has always been bothering me and I'm not entirely sure why it's there:

          // Remove the order that was set above, to avoid
          // crashes during the entity save process.
          $order_item->order_id->entity = NULL;

This was added a long time ago (See #2873394: Make taxes functional → ). While doing some profiling with Blackfire, I found this line being responsible for a double order item save.
On add to cart via JSONAPI, the order_id field is set on the order item, and it's then unset by the OrderRefresh causing an unnecessary extra save.

Let's see what happens with the tests after removing this line.

📌 Task
Status

Active

Version

2.0

Component

Order

Created by

🇮🇱Israel jsacksick

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @jsacksick
  • Status changed to Needs review 11 months ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update 11 months ago
    CI error
  • Comment 11 months ago →
    🇮🇱Israel jsacksick
  • Comment 5 months ago →
    🇳🇿New Zealand sjmcl

    That line of code has cost me several hours trying to fix a bug that looked like it was caused by the Currency Resolver module. Removing that line fixed it.

    Thanks for posting that patch.

  • Comment 5 months ago →
    🇮🇱Israel jsacksick

    I've never committed this due to the tests not completing with this change for whatever reason...

  • Merge request !302Issue #3416998 by jsacksick: Stop unsetting the order ID reference from the order refresh. → (Open) created by jsacksick
  • Comment 4 months ago →
    🇮🇱Israel jsacksick

    Let's see if the tests are passing with Gitlab CI.

  • Comment 4 months ago →
    🇮🇱Israel jsacksick

    A lot of tests are failing with this change lol... The patch has been applied to one of the projects I'm working on for months without side effects (at least not that I'm aware of).

  • Pipeline finished with Failed
    4 months ago
    Total: 1785s
    #253790
  • Pipeline finished with Success
    4 months ago
    Total: 415s
    #280744
  • Pipeline finished with Success
    4 months ago
    Total: 509s
    #280775
  • Comment about 1 month ago →
    🇨🇭Switzerland mazze

    Thank you so much for the patch. I already started thinking about calling the customer, telling him that we have a major problem, now everything is working properly:-)

  • Comment about 1 month ago →
    🇮🇱Israel jsacksick

    I wanted to commit the fix, but the tests are not completing without it... So this line is indeed fixing something... But breaking others...

  • Comment 16 days ago →
    🇵🇰Pakistan masood ahmad

    Thank you @jsacksick, your patch worked for me.

  • Pipeline finished with Failed
    15 days ago
    Total: 51s
    #365361
  • Pipeline finished with Failed
    15 days ago
    Total: 52s
    #365369
  • Pipeline finished with Failed
    15 days ago
    Total: 53s
    #365372
  • Pipeline finished with Failed
    15 days ago
    Total: 52s
    #365374
  • Pipeline finished with Failed
    15 days ago
    Total: 113s
    #365378
  • Pipeline finished with Failed
    15 days ago
    Total: 65s
    #365386
  • Pipeline finished with Success
    10 days ago
    Total: 319s
    #369785
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024