Allow overriding order items before being packed [#2858724]

Had a conversation with bojanz in Slack.

The crux of my issue is this paragraph:

The problem I have is that if I create a custom packer to do this [remove items from being packed], then no other packer can ever run on the site, because my packer would have to be the first one that runs. There doesn't seem to be any flexibility there. I'm working with another developer on commerce_fedex, with a custom packer there to handle things such as special packing for dry ice and hazmat shipments. That is a good use case for a custom packer it seems, but I would't be able to use it on my site because I want to remove quote items from being packed and that requires me to take over the entire packing process.

His idea for a potential solution to that:

I guess we could refactor $order->getItems() into a helper method on the DefaultPacker that runs an event afterwards

I just about have a patch ready for testing and further discussion on this issue, as well.

Comment	File	Size	Author
#10	2858724-10.patch	7.15 KB	bamberjp
#10
#9	commerce_shipping-before_packer_event-2858724-9.patch	3.75 KB	bmcclure
#9
#8	commerce_shipping-before_packer_event-2858724-8.patch	6.86 KB	amklose
#8
#3	commerce_shipping-before_packer_event-2858724-3.patch	6.81 KB	bmcclure
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

8 March 2017 at 10:08

bmcclure created an issue. See original summary.

Comment #2

bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commented 8 March 2017 at 10:29

Issue summary:

View changes

Comment #3

bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commented 8 March 2017 at 10:33

Status:

Active

» Needs review

File	Size
commerce_shipping-before_packer_event-2858724-3.patch	6.81 KB

Patch for review, creates a CommerceShippingEvents::BEFORE_PACK event.

Comment #4

googletorp CreditAttribution: googletorp at Reveal IT commented 10 April 2017 at 19:38

Status:

Needs review

» Needs work

We need test coverage for this.

Comment #5

bojanz CreditAttribution: bojanz at Centarro commented 10 April 2017 at 19:39

I approve of the approach, but wanted to bikeshed the event name.

Comment #6

googletorp CreditAttribution: googletorp at Reveal IT commented 10 April 2017 at 19:40

I feel like we can do better as well, BEFORE_PACK seems a bit weird.

Comment #7

bmcclure CreditAttribution: bmcclure as a volunteer and at Top Floor commented 11 April 2017 at 08:07

My thinking was that a packer's purpose is to pack, so a natural event system to modify that behavior seems like it would be BEFORE_PACK and AFTER_PACK. However I can definitely see how, outside of the context of a packer, the name doesn't make much sense, and since the name's used globally, maybe something more descriptive would be helpful.