Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Revert PR #224 due to issue like #288 #289

Merged
merged 2 commits into from
Dec 7, 2017

Conversation

ezimuel
Copy link
Contributor

@ezimuel ezimuel commented Dec 7, 2017

This PR revert #224 due to issue like #288.

Summary: I tried to solve #35 with #224 but I didn't cover all the use cases, for instance #288. After reading also the php-cvs 46848 and 47302, mentioned in PHP BUG 43130, I decided to revert #224 and mark #35 as won't fix. It's not considered a best practice to use these characters in bind param name.

@weierophinney
Copy link
Member

I guess the question I have is: do we want to safeguard against invalid parameter names when using PDO? #224 provided a fix for that, clearly, but as you also note, a number of other bugs indicate that most recommend against using invalid characters in the first place.

Perhaps we could do a check for invalid characters (basically, anything outside the accepted character set) and raise an exception?

@michalbundyra
Copy link
Member

@weierophinney

Perhaps we could do a check for invalid characters (basically, anything outside the accepted character set) and raise an exception?

That's actually a good idea.

@ezimuel
I'm not sure if this PR completely reverting #224, I can see here "Files changed 4 (+8 −37)", but in #224 we have "Files changed 6 (+67 −5)"

@ezimuel
Copy link
Contributor Author

ezimuel commented Dec 7, 2017

@weierophinney throwing exception is a good idea, I'll work on it.

@webimpress yes, it reverts #224 completely, I checked again. #224 contains multiple changes on the same code base.

@ezimuel
Copy link
Contributor Author

ezimuel commented Dec 7, 2017

@weierophinney, @webimpress I added the Exception for invalid param name.

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

Perfect! I'll merge shortly and issue the 2.9.1 release.

@ezimuel
Copy link
Contributor Author

ezimuel commented Dec 7, 2017

@weierophinney thanks!

@weierophinney weierophinney merged commit 43104a8 into zendframework:master Dec 7, 2017
weierophinney added a commit that referenced this pull request Dec 7, 2017
weierophinney added a commit that referenced this pull request Dec 7, 2017
weierophinney added a commit that referenced this pull request Dec 7, 2017
weierophinney added a commit that referenced this pull request Dec 7, 2017
Forward port #289

Conflicts:
	CHANGELOG.md
@froschdesign froschdesign added this to the 2.9.1 milestone Dec 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants