Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-21068 - getLastModified fails more gracefully now. #10863

Merged
merged 1 commit into from
Aug 27, 2017

Conversation

rtobias81
Copy link
Contributor

@rtobias81 rtobias81 commented Aug 15, 2017

Overview

This helps resolve some errors when trying to view contacts.

Before

Without this change, there were cases were some contacts were throwing Not Of Type Integer errors in the Contact Summary view (from getDisplayAndImage).

After

This change makes the getLastModified fail more gracefully, so it doesn't display an error if something is missing for a contact when getDisplayAndImage is called from the Summary View.

Technical Details

Comments


@civicrm-builder
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@mattwire mattwire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rtobias81 As discussed on chat.civicrm.org :-)

@@ -42,18 +42,22 @@ class CRM_Core_BAO_Log extends CRM_Core_DAO_Log {
* @param string $table
*
* @return array|null
*
* This just has a couple of small changes suggested by Eileen to make this function fail more gracefully.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment doesn't have much meaning once this change is committed and should probably be removed. It's more of a "commit message" comment

if ($log->find(TRUE)) {
list($displayName, $contactImage) = CRM_Contact_BAO_Contact::getDisplayAndImage($log->modified_id);
if ($log->modified_id) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, I might want a short comment explaining why we check modified_id

@totten
Copy link
Member

totten commented Aug 15, 2017

jenkins, ok to test

@eileenmcnaughton
Copy link
Contributor

@rtobias81 do you know how to deal with the style warnings? We use drupal whitespace conventions
https://www.drupal.org/docs/develop/standards/coding-standards.

@rtobias81
Copy link
Contributor Author

Not really. Or at least not if it is anything other than just going back and fixing the spacing issues. It helps when you can see (I didn't realize I didn't have the right indenting for the function declaration). That said, I saw it complaining about stuff in lines 71-90something, which I find odd as I only replaces lines 48-69. Can I just commit my fixed function declaration spacing and rerun the test to see if that fixed other issues?

@eileenmcnaughton
Copy link
Contributor

Yep, if you fix the spaces & do git commit --amend & then git push -f to keep it clean jenkins will try again

@rtobias81 rtobias81 force-pushed the lastModified-graceful branch 4 times, most recently from e987180 to c1d6875 Compare August 15, 2017 21:42
@eileenmcnaughton
Copy link
Contributor

@rtobias81 this looks good. I want to push you a little more for a couple of bits of tidy up

  1. edit the PR description to fill in the fields above (overview, before after)
  2. this is kinda more advanced git - but your commits have wound up with messages that don't accurately describe them. Can you have a go at tidying them up. The way to do it is (assuming your remote branch for the core repo is called 'upstream', change to 'origin' or whatever if not'
    git rebase -i upstream/master
    You will then see a list of commits in vi or similar, on the second one change the word 'pick' to 's' for 'squash'. It will then squash the 2 commits together & give you a chance to edit the message.

When you push to your branch on github you will need to use -f as you are rewriting history (you didn't know the past could be changed so easily!)

@eileenmcnaughton
Copy link
Contributor

add to whitelist

@rtobias81 rtobias81 force-pushed the lastModified-graceful branch from c11ef9a to e89461f Compare August 16, 2017 16:00
@rtobias81 rtobias81 force-pushed the lastModified-graceful branch from e89461f to 4c7ef0e Compare August 16, 2017 17:16
@eileenmcnaughton eileenmcnaughton merged commit df809d9 into civicrm:master Aug 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants