The provided checkout panes throw errors in the event that a donation item is not found on the given order. However, all panes are loaded by Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowWithPanesBase->getPanes() (Line: 117) regardless of whether or not they are active for the current checkout flow.

Adding patch that checks for the existence of the panes in the current checkout flow. This is a bit awkward wrt to DonationCheckoutPane && DonationItemPaneBase, as commerce_donation_pane is defined by the prior, but we need to check for its existence in the latter. May want to refactor that logic a bit.

CommentFileSizeAuthor
#12 commerce_donation_flow-panes_break_other_flows-3178042-11.patch1.79 KBfathershawn
#10 commerce_donation_flow-panes_break_other_flows-3178042-10.patch3.21 KBmrweiner
#8 interdiff_2_8.txt4.01 KBfathershawn
#8 commerce_donation_flow-panes_break_other_flows-3178042-8.patch1.45 KBfathershawn
#2 commerce_donation_flow-breaking_pane_definitions-3178042-2.patch3.19 KBmrweiner

Comments

mrweiner created an issue. See original summary.

mrweiner’s picture

mrweiner commented
StatusFileSize
new commerce_donation_flow-breaking_pane_definitions-3178042-2.patch3.19 KB
mrweiner’s picture

mrweiner commented
Status: Active » Needs review

Whoops, setting status

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

@mrweiner I'll have a look - thanks for the patch.

I want to be sure I understand your case. Since the project that we generalized and extracted this from had only one active checkout flow, is your case that there is more than one active flow and these panes are causing an issue in the flow where they are not used?

DonationItemPaneBase provides common code for two panes used in our flow, as it also is a base class for MemorialCheckoutPane

Status: Needs review » Needs work

The last submitted patch, 2: commerce_donation_flow-breaking_pane_definitions-3178042-2.patch, failed testing. View results

mrweiner’s picture

mrweiner commented
Status: Needs work » Needs review

Yup, still building the site but there's another, pre-existing "default" checkout flow that we have running. So I'm seeing the errors from these panes when checking out with the Default flow even though the panes are disabled there.

mrweiner’s picture

mrweiner commented
Status: Needs review » Needs work
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
StatusFileSize
new commerce_donation_flow-panes_break_other_flows-3178042-8.patch1.45 KB
new interdiff_2_8.txt4.01 KB

The reason for throwing an exception is that everything the pane does depends on data from a donation OrderItem. Your case didn't have trouble with MemorialCheckoutPane because it overrides the default isVisible() method and includes a check for this object. This patch adds an inVisible() method to the base class and the ThankYouPane.

  public function isVisible() {
    if (!$this->donationItem instanceof OrderItemInterface) {
      return FALSE;
    }
    return TRUE;
  }

A pane that is not visible will not be built in the flow and this will accomplish the same purpose.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Needs work » Needs review
mrweiner’s picture

mrweiner commented
StatusFileSize
new commerce_donation_flow-panes_break_other_flows-3178042-10.patch3.21 KB

Still seeing the errors with #8 applied. Actually, even if I delete all of the logic in DonationItemPaneBase::create() and return the $instance early, DonationItemPaneBase::isVisible() doesn't seem to fire during my Default flow. I suspect that isVisible() only fires if the pane itself is enabled, while create() seems to be fired no matter what.

My initial patch actually broke the donation flow itself, but here's an alternative approach. Just checks the $configuration to see whether or not the pane is enabled.

You could certainly add the isVisible() stuff as well if you wanted to, but that will affect pane visibility when it is enabled.

Status: Needs review » Needs work

The last submitted patch, 10: commerce_donation_flow-panes_break_other_flows-3178042-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
StatusFileSize
new commerce_donation_flow-panes_break_other_flows-3178042-11.patch1.79 KB

Oh! I dropped the the check in create() for ThankYouPane but missed dropping it in DonationItemPaneBase. That was part of my whole point. It's important not to ask the pane to do it's work without a donation OrderItem, but that shouldn't keep the pane from being instantiated as an object. Throwing an exception in create() prevents instantiation, but putting a check in isVisible() keeps the pane from being presented.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Needs work » Needs review
mrweiner’s picture

mrweiner commented

Ah! Thought you did that on purpose, ha. Alright yeah I see what you're saying. No need to even check whether it's enabled or disabled since Commerce will check on its own after it's been instantiated and then, if it is enabled, isVisible() will determined whether or not to display it.

Looks good to me. Thanks!

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Great :-) If it works in your use case, please mark as RTBC! The automated functional test passes so that's a good first sign.

mrweiner’s picture

mrweiner commented
Status: Needs review » Reviewed & tested by the community

  • FatherShawn committed 7e86585 on 1.0.x 
    Issue #3178042 by mrweiner, FatherShawn: Pane definitions break non-...
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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