fixes core#4027 - don't crash when a contribution has no line items #25145
Conversation
|
(Standard links)
|
|
Jenkins re test this please |
| if ($lineItems) { | ||
| $this->setPriceSetID($lineItems[0]['price_field_id.price_set_id']); | ||
| } |
There was a problem hiding this comment.
I think we should log a warning here because there's likely to be other issues and the "real" fix really needs lineitems to be created otherwise the data is effectively invalid. As we move more towards checking lineitems it will probably create more problems.
|
@MegaphoneJon Have added a comment about logging a warning. A "better" fix would be ensuring all contributions have lineitems and possibly creating them if they don't? But that's obviously a much more complex fix and I'd say outside of the scope of this PR. |
|
@mattwire I certainly considered that - but I don't know how one would go about creating those line items. If there's a function I could build off of, I would certainly consider this approach. |
|
@MegaphoneJon I just want to understand this comment - "// Contributions should all have line items, but historically, imports did not create them." - how long back are we looking here? Re adding logging - I would suggest a Status Check would be better than logging - if we know to look for a certain sort of issue we should probably do it up front rather than at run-time. But I don't think adding one of the other has to be in scope for this PR. I'm mostly merge-ready on it - I just want to understand a little more about the contributions that don't have them |
|
Hi all, I'm responding since Seamus pinged me on this one. We did discover this independently when performing testing prior to making Civi 5.53 live. I made a similar patch (but I think this one is a bit better than mine). The patch fixed the issue and we've been running this for a couple of months now without any further issues. |
|
Just noting I found we have 683 contributions dating between 2012 and 2015 with this issue. I wasn't able to determine how they came about but good to see none are recently created & having evaluated them we are probably not going to create the missing financial entities (small number involved, old contributions) & as @MegaphoneJon mentions it isn't entirely straight forward. Anyway - I'm going to merge this now since I think the conversation is generally supportive - with some possible follow ups around logging or status checks or data fixes. I will also include in the 5.56 backport PR - this is similar priority level to the others already being backported, none of which are enough to trigger a drop but suitable to include in one |
Overview
Full details are on https://lab.civicrm.org/dev/core/-/issues/4027.
Before
Crash when viewing a contact's contribution tab if a recurring contribution is present and its "template" contribution has no line items.
After
Displays normally.
Comments
This is a regression from 5.53 - which seems close enough to put against the RC.