[Form] made ValueToStringTransformer convert Boolean false to empty string #4180
Conversation
|
I don't agree, false indicates something failed in a way or another, but the common "lack of a value" is null. That's what properties default to when you just do |
|
I understand that point of view, but do we have a clear set of standards for what PHP should return to represent the lack of a value? The reason for the PR is that there doesn't seem to be one - and indeed, for good or ill, the standard library hasn't been fussy. The code here is using values that are normally inferred from reflection of an entity class - which can be completely de-coupled, and therefore any userland class. In the case of a getter method that returns false because there is nothing to return, to me that does indicate a kind of failure (and, in the case of the form component, getters are called before properties are accessed). I'm not saying one should use Re: the exception - I agree about improvement to the message, and will have a look at that. |
|
Well, I guess we'll need @bschussek to answer about the real intent behind this code, and why it's like that. That said, IMO a getter returning false because there is no value is broken, and it should be fixed. False is a value, it's a boolean, the opposite of true, a negative value of sorts, but it's not nothing. Old code written that way typically will work no matter if it's false or null, so why not fix it? |
|
As I say, I understand that thinking, and don't disagree! (Using The argument behind the PR still stands, because it's common idiomatic usage that is even used by the standard library, even if it can be argued as being misuse. |
|
Fair enough, but I suppose if it's like this there is a reason, in which case all we can do is improve the exception message to describe what's happening and how to fix it best. Anyway let's wait for a word from @bschussek :) |
|
This is invalid anyway as ValueToStringTransformer should be reverted (removed) again. See #4102 for more information. |
|
Thanks for the clarification. |
|
@bschussek I'd love to contribute, so have bookmarked that one and intend to put a PR together when I have a moment! |
It's common in PHP for Boolean
falseto represent the lack of a value (e.g. core functions likestrpos()) - therefore the code here should intuit this intention to represent the lack of something withfalseand convert it to an empty string the same way it does fornull, rather than throwing a rather opaque exception.