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

Statement extends \PDOStatement implements \IteratorAggregate #6

Merged
merged 3 commits into from
Jul 25, 2015
Merged

Statement extends \PDOStatement implements \IteratorAggregate #6

merged 3 commits into from
Jul 25, 2015

Conversation

adaamz
Copy link
Contributor

@adaamz adaamz commented Jul 14, 2015

Make it more like PDO+PDOStatement usable.

@taq
Copy link
Owner

taq commented Jul 14, 2015

Argh, I'm on a client now and just missed the point of your pull request and made some lame comments, sorry. Now I see you're extending PDOStatement, nice! I'll take a look closer on it later. :-)

@taq
Copy link
Owner

taq commented Jul 15, 2015

Running the tests, we missed the error code returned by an invalid statement. I'll take a look on it.

@taq
Copy link
Owner

taq commented Jul 15, 2015

Seems the queryString on Statement, when extending PDOStatement, gives an error because is then read-only. If we extend, we need to deal with the PDOStatement constructor, to need to check if all the current behaviours are ok.

@adaamz
Copy link
Contributor Author

adaamz commented Jul 15, 2015

We can remove line 55. querystring is used nowhere.

$querystring is read-only in \PDOStatement
@taq
Copy link
Owner

taq commented Jul 15, 2015

Yeah, I removed it here and it worked. Can you run the test suite without it and see if there is no other errors about extending the class?

@adaamz
Copy link
Contributor Author

adaamz commented Jul 23, 2015

Sorry I don't have access to server with Oracle database on which I can test it :(
But I use this class on website which runs well :)

@taq
Copy link
Owner

taq commented Jul 23, 2015

Np, so I'll take a look on it, I have a server for testing.
Em 23/07/2015 10:40, "Adam Žurek" notifications@github.com escreveu:

Sorry I don't have access to server with Oracle database on which I can
test it :(
But I use this class on website which runs well :)


Reply to this email directly or view it on GitHub
#6 (comment).

@taq taq merged commit a20edcb into taq:master Jul 25, 2015
@taq
Copy link
Owner

taq commented Jul 25, 2015

Just run the tests here, everything seems ok. I made some tweaks and upgraded the Composer version. thank you! :-)

@adaamz
Copy link
Contributor Author

adaamz commented Jul 25, 2015

Thanks, I will update composer :-)

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