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.

Sign up for GitHub

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

Jump to bottom

[REF][PHP8.1] Fix test failure on civiimport unit test because sequen… #24736

Merged
merged 1 commit into from
Oct 20, 2022
Merged

[REF][PHP8.1] Fix test failure on civiimport unit test because sequen… #24736

merged 1 commit into from
Oct 20, 2022

Conversation

seamuslee001
Copy link
Contributor

…ce key and keys are using id not _id

Overview

This fixes the following test failure

CiviApiImportTest::testApiActions
PEAR_Exception: DB_DataObject Error: update: trying to perform an update without
                        the key set, and argument to update is not
                        DB_DATAOBJECT_WHEREADD_ONLY
                    Array
(
    [seq] => Array
        (
            [0] => id
            [1] => 1
        )

    [keys] => Array
        (
            [0] => id
        )

)


/home/jenkins/bknix-edge/build/build-2/web/sites/all/modules/civicrm/CRM/Core/Error.php:955

Before

Test fails on php8.1

After

Test passes on php8.1

@civibot
Copy link

civibot bot commented Oct 13, 2022

(Standard links)

@civibot civibot bot added the 5.55 label Oct 13, 2022
@seamuslee001 seamuslee001 reopened this Oct 19, 2022
eileenmcnaughton
eileenmcnaughton reviewed Oct 20, 2022
View reviewed changes
ext/civiimport/Civi/BAO/Import.php Outdated
if (!isset($keys)) {
$keys = ['_id'];
}
return $keys;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we have all this rather than just return ['_id'] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok updated to that @eileenmcnaughton

eileenmcnaughton
eileenmcnaughton reviewed Oct 20, 2022
View reviewed changes
ext/civiimport/Civi/BAO/Import.php Outdated
public function sequenceKey() {
static $sequenceKeys;
if (!isset($sequenceKeys)) {
$sequenceKeys = [$this->getFirstPrimaryKey(), TRUE];
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we could also just return ['_id', TRUE] here - seems overly complicated to be setting statics & calling functions when the known thing is that it is _id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok updated @eileenmcnaughton

@seamuslee001
[REF][PHP8.1] Fix test failure on civiimport unit test because sequen…
2100b9a 
…ce key and keys are using id not _id

Additional patches from Eileen
@seamuslee001 seamuslee001 merged commit 629f26b into civicrm:5.55 Oct 20, 2022
@seamuslee001 seamuslee001 deleted the import_ext_test_fix branch October 20, 2022 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eileenmcnaughton eileenmcnaughton eileenmcnaughton left review comments

Assignees
No one assigned
Labels
5.55 merge on pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@seamuslee001 @eileenmcnaughton