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

Enotice fix #20122

Merged
merged 1 commit into from
Apr 22, 2021
Merged

Enotice fix #20122

merged 1 commit into from
Apr 22, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fixes an enotice
image

Before

Adding a managed entity using api v4 works but gives an enotice (other gaps exist but out of scope here)

After

poof

Technical Details

Comments

@civibot
Copy link

civibot bot commented Apr 22, 2021

(Standard links)

@civibot civibot bot added the master label Apr 22, 2021
@@ -237,7 +237,7 @@ public function reconcileUnknownModules() {
*/
public function insertNewEntity($todo) {
$result = civicrm_api($todo['entity'], 'create', $todo['params']);
if ($result['is_error']) {
if (!empty($result['is_error'])) {
$this->onApiError($todo['entity'], 'create', $todo['params'], $result);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does my read of this match what you're saying:
When api4 succeeds, there is no expected is_error element (because it's an object instead).
When api4 fails, there should still be an array with $result['is_error']=1. (That's not quite what I get when I try it, but maybe that's the gap you mean.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think api4 throws an exception & would bypass this anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

api4 by itself might throw exception but this is the wrapper civicrm_api() which doesn't. I think this is ok just when I tested I got some other kind of error - sorry that's not the most detailed bug report but I didn't want to spend a lot of time on it if it was already a known "gap" since you mentioned there were some.

@colemanw colemanw merged commit 6c85ac1 into civicrm:master Apr 22, 2021
@colemanw colemanw deleted the mgd branch April 22, 2021 21:03
wmfgerrit pushed a commit to wikimedia/wikimedia-fundraising-crm that referenced this pull request Apr 29, 2021
Note that I'm just adding this at this stage - but when we enable
we need to configure the right monolog loggers - see the readme for what we get by default

To add a wmf logger we will need to add a new monolog logger and then
we can, for example, do Civi::log('wmf')->debug('merging contacts 1 & 2')

And that will be output to whatever is configured for channel 'wmf'
with debug or lower as minimum severity.

The firephp thing is kinda fun.

Known issues
- I planned to replace failmail too but we have an out of date version of phpmailer so I didn't
bring it into scope on this round
- Error on disable civicrm/civicrm-core#20144
- Notice on enable civicrm/civicrm-core#20122
- I wanted to create a full edit form but pending https://lab.civicrm.org/dev/core/-/issues/2567
- And I wanted to display the configured below the add - https://lab.civicrm.org/dev/core/-/issues/2569

Current commit https://lab.civicrm.org/extensions/monolog/-/commit/d7ff4a06c28af7cb84007e992b471566b995df67


Bug: T279983

Change-Id: I981513ea506a6484871b9db13d769bbf8c7927b6
wmfgerrit pushed a commit to wikimedia/wikimedia-fundraising-crm that referenced this pull request May 10, 2021
Note it can be found via Administration->Customise Data And Screens->Deduper->Deduper equivalent Names

It should show up after you do a menu rebuild (hit the url civicrm/menu/rebuild) or a System flush
might be needed too (System.flush is an api you can hit via drush or the api explorer).

Known e-notice civicrm/civicrm-core#20122 - I wasn't going to backport that one

I documented here
https://civicrm.org/blog/eileen/leveraging-search-kit-deduper

Note I also made some release changes & a partial fix related to getting the docs to publish
Bug: T244404

Change-Id: Ia2573d6b50b99af6c7a33c3a8b531c0710bbcd6c
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.

3 participants