Alter method of getting the donation item

EDIT: Whoops, may not be applying

This one doesn't reorder the use statements.

Not my day, ha. This is an error on bool not on null. Guess I forgot how to read.

Alright, just a simple ! check instead of isset

@FatherSean I think this updated approach makes sense. I just made some small changes to simplify the logic in initDonationItem(). What do you think? Also, do you think this bit is needed anymore?

// Ensure an order item is set and easily found.
$items = $instance->order->getItems();
if (empty($items)) {
  /* @see \Drupal\commerce_donation_flow\NewOrder::get() */
  throw new \UnexpectedValueException('Order Items not properly initialized');
}

If the there are no items on the order then I think the while loop will handle that on its own by creating one.

And actually a question about submitPaneForm().

  /**
   * {@inheritdoc}
   */
  public function submitPaneForm(array &$pane_form, FormStateInterface $form_state, array &$complete_form) {
    $donationItem = $form_state->get('donation_item');
    if ($donationItem instanceof OrderItemInterface) {
      $donationItem->save();
    }
    else {
      // Allow extending panes to catch an Exception if desired to deal with
      // a missing donation OrderItem.
      throw new MissingDonationItemException('Instance of OrderItemInterface not stored as `donation_item` in FormState');
    }
  }

I don't know if we necessarily want to throw an error. If the orderItem was deleted/unset before this method fires, wouldn't it be valid to just not save the item? The exception might problematic because I don't think it could be caught by any logic other than an extending pane. If somebody is using the default pane, they don't really have a way to catch the exception. Actually, if this is a problem here, then the LengthException I added in validatePaneForm could be problematic as well.

Oh right, I forgot that this was a Base class so yes, those exceptions do make sense. I'll need to do some testing to see how the latest changes work with our flow, but they all look like they should work out fine.

One thing I notice is that a null $donationItem could be passed into buildDonationItem() and I'm not sure if there's handling for that. Might need to add a guard to buildDonationItem() to ensure we aren't trying to call methods on a NULL item.

Re #12, not sure I'm totally understanding what you mean. That said, at the moment we are using the default checkout flow with the donation pane on the "Order Information" step, which is the first pane that a customer encounters unless they are anonymous and hit the Login step. In the case of a logged in user, I don't know that they would have the chance to encounter this proposed new pane before hitting the step with the donation pane. I'm happy to test things out, though.

Oh just one other thought. It's certainly preference so could go either way, but I wonder if getDonationItem() might be a better name than donationItem() now that it's acting as a getter method instead of a property?

Oh for #12, if the idea is to make the donation optional, we are actually handling that on our own by adding a checkbox to the donation pane itself to keep or remove a donation and apply the user's desired pricing. When the order is created, we are using NewOrder::addDonationItem() to create a $0 donation item and just hiding it in the order summary. Then when they submit that pane and continue to review the order, the donation item price is updated or the item removed based on their selections.

It would probably be more graceful to not need to add the $0 donation item on order creation. I don't think this was possible with the current alpha/dev, but might be possible with all of these updates.