WIP ensure line items without price field id set doesn't cause problems

[jmcclelland]

Addresses https://lab.civicrm.org/dev/core/-/issues/3150

Overview

A recent change (https://lab.civicrm.org/dev/core/-/commit/4df23f2a270c08bc42fd1ff93fcfe318ecdc0e19) causes CiviCRM to expect all line items to have a value for the price_field_id. In some workflows, a line item with a NULL price_field_id causes a back trace when browsing contributions and when processing new payment processor charges.

Before

Steps to reproduce (Note: I don't know how to "naturally" create a contribution with a line item that has a NULL price_field_id or even if it's possible):

1. Create a recurring contribution.
2. Add a contribution record to it.
3. Find the resulting line item and set the price_field_id to NULL.
4. Browse to the contact record and click the Contributions tab
5. Get a traceback:

[Symfony\Component\Debug\Exception\FatalThrowableError]                                                
  Type error: Argument 1 passed to CRM_Financial_BAO_Order::setPriceSetID() must be of the type int, null given, called in /var/www/powerbase/sites/all/modules/civicrm/CRM/Financial/BAO/Order.php on line   
  905 

[You have to open the console and open the ajax call in a new tab to see the traceback message.]

After

After changing the code to detect a NULL price_field_id and setting the default price set ID automatically, the problem is resolved.

Technical Details

It's not clear to me whether all line item really should have a price field id set (and it should be set to the default value for contributions or memberships depending on the line item itself) OR if CiviCRM should accomodate line items wihtout price field ids set.

This PR provides a test to demonstrate the problem and experiments with the accomodation approach to see what it would look like to gracefully handle that situation in at least one known worflow where a missing line item is found.

Would it be better to have an upgrade script simply fill in the missing values in the civicrm_line_item table???

Comments

This problem has serious implications for payment processors. For example, with iATS, it can lead to monthly recurring donors getting charged daily.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[jmcclelland]

I'd love to get your feedback on this one @eileenmcnaughton - thanks!!

[jmcclelland]

Also wondering if, @JoeMurray, you have any thoughts since I know you have spent a lot of time on the financial side of CiviCRM. Are all line items exepcted to have a price field id set?

[eileenmcnaughton]

@jmcclelland yes - I think the current model IS that all line items are attached to a price set - having said that we have had some soft discussions about changing it so you can mix and match lines from different price sets as part of how we might add creating orders to form builder - but the underlying work for that isn;t done at this stage

[eileenmcnaughton]

@jmcclelland for 'simple' contributions the default price set is expected. - the api to create orders should or the repeattransaction api should ensure that is the case & as long as the custom code is using those core api then it is a core bug if price_set_id is not populated for some reason

[jmcclelland]

Thanks for the feedback Eileen. So far this doesn't seem to be a widespread problem with Powerbase sites. Only two of our sites seem to have a signficant number of line items without a price_field_id value set.

Surveying all of our sites I found:

• 10% of all sites have at least one line item missing a price_field_id value
• Only 1.3% of sites have a line item missing a price_field_id that is linked to a recurring contribution (and ones not linked to a recurring contribution do not trigger a traceback when viewed)

So, all in all, I've narrowed it down to one site with over 3,000 broken line items, one site with about 100, one site with 1 and another site with 2. I'm guessing the sites with 1 and 2 are just weird flukes and will try to find similarities between the other two sites to see if I can find a pattern.

For now I'll close this PR.

Thank you!

[eileenmcnaughton]

Have fun @jmcclelland

[jmcclelland]

For the record, I tracked the problem down to groups receiving recurring contributions via a webform who did not have the transaction legacy extension installed, which feels like a very solid explanation for the problem. Thanks for your help @eileenmcnaughton!

[eileenmcnaughton]

@jmcclelland I think the latest webform doesn't require that?

[jmcclelland]

Oh - good to know! @mattwire or @colemanw - can you confirm that we no longer need to use the transaction legacy extension with webform civicrm when accepting contributions?

[jmcclelland]

For the record, it appears that the Contribution.Transact legacy problem is fixed in the Drupal 8 version of webform, but not in the Drupal 7 version.

But actually...the fix for Drupal 8 does not seem to really be a fix - it seems to just copy the old legacy code from CiviCRM core. So you get all the same badness but without the deprecation warnings.

[eileenmcnaughton]

@jmcclelland oh ok....

[jackrabbithanna]

We've encountered this issue on at least 2 sites, after upgrading to ESR 5.45
Would be nice if it was fixed!!!
Should a LineItem need a price_field_id ?
What was the reason for not wanting this fix, or at least a fix?

[eileenmcnaughton]

@jackrabbithanna yes - line items DO need a price_field_id - that is part of the data schema & core + supported core apis do ensure that there is one - that old transact api never seemed to go away despite us telling people it wasn't tested or supported - but I don't know exactly why it would have created contributions which didn't have valid line items since AFAIK it called Contribution.create api which Does

[jackrabbithanna]

Ok, good to know. I know that it is possible to create line items via APIv3 without a price field id, that is no API error or SQL constraint error.

[eileenmcnaughton]

Yeah - we should really have that constraint since the code expects it