Problem/Motivation

On a project I'm actively working on, we've identified serialization / normalization as being one our main performance bottlenecks.
We're doing our best to move away from JSONAPI "includes".

The problem is, the fixOrderInclude() method will always include order items and purchased entities, as long as an "include" parameter is set.

The logic in this method doesn't make sense IMO, because we're skipping includes alltogether when the "include" parameter is omitted. I'd expect that specifying "include" means the API consumer knows what it is doing...

I'm suggesting we remove this all together as there's currently no way to bypass this behavior... Unless ALL includes are removed.

This will of course affect existing installations which means we should highlight this change in the release notes.

CommentFileSizeAuthor
#7 3268121-7.patch927 bytesjsacksick
#2 3268121-2.patch6.82 KBjsacksick

Comments

jsacksick created an issue. See original summary.

jsacksick’s picture

jsacksick commented
Status: Active » Needs review
StatusFileSize
new 3268121-2.patch6.82 KB
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Works for me, I wasn't sure how I felt about in the first place. It was added to appease the DX for new users.

jsacksick’s picture

jsacksick commented

I think this "feature" helps, and that might break existing installations which means some people would/could complain... But unless we add a setting or something, there's currently no way of skipping this logic... :(

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Use a Settings::get escape. This could allow turning it off. So mark it as deprecated before stable. Maybe add a `@trigger_error`, which may be noisy.

* So use Settings::get to see if someone has opted out, skip running logic
* Make a change record saying it'll go away before full release

Probably not trigger_error since it's noisy. And users are small enough that spamming in #commerce and release notes is OK

jsacksick’s picture

jsacksick commented

So maybe the getaway is to just add the setting, and call it a day.

We probably should document this in the README or in the documentation though...

Something like the following:

$fix_order_includes = Settings::get('commerce_api_fix_order_includes', TRUE);

So it can be opted out... Sounds like a safer option.

jsacksick’s picture

jsacksick commented
StatusFileSize
new 3268121-7.patch927 bytes

The attached patch is what I have in mind.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

+1, this is what I was thinking. It is about the best that can be done, right now.

jsacksick’s picture

jsacksick commented
Title: Remove the fixOrderInclude() helper » Allow disabling the automatically "fixing" of order includes
Status: Needs review » Fixed

  • jsacksick committed 2c9341c on 8.x-1.x 
    Issue #3268121 by jsacksick, mglaman: Allow disabling the automatically...

Status: Fixed » Closed (fixed)

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