Skip to content

Conversation

@Noneatme
Copy link
Contributor

No description provided.

@avbdr
Copy link
Collaborator

avbdr commented Jun 28, 2016

very nice patch. Thank you. Ill send me code comments a bit later for you to adjust and we are ready to land

@Noneatme
Copy link
Contributor Author

Alright! Thanks!

@avbdr
Copy link
Collaborator

avbdr commented Jul 8, 2016

Sorry, it took me longer :) Patch looks great except couple issues:

  1. default settings validation: You can create an array of default values and then do an array_merge with settings passed into the function.
  2. revert if(file_exists($importFile)) to an early return to streamline the code.
if (!file_exists) {
     throw exception
     return
}
        if($result) {
                $success = true;
            }
            // Something went wrong
            else {
                $success = false; 
            }

this is the same as return (bool)$result;

  1. rename loadXML -> loadXml for a naming consistency.

@avbdr
Copy link
Collaborator

avbdr commented Jul 8, 2016

as well i think it doesnt make sense to add escapeQuery parameter as most of escaped sql queries will fail. You need to escape variables, not SQL query itself.
So you can remove that.

@Noneatme
Copy link
Contributor Author

Noneatme commented Jul 8, 2016

I fixed the issues you you mentioned. Thanks!
But now, the tests fail somehow (data doesn't get inserted). I have to take a deeper look. Don't merge right now, I will push a commit later when I found the issue.

@Noneatme
Copy link
Contributor Author

Noneatme commented Jul 8, 2016

It should work now. The .csv test file was not in dos mode so the \r\n selector failed.

@avbdr avbdr merged commit 0f0ca57 into ThingEngineer:master Jul 8, 2016
@avbdr
Copy link
Collaborator

avbdr commented Jul 8, 2016

thank you, merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants