[META] Commerce Shipping 8.x

Please don't hijack the issue queue. If you want to develop your own version of commerce shipping for Drupal 8, that's fine, but don't use this issue queue for organizing the work. I'm not convinced that packaging/boxing, labeling should be part of commerce shipping.

I would rather go "small core" and figure out how to make commerce shipping extensible and other other modules depend on commerce shipping to handle these features. In that regard commerce_shipping is probably a bit misleading name, as the focus is shipping quotes.

smccabe has been working on the new shipping architecture with rszama and mgalaman. It is a direct continuation of the 7.x-2.x effort.

For the last 2 years there has been a strong agreement between us that shipping needs to handle multiple shipments. There have been multiple futile efforts in 7.x to handle this. Looking at other systems, they all have APIs for packaging and quoting.

googletorp, your feedback and participation is welcome and appreciated, but please don't shut down Shawn's efforts.

Let's schedule a call to get on the same page. As far as I can see the 8.x-2.x code here has been inactive, so there's not much of a conflict effort-wise.

Yeah, for what it's worth, I've reviewed the documentation and proposals and support the direction as well.

What we've had in Commerce 1.x has essentially been "treat the entire order as one anonymous shipment and come up with a price for it." This has forced every Shipping module to either create their own logic or depend on another module - but there hasn't even been a clear consensus which module should be responsible for producing shipments.

Therefore, the main change in the 2.x proposal is to actually create shipments up front and calculate shipping against that shipment. It still satisfies the 1.x use case of a single anonymous shipment per order, but it also supports every carrier rating service or complex shipping workflow we need by supporting the creation and rating of multiple shipments based on whatever criteria a site needs.

This also simplifies two other things: identifying the entity responsible for creating a price adjustment on an order (the adjustment simply references the shipment entity) and eliminating the need for the shipping module to retain pseudo-line items in its data object for selection on the checkout form.

Definitely interested in your feedback on the proposals, but I don't see any reason for every issue to simply be shut down like this. smccabe and their team are willing to invest a lot of time and energy into this module similar to their work on https://www.drupal.org/project/commerce_pos - but it's not like it's undirected / reviewed. : P

@googletorp: If you feel like being bypassed in the discussions about the right direction for the 8.x branch, I can imagine you perceive this like issue-hijacking.

As stated by bojanz and rszrama a lot of good work has been done already. If I understand you right, your main question has to do where that work must be located (a.k.a. in commerce_shipping or in commerce_shipping contrib).
That is a fair discussion. So it would be great if you can catch up with bojanz as proposed by him in #4.

From my understanding in Commerce 8.x-2.x "small core" has a different approach than in Commerce 7.x-1.x.
- We try to avoid the need for multiple contrib modules to create most common MVP implementations.
- We also try to avoid scattering of similar functionality in multiple modules.

One example (except shipping) are 'discounts/coupons'. In 7.x you needed multiple modules that were overlapping. A hell of job for site-builders to get this done.
In 8.x the baseline is done in commerce/core : https://www.drupal.org/project/issues/commerce?version=8.x&component=Pro...
A second example are 'stores'. In 7.x there were efforts to get this done, but it needed so much rewrites that is was practically impossible.
https://drupalcommerce.org/blog/42419/commerce-2x-stories-stores
Both are examples why we are moving from (my words) "micro core / smallest possible core" to "lean core".

So in the case of commerce_shipping I am also voting to keep continuing on smccabe's work.

TL:DR;
I think the way AM has handled this is crap, but I'm not against what they are doing (if it's actually good). But please don't use the issue queue for a rogue implementation behind module maintainers back, that will make him sad :)

@googletorp: If you feel like being bypassed in the discussions about the right direction for the 8.x branch, I can imagine you perceive this like issue-hijacking.

I have implemeted the module. I have maintained it for 5 years, yet no one found it relevant to reach out to me. Being bypassed is not a feeling, it's simple what has happened.

If the end result is good, I'm wont mind taking on a co-maintainer who is willing to maintain the code along with the actual code base.

I'm not happy with the fact that Acro Media guys felt there was reason to involve me. They have basically their own code base (not built from commerce_shipping) and a merge is not possible. They are basically creating their own module. (This is all good, they can make all the modules they want)

The only thing I ask, is that they don't use commerce_shipping issue queue to coordinate work on their module.

Please note, I'm not against this work. If Acro Media guys want to implement the module and make it better than it was in Drupal 7, that's great news.

I am unhappy with how this has been handled. They way things has turned out, I basically don't want to see any issues about what is going on in their github repo before:

a. They actually reach out to me and we have a chat about this.
b. They have a PR with a stable version of the new architecture.

@googletorp I understand the reason you feel the way you do, but I would also reiterate that by moving forward on a D8 version, no one is saying they don't appreciate your maintainership of the module here. However, the reason Acro Media has developed what's in their repo and used the queue the way they have is because they saw what they were doing as an extension of the 2.x branch, which I essentially developed from scratch.

They did involve me (and Matt Glaman, a Commerce co-maintainer) in reviewing and helping direct their thoughts when they could peel me away from directing the separation of CG from Platform.sh, and had I more time we certainly could've arranged to discuss it more together. (They've also blogged about it on Drupal Planet multiple times, so it hasn't been developed behind closed doors.) I can only apologize for not having time to involve you directly sooner, but I daresay the fastest way for your feedback to be heard would've been to actually put your thoughts in the issues they had open instead of shutting them all down. : P

In other words, I don't believe Acro has been going rogue or doing anything behind a maintainer's back (unless we don't count me as a developer / maintainer), and all modules written for Drupal 8 need to be rewrites anyways to truly leverage the new architecture (case in point: Drupal Commerce itself). My recommendation to them would've been to use the existing repository, create a new branch, and empty the sucker out to start from scratch anyways.

@smccabe Ultimately, we do want to preserve a commit history for the new branch inside this repository, and that may have been easier starting from a full clone if you didn't. That said, let's also make sure the history isn't full of a hundred little commits - i.e. the most useful history will be single, larger commits as features are developed with smaller commits just coming later as bugs are fixed.

I quickly read though the code found at https://github.com/smmccabe/commerce_shipping

From what I can tell.

• Module doesn't actually do anything atm (no integration to commerce)
• They decided to use .yml files to configure plugins, IMO we should use config entities to make it possible to configure in the interface.
• Standard documentation is missing (like documenting classes etc)
• Code parts has been disabled using comments
• Coding standards not followed everywhere.

So there still is a long way to go for this :)

They decided to use .yml files to configure plugins, IMO we should use config entities to make it possible to configure in the interface.

This was a decision I made with smccabe. If you look at Commerce we have many patterns of plugins in YML with a matching config entity, if needed.

@ryan I hear what you say.

I did comment on this issue (when I got back from holidays) basically saying, I'm not sure this is the way to go. smccabe hasn't actually responded, but has instead been "trolling" the issue queue (creating and closing issues).

I can confirm that he didn't make a fork/clone of the commerce_shipping repo.

I actually started work on this, with a different direction in mind back in May (before the blog post was posted).

All of this gives me the idea that smccabe would rather do their own thing, than actually collaborating on this.

Luckily I'm a pretty patient man, so I'm not tilting or anything.

That said I would prefer that the commece_shipping issue queue is reserved for working on commerce_shipping code base. I would also prefer that some general architecture consensus is made before tasks to implement random stuff are created.

This was a decision I made with smccabe. If you look at Commerce we have many patterns of plugins in YML with a matching config entity, if needed.

@mglaman I don't agree that this is the correct choice. Also the only place where yml discovery is used in commerce (from what I can tell) is for the order states, which is forced to do so, since we depend in the state machine module.

We should use config entities everywhere (looking at the code base I didn't find any config entities). Config entities can be created using a config files in config/install, but that's not how the plugins have been implemented.

Note I only used 5 mins to read the code base, so I might be missing some things, but generally I'm not too happy with the approach. From what I could tell it's mostly just a bunch of plugin and entity definitions.

So @googletorp was not involved in the discussions early on. Because he is a maintainer for years now this should have happened honestly. Not on purpose probably and it is never too late to catch up again.

Since a couple of weeks we have pingpong (closing/opening/closing) issues. Let's not continue that until there is agreement @googletorp included.
Detached development on github without drupal.org issues until it is finished is undesirable because that is not how our community works.

I think there are 2 routes here.

One route is "agreement to disagree". This is not preferred at all.
Then either the "small core" or the current work on github must move to a different namespace/module.
Which one of the two should be determined by voting I guess. But still I prefer consensus first.

The most preferable route is agreement on the direction and architecture between current maintainers of commerce_shipping (googletorp, rszrama), 8.x maintainers of commerce (bojanz, mglaman) and lead-developer of github branch now (smccabe).
As mentioned in #4 and #7: let's talk!
It can happen here in this meta-issue or in a scheduled call/meeting. Please reach out and do this.
When there is agreement on the main-architecture follow-up separate issues about implementation and refactoring can be opened and tackled one-by-one.

@ndf, I appreciate you taking the time to summarize things, but we do have ongoing projects that require Shipping in Commerce 2.x. The reality is we cannot stop making forward progress on the 8.x work on GitHub - nor do I think we need to keep that work out of this queue. (I'm just as much a maintainer of this module as googletorp, and Acro's work is actually based on my branch and conversations with me, Bojan, and Matt as Drupal Commerce maintainers as well...)

I really think we need a strategy that sees us adjust communications while still making forward progress. Happy to plan to meet at DrupalCon Dublin, though. Matt and I will both be there!

@rszrama ++

@googletorp, @rszrama,

We would love to work on Commerce 2 Shipping (we need it in a project). A fair bit of effort has already been spent, looking at the child issues here and the github repo. But work seems to have stalled since the difference in opinion between the two maintainers. Is there a way we can restart work on C2 Shipping? If the differences cannot be resolved we might explain that to visitors at the project page or the top of this issue: point two branches of future development. It seems such a shame that no progress is being made now, while the rest of Commerce and D8 are moving ahead.

Please let us know your thoughts on this.

@finne My status is that I'm waiting for a patch which can be applied to the 8.x-2.x branch, so we can review and potentially get this is. Seems like development on github has stopped though and code still looks incomplete.

Not sure what Ryan might be cooking for his customers.

From what I can tell we have

- initial implementation on Drupal.org with the basics (which could be developed into something similar to what commerce 7.x had)
- half baked prototype on github where development has stopped
- unknown code by commerce guys

Not sure which would be best to bet on. From what I saw on the github code, it requires improvement/refactoring before it can get it. I would like to see config entities being used over .yml file definitions, have GUIs etc. There is no real quoting and plugin code seems to be lacking the serious things. Most is just a lot of class definitions with getter/setter

I've scheduled some time with Matt Glaman to review the GitHub repo (which does seem to be mostly autogenerated code), yaml VS config for plugins, etc. We can then create a plan for moving forward. Stay tuned.

I had a call with Matt Glaman (mglaman, my Commerce co-maintainer) and Andy Giles (andyg5000, Box/Fedex/UPS/USPS contributor).
We reviewed the various initial plans as well as the two existing codebases (d.o module, GitHub repo). Both codebases are mostly empty and non-functional so we focused more on the architecture that we wanted to have. After reviewing a number of shipping APIs I also had a discussion with Ryan, where we validated and tweaked the plan.

I've reviewed the Active Shipping library, services that interface with multiple carriers (Shippo, Easypost, Postmen, Postmaster), and a few carrier APIs (FedEx, USPS, DHL, mostly through their Commerce 1.x integrations).

Note that this plan is not final. Not everything was validated, not everything will make sense as we start coding. But I believe it's a good start.

We need:
1) Shipping method plugin and matching config entity (as seen on checkout flows, payment gateways)
2) ShippingService and ShippingRate value objects
3) Package type YAML plugin type (previously known as Box)
4) ShipmentPacker plugin type
5) Shipment content entity
6) shipments reference field on orders
7) ShippingInformation checkout pane
8) Shipment refresh process

Now, in more detail...
1) Shipping method plugins are "flat rate", "flat rate by item", "FedEx", "USPS", "Shippo", etc. Each instance stores its configuration in a config entity, which has conditions ("shipment weight larger than 1kg", "shipping country is Canada", "user is premium") and a weight.

The shipping method can have configuration for API keys, other specific settings such as limiting the supported carriers in case of multi-carrier APIs. By default (base class) it stores the default package type, which might or might not be used by the Packer plugin.

Shipping methods define available services (Standard, Priority, Overnight, etc) in their annotation (a "services" id => label array).
The available services can be reduced in the plugin settings (just like payment method types on payment gateways).
The getServices() plugin method returns the allowed services as an array of \Drupal\commerce_shipping\ShippingService value objects (id, name), keyed by id.

Main methods:
- calculateRates(ShipmentInterface $shipment) -> returns a set of Rate value objects.
- selectRate(ShipmentInterface $shipment, ShippingRate $rate) -> no return value. Populates the shipment entity with the amount, method/service.
- purchaseRate(ShipmentInterface $shipment, ShippingRate $rate) -> no return value. Does an API call for the purchase when needed.
- getTrackingHistory($tracking_code) -> a set of TrackingStatus value objects indicating the tracking status, the date, the message.

We can postpone the tracking part to a followup, tweak the terminology.

2) The service object was described above. The \Drupal\commerce_shipping\ShippingRate object looks like this:
getShippingMethodId() -> The shipping method ID (that's the entity ID, not the plugin ID)
getShippingService() - a ShippingService value object.
getPrice() - instance of Drupal\commerce_price\Price
getDeliveryDate() - estimated date of delivery, if known
getDeliveryTerms() - string, a description shown to the user ("Delivery in 1 to 3 business days."), if known

This too is a value object (getters only, values populated via $definition in constructor, just like CreditCardType in Commerce).
Some APIs would allow for a getDeliveryDays() too (an int showing the number of business days for delivery), but we didn't find it to be useful next to the delivery date, so it was omitted.

3) The package type YAML plugin. Has a label, takes dimensions and weight.
Each remote API has this concept, though its naming varies.
This plugin type was initially called "box", which I liked. However, turns out that no API uses the "box" naming, and not every package type is a box (envelopes, cylinders, tubes are technically not boxes). So we chose "package type", which is how Fedex, UPS, eBay, Postmaster refer to it. Shipwire also has something similar (packaging type).
I'm not married to the package type naming. USPS uses "container" which felt too generic, Postmen uses "box type".
Ryan also suggested "packaging", which sounded a bit too verby to my non-native ears, but I'm open to other opinions.

So, shipping methods define their package types in YAML. You can see a sample list here. There is also a need for users to be able to define their own custom package types. For that we'd need a config entity that uses a plugin deriver to expose that data as plugins (like menu links in core). This too can be done as a followup.

Each shipping method instance has a default package_type setting that's used by the default Packer.

4) The ShipmentPacker plugin type is responsible for creating one or more shipments from a given order.
The default logic is "take all shippable products and put them in one shipment, assign the default package type".
Other implementations can create multiple shipments based on the stock locations (that's the primary advanced use case).

Each shipping method instance has a packer setting. An additional step would be to make packers configurable and store their settings in the shipping method config entity, but that can be a followup.

5) The shipment content entity has no bundles, but it does have a UI for attaching fields (settings form + tabs for the Field UI), that allows people to add a "Special instructions" field or a "Phone" field.

Base fields:
- Shipping method (entity reference)
- Shipping service (text field, stores the ID)
- Shipping profile (entity reference)
- State (state field)
- Items
- Tracking code (text)
- Amount
- Adjustments
- Created timestamp
- Changed timestamp
- Shipped timestamp

The states are for internal use, not shown to the customer. By default they are:
Draft -> Ready (to be shipped) -> Shipped -> Canceled
Draft shipments are on draft orders (haven't been placed yet). Cancelling an order cancels all shipments.

Items represent what's inside the shipment. My current thinking is to have them serialized ala adjustments, each ShipmentItem is a value object that has a purchased entity type, purchased entity id, quantity. We also talked about storing an order item ID but at this point I'm no longer convinced it's necessary. Might just need a use case for it though.
The reason why I want to serialize items is because they're going to be recalculated during the refresh. If our default behavior is "single shipment per order", then each change to the order (at cart/checkout) must be reflected in the shipment.

The amount is a price field, taken from the selected rate. Adjustments store handling fees and/or taxes.

6) For now let's just alter the order base fields and add a shipments reference field, just like cart/checkout/payment do with their own fields. In the future we'll need a mechanism for marking an order type as shippable, adding the needed field (we're calling the concept "entity traits" for now, and it would also be used on products to mark which types are shippable and which optionally have dimensions).

7) The checkout pane assumes that there's only a single shipment for now.
It then embeds the entity form for the shipment. It shows first the shipping profile (allowing the user to provide an address) then the available rates. If someone attached a custom field such as Special Instructions, it would show up as well.

A followup would make the pane ask for a shipping profile, then show an entity form for each shipment with the shipping profile element hidden. That allows for rates per shipment, while still keeping a common shipping profile.

8) The Shipping order processor calls the ShipmentPacker if $order->shipments is empty. It also always triggers the Shipment refresh, which invokes shipment processors. This process allows taxes and handling fees to be added to the shipment.

The order total handler will need to be able to take into account both line item and shipment adjustments when summarizing adjustments.

Also wanted to clarify a few assumptions:

1) Flat rate shipping is our primary use case (20k out of 26k Shipping 7.x installs use flat rate). The improvements to that workflow are more obvious conditions (by country, by weight), a Flat rate per item plugin.

2) We want to allow remote shipping methods (USPS, UPS, FedEx) to do less work by standardizing the package type, flow for purchasing a rate, tracking a purchased shipment.

3) Both remote APIs and stock benefit from the concept of a Packer plugin.

4) Because of #1 we won't force shippable products to have dimension/weight fields. Instead, we'll offer optional integration when the Physical module is enabled (along with a weight field on the Shipment), and allow the remote shipping method plugins to throw an exception when given products with no dimensions.

And for the UI side of shipments, the model is similar to payments: admin/commerce/orders/{commerce_order}/shipments.

I've updated the issue summary based on #19 and #20.

I just realized I can't add maintainers to this project for some reason. Can we make sure bojanz and mglaman have commit access, as they'll be actively engaged in this development? We may need to add jsacksick as well.

@Ryan, I have added Bojanz.

I would like see the code that goes into the project and I believe I can handle committing all things without being a bottleneck. I would like to still be able to maintain the project in case commerce guys wont have the resources to do it.

Please let me know if you feel it becomes a hindrance for the project, and we can rethink it, or have a chat on IRC.

Aye aye, will let you know. Generally speaking, maintenance and active development of a new branch are two different things; definitely appreciate your maintenance support on 7.x-2.x and any attention you can give the 8.x branch in the future, but given our team's experience / proposed architecture, I'd expect we'll be able to review new code internally before we commit it. I can't imagine a scenario in the next 5 years where we won't have time for the primary shipping suite for Commerce 2.x.

Ryan, I hear what you are saying. I thought that this was going to be the case 5 years ago, but it didn't exactly turn out like that. So in case of some odd circumstances create the same scenario, I would like to better prepared this time around.

I'm used to working as team lead and doing peer review, it's something I enjoy, so for me it shouldn't be an issue to look at what you are making. I hope you guys will review the stuff I write as well, so we can catch issues before they get committed.

#2825454: Reboot the 8.x-2.x branch has landed, 8.x-2.x-dev now has the described shipping method plugin and config entity (+UI), ShippingService and ShippingRate value objects, a flat rate plugin, an incomplete Shipment interface.

I'll create issues for the rest in the morning.
Still very open to feedback.

I forgot to reply to you Shawn, apologies.

The Physical module, which was a blocker, now has an RC1 release.
I just received two weeks of client funding to work on Shipping, so the pace should accelerate dramatically.
We want a beta by end of January.
I'll start by wrapping up the shipment interface and merging swickham's and anthonygq's work.

For product properties like dimensions and weight, is the idea to have the shipping module have some standard getWeight and getDimensions methods to act as standard interface, such as getting values from physical or returning null or an exception if it is not available, or would be Shipping Method be expected to handle that directly?

We'll need to be able to differentiate between "physical" and "physical with dimensions" products. Both shippable, but only the second group has the weight/dimension fields. Flat rate doesn't care about dimensions & weight. Others do, and would need a way to signal that they only accept such products. We'll see how this turns out.

or would it be expected to provide defaults for tracking?

Tracking would be a separate interface. We obviously can't provide that for many/most flat rate scenarios.

I assume the plan is to use the conditions work Matt Glaman was doing and NOT rules like was previously used, correct?

Correct, we need to take the code written for Promotions and then start improving it.

We now have a beta1: https://www.drupal.org/project/commerce_shipping/releases/8.x-2.0-beta1
See the README for setup instructions.

The plan from #19 has been implemented. We also have support for tracking urls in the API, but no support for purchasing shipping labels yet.
A lot of iteration was done on Packers to make them as flexible as possible for all use cases.
We've also done a lot to support the address-per-shipment use case, which is a use case we didn't plan to support initially, but needed to because that was Adapt's use case. It requires custom code to complete (they will be turning their code into a contrib at some point), but at least the module doesn't do anything against it.