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

[REF] Cleanup on import rows error #20196

Merged
merged 1 commit into from
May 20, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

[REF] Cleanup on import rows error

Trying (and not yet succeeding) to replicate https://lab.civicrm.org/dev/core/-/issues/2566
I was able to step through & eliminate one more place where we call dao->query() instead
of CRM_Core_DAO::executeQuery

Before

$db->query($query, $args);

After

CRM_Core_DAO::executeQuery

Technical Details

This is a pretty minor simplification but am looking for more help replicating https://lab.civicrm.org/dev/core/-/issues/2566 - which could be done in conjunction with an r-run on this

Comments

@civibot
Copy link

civibot bot commented Apr 30, 2021

(Standard links)

@civibot civibot bot added the master label Apr 30, 2021
@eileenmcnaughton eileenmcnaughton force-pushed the import branch 3 times, most recently from 89b7039 to 223eda0 Compare April 30, 2021 04:38
Trying (and not yet succeeding) to replicat https://lab.civicrm.org/dev/core/-/issues/2566
I was able to step through & eliminate one more place where we call dao->query() instead
of CRM_Core_DAO::executeQuery
@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy @jitendrapurohit - is this something either of you would review? It's a minor refactor I did while trying to replicate something else

@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • Gave it a little run while trying to replicate the original lab issue, which I did - see lab.
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
      • It was there before but variables inserted into sql statements make me nervous, but I suppose it's limited here how they get their values.
      • For the message string, before the patch it might have inserted null, whereas now it would insert ''. I don't think it makes a difference here though if it just gets output into a csv later.
    • [r-maint] ?
    • [r-test] PASS

@demeritcowboy
Copy link
Contributor

jenkins retest this please

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy thanks - & I'm glad you managed to figure out / replicate the original issue - we might be able to resolve it now

@seamuslee001 seamuslee001 merged commit f97587a into civicrm:master May 20, 2021
@seamuslee001 seamuslee001 deleted the import branch May 20, 2021 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants