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

Avoid failing lookups for empty array values (bis) #98

Merged
merged 2 commits into from Oct 9, 2012

Conversation

bakura10
Copy link
Member

@bakura10 bakura10 commented Oct 9, 2012

This is a corrected version of #97

@Ocramius
Copy link
Member

Ocramius commented Oct 9, 2012

@bakura10 hmm, still not really good since '' could be an identifier. Nevermind for now I guess.

@bakura10
Copy link
Member Author

bakura10 commented Oct 9, 2012

How can an empty string be a primary key ? Anyway, David use case have much higher chance than having an empty string as pk...

Envoyé de mon iPhone

Le 9 oct. 2012 à 19:53, Marco Pivetta notifications@github.com a écrit :

@bakura10 hmm, still not really good since '' could be an identifier. Nevermind for now I guess.


Reply to this email directly or view it on GitHub.

@Ocramius
Copy link
Member

Ocramius commented Oct 9, 2012

@bakura10 the fact is that there's an abyss between false, '', 0 and null, heh... (and '' as PK is actually possible :P ).

It is just a matter of being correct, but we have to deal with this data right now.
Maybe should note it in a comment :|

@bakura10
Copy link
Member Author

bakura10 commented Oct 9, 2012

But this is problematic. We just CANNOT don't hydrate a value because it is empty (just think of a field that you update by removing the value, you still have to set an empty value).

So I think this fix covers the most usual case.

@Ocramius
Copy link
Member

Ocramius commented Oct 9, 2012

Yep, still annoying :( Can you please add a comment there? After that I'll merge.

@davidwindell thank you for this catch!

@bakura10
Copy link
Member Author

bakura10 commented Oct 9, 2012

Done. I have also updated the doc with a longer comment.

@davidwindell
Copy link
Contributor

Chaps, no problem! We've got a rather large API running so there's a heck of a lot of hydration going on bringing things like this to note.

Going forward, would either of you be opposed to a PR that updates the Hydrator to implement the StrategyEnabledInterface so that we can apply custom Hydration strategies before rather than after the Doctrine Hydrator has passed the data.

NB At the moment we apply our Hydration Strategies to the Class Methods hydrator attached so they occur after the case

@bakura10
Copy link
Member Author

bakura10 commented Oct 9, 2012

I completely follow you for this ! I wanted to do it a few weeks ago but completely forgotten ! This should be a great update and should not be a BC.

@bakura10
Copy link
Member Author

bakura10 commented Oct 9, 2012

This hydrator is becoming really good, I'm quite happy with the work we did with him ! Thanks everyone ;-) !

Ocramius added a commit that referenced this pull request Oct 9, 2012
Avoid failing lookups for empty array values (bis)
@Ocramius Ocramius merged commit 7397099 into doctrine:master Oct 9, 2012
@Ocramius
Copy link
Member

Ocramius commented Oct 9, 2012

Aaaaaand, it's done :)

@davidwindell
Copy link
Contributor

:) working really well, thank you @bakura10.

I'll raise a PR for StrategyEnabledInterface

@davidwindell
Copy link
Contributor

@bakura10 @Ocramius #101

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1e0b4f6 on bakura10:hotfix97 into * on doctrine:master*.

gencer pushed a commit to gencer/DoctrineModule that referenced this pull request Apr 11, 2014
Fixed cache drivers that can be better configurable.
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.

None yet

4 participants