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

dev/core#592 Add default for in Exception class. #13263

Merged
merged 1 commit into from
Dec 13, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fixes non-fatal error

ArgumentCountError: Too few arguments to function CiviCRM_API3_Exception::__construct(), 1 passed in [drupal root]/sites/all/modules/civicrm/Civi/API/Api3SelectQuery.php on line 130 and at least 2 expected in CiviCRM_API3_Exception->__construct()

Before

Above error occurs... when an error occurs

After

Only the error occurs....

Technical Details

Throw line is

            throw new \CiviCRM_API3_Exception("'$key' specified in OR group but not added to params");

parent is

    public function __construct($message = "", $code = 0, Throwable $previous = null) { }

so adding a default is closer to the parent

Comments

https://lab.civicrm.org/dev/core/issues/592

More similar to parent & fixes a notice
@civibot
Copy link

civibot bot commented Dec 13, 2018

(Standard links)

@civibot civibot bot added the master label Dec 13, 2018
@seamuslee001
Copy link
Contributor

I'm going to merge this as it seems to be the more correct solution

@seamuslee001 seamuslee001 merged commit 78a3f61 into civicrm:master Dec 13, 2018
@seamuslee001 seamuslee001 deleted the api_ex branch December 13, 2018 06:27
@bjendres
Copy link
Contributor

bjendres commented Dec 13, 2018

I'm going to merge this as it seems to be the more correct solution

@seamuslee001: I agree, and definitely more robust.

But shouldn't the default value be undefined, following this or this logic? @eileenmcnaughton?

@eileenmcnaughton
Copy link
Contributor Author

Hmm maybe -I looked at the parent's default which was 0

@bjendres
Copy link
Contributor

bjendres commented Dec 13, 2018

That also makes sense. Let's leave it as it is for now :)

@eileenmcnaughton
Copy link
Contributor Author

@systopia - I'm glad you came around to the less-effort option :-)

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