Skip to content

Conversation

@pmjones
Copy link
Contributor

@pmjones pmjones commented Dec 30, 2020

Hey @dbraff it turns out there was a BC break in #12 . Specifically, it used to be that PDO::prepare(), when called directly, would return a LoggedStatement; under #12 it does not. (The removal of the call to set the statement class attribute was the problem.)

This PR puts that functionality back in place, honoring the connection persistence, and makes the relevant necessary modifications to LoggedStatement.

Further, ...func_get_args() won't honor argument references such as &$variable; this PR makes some changes so that they are honored, at the cost of more repetition.

Please try out this branch @dbraff and tell me if it continues to suit your needs.

@dbraff
Copy link
Contributor

dbraff commented Dec 30, 2020

Hey @dbraff it turns out there was a BC break in #12 . Specifically, it used to be that PDO::prepare(), when called directly, would return a LoggedStatement; under #12 it does not. (The removal of the call to set the statement class attribute was the problem.)

This PR puts that functionality back in place, honoring the connection persistence, and makes the relevant necessary modifications to LoggedStatement.

Further, ...func_get_args() won't honor argument references such as &$variable; this PR makes some changes so that they are honored, at the cost of more repetition.

Please try out this branch @dbraff and tell me if it continues to suit your needs.

Hey @pmjones, this all makes sense and I've checked out the branch and tested that the changes will work for our needs. Apologies for not realizing this would cause a BC break and thank you for resolving it!

@pmjones
Copy link
Contributor Author

pmjones commented Dec 30, 2020

Apologies for not realizing

No problem -- that one was a little subtle.

@pmjones pmjones merged commit 24125f9 into 1.x Dec 30, 2020
@pmjones pmjones deleted the persistent branch December 30, 2020 19:09
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.

3 participants