[REF] Catch Exceptions caused by CRM_Utils_Array::single() if there i… #32338

seamuslee001 · 2025-03-11T01:16:24Z

…s either no record found for potential permission reasons

Overview

It is possible that 0 records might be returned if permissions don't get checked or the cache is stale for some reason. I figure catching the exceptions thrown here https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/Array.php#L1218 is probably useful and logging them

Before

Potential for Expected to find one record but found nothing errors

After

Contact pages load

ping @eileenmcnaughton @andrew-cormick-dockery @johntwyman

civibot · 2025-03-11T01:16:27Z

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

➡️ PR Demo site (can be rebuilt with the command test this please).
📖 Review standards
🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

andrew-cormick-dockery · 2025-03-11T03:30:05Z

CRM/Custom/Page/CustomDataTrait.php

+      ])->single();
+    }
+    catch (CRM_Core_Exception $e) {
+      Civi::log()->warning('Tried loading Custom field data for Entity ' . $entity . ' id: ' . $id . ' and got the following message . ' $e->getMessage());


Quote is in the wrong place. should be ' and got the following message: ' . $e->getMessage()

This makes me think that this has not been tested at all. This gives a parse error, even when the exception is not caught.

We're currently running this patch in production (with the parse error corrected).

Prior to the implementation of this patch, we had many reports of "Expected one Address but found 0" occurring intermittently (as in, people who experienced this error upon loading a contact were able to load it without issue later in the day).

There has not been such a report since, and I can see quite a number of instances where the new logs appearing.

It's important to understand that we do not have any custom fields associated with Addresses or Emails, so the fact that this function simply returns with an empty array doesn't seem to have any impact on the operation of our site.

Sjord · 2025-03-11T08:37:01Z

[REF] Catch Exceptions

What does [REF] mean here? Doesn't that mean refactor? This seems like a functional change and not a refactor. Does that [REF] belong here?

Sjord · 2025-03-11T08:38:26Z

CRM/Custom/Page/CustomDataTrait.php

+          ['id', '=', $id],
+        ],
+        'checkPermissions' => TRUE,
+      ])->single();


Is single() still the correct method to call? It is meant for when you are sure that there is only a single result, but that you catch the exception seems to imply that this isn't always the case.

The problem is that sometimes the array returned is empty, presumably due to a failed ACL check, not because there is actually no Address or Email. This leads the call to fail, which is now captured by the exception.

@Sjord I feel like to keep the change as minimal as possible it makes sense to keep the code as is and just catch the exception as needed.

Sjord · 2025-03-11T08:39:15Z

CRM/Custom/Page/CustomDataTrait.php

+        'checkPermissions' => TRUE,
+      ])->single();
+    }
+    catch (CRM_Core_Exception $e) {


Could this exception be made more granular? That this only catches a NotASingleResultException or something like that?

@Sjord that would mean changing the exceptions thrown in CRM_Utils_Array::single but that might have changes required in other places. But also its probably worth catching any API exception which is generally a CRM_Core_Exception here anyway e.g. DB errors and the like.

Sjord · 2025-03-11T08:40:40Z

CRM/Custom/Page/CustomDataTrait.php

+      ])->single();
+    }
+    catch (CRM_Core_Exception $e) {
+      Civi::log()->warning('Tried loading Custom field data for Entity ' . $entity . ' id: ' . $id . ' and got the following message . ' $e->getMessage());


This makes me think that this has not been tested at all. This gives a parse error, even when the exception is not caught.

eileenmcnaughton · 2025-03-12T04:54:56Z

@seamuslee001 can you explain any further how this line would come to retrieve no records? It does seem a bit odd - perhaps it is around when they don't actually have permission to see it?

seamuslee001 · 2025-03-12T05:03:10Z

@eileenmcnaughton yeh @andrew-cormick-dockery seems to think there is some ACLs / cache at play

I do note that the overall page permissions here https://github.com/civicrm/civicrm-core/blob/master/CRM/Contact/Page/View.php#L264 ignore the acl_contact_cache table all together but the APIv4 does in checkPermissions=True.

I think AUG have been getting some ACL cache deadlocks.

To my mind it is kind of a refactor of code. I will fix the syntax error and I just threw this up so ACD could try and deploy onto their D10 instance.

…s either no record found for potential permission reasons

eileenmcnaughton · 2025-03-12T05:34:53Z

OK - we can maybe wait & for a few days & see if more info surfaces ?

seamuslee001 · 2025-03-12T06:15:09Z

I know AUG have put this into their production to at least mitigate the problems the users were having

andrew-cormick-dockery · 2025-03-12T06:34:32Z

I know AUG have put this into their production to at least mitigate the problems the users were having

And it appears to be achieving its aims (so far at least).

civibot bot added the master label Mar 11, 2025

andrew-cormick-dockery reviewed Mar 11, 2025

View reviewed changes

Sjord reviewed Mar 11, 2025

View reviewed changes

[REF] Catch Exceptions caused by CRM_Utils_Array::single() if there i…

a878301

…s either no record found for potential permission reasons

seamuslee001 force-pushed the catch_exception_when_loading_customdata branch from 74787ac to a878301 Compare March 12, 2025 05:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REF] Catch Exceptions caused by CRM_Utils_Array::single() if there i… #32338

[REF] Catch Exceptions caused by CRM_Utils_Array::single() if there i… #32338

seamuslee001 commented Mar 11, 2025

civibot bot commented Mar 11, 2025

andrew-cormick-dockery Mar 11, 2025 •

edited

Loading

Sjord Mar 11, 2025

andrew-cormick-dockery Mar 12, 2025

Sjord commented Mar 11, 2025

Sjord Mar 11, 2025

andrew-cormick-dockery Mar 12, 2025

seamuslee001 Mar 12, 2025

Sjord Mar 11, 2025

seamuslee001 Mar 12, 2025

Sjord Mar 11, 2025

eileenmcnaughton commented Mar 12, 2025

seamuslee001 commented Mar 12, 2025

eileenmcnaughton commented Mar 12, 2025

seamuslee001 commented Mar 12, 2025

andrew-cormick-dockery commented Mar 12, 2025

[REF] Catch Exceptions caused by CRM_Utils_Array::single() if there i… #32338

Are you sure you want to change the base?

[REF] Catch Exceptions caused by CRM_Utils_Array::single() if there i… #32338

Conversation

seamuslee001 commented Mar 11, 2025

Overview

Before

After

civibot bot commented Mar 11, 2025

andrew-cormick-dockery Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjord commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Mar 12, 2025

seamuslee001 commented Mar 12, 2025

eileenmcnaughton commented Mar 12, 2025

seamuslee001 commented Mar 12, 2025

andrew-cormick-dockery commented Mar 12, 2025

andrew-cormick-dockery Mar 11, 2025 •

edited

Loading