Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A hook to replace box packing class? - Box Packer is timing out #123

Open
joshuapease opened this issue Mar 16, 2024 · 3 comments
Open

A hook to replace box packing class? - Box Packer is timing out #123

joshuapease opened this issue Mar 16, 2024 · 3 comments

Comments

@joshuapease
Copy link
Contributor

joshuapease commented Mar 16, 2024

What are you trying to do?

Hi there! Thanks for making Postie awesome.

We've got a client who sells super small, lightweight products. It's not unheard of for an order to contain quantities of 50,000 - 100,000 (usually just a handful of line items, but each having really high quantities)

We've found that when line item quantities get large, the box packing algorithm ends up timing out.

They don't use dimensions for their products (just weight), so the box packing seems a little fruitless in this case.

Are there any hooks we could use to bypass the default box packing & instead provide our own box packing logic (in most cases, this would just be a single box with the total weight of the products).

What's your proposed solution?

I could see it being useful to have some hooks or config options for customizing how items are added to the box packer.

  • For each line item, modify how it's added to the packer
    • In our use case, we'd probably merge together items that are super small, or have no dimensions.
  • Or... it might also be useful to modify and merge together items right before calling $packer->pack()
    • Although... it doesn't seem like the Packer class exposes its items.
  • Provide a way to inject a custom packer class - right around here

It's a bit yucky opening up these internal features, but we've got a very unique Craft Commerce client (with 662,650 products & counting).

Feels like perhaps the cleanest path would be to let folks override the InfalliblePacker class with their own version of the Packer class. I'd love to hear your thoughts on these solutions.

I'm more than happy to make a PR for this if you feel it's worth pursuing.

Additional context

No response

@joshuapease joshuapease changed the title Could use a hook to replace box packing class? - Box Packer is timing out A hook to replace box packing class? - Box Packer is timing out Mar 16, 2024
@engram-design
Copy link
Member

Interesting, I've not tested things on that scale to fine-tune performance (I'll do that), but you're right that it might be easier to override box packing in some fashion. There are before/after packing events but they're used for modification of things, rather than replacing the packing mechanism.

You could certainly use EVENT_BEFORE_PACK_ORDER to replace the box packer with your own class. That would be my preference, although it's a little more work for developers to handle (creating some packer classes), as opposed to just event callbacks. But I think that's going to be the more rebust approach.

I think being able to modify how things are added to the packer is also an option.

Let me know if using the EVENT_BEFORE_PACK_ORDER is an option?

@joshuapease
Copy link
Contributor Author

Thanks for the quick reply! I think replacing the box packer will work great in my use case.

EVENT_BEFORE_PACK_ORDER almost does the trick, but I had to make a small modification after the event is handled.

if ($this->hasEventHandlers(self::EVENT_BEFORE_PACK_ORDER)) {
    $this->trigger(self::EVENT_BEFORE_PACK_ORDER, $packOrderEvent);

    // Allow event hander to override $packer
    $packer = $packOrderEvent->packer;
}

I've created a PR with that additional line.

@engram-design
Copy link
Member

Oh, great point, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants