Zend Framework

Zend_Db_Table_Row_Abstract::__isset() returns true for null values

Details

  • Type: Bug Bug
  • Status: Postponed Postponed
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: Next Major Release
  • Component/s: Zend_Db_Table
  • Labels:
    None
  • Fix Version Priority:
    Should Have

Description

Zend_Db_Table_Row_Abstract::__isset() uses array_key_exists rather than isset, which results in null values returning true. Quote from Bill Karwin on the mailing list:

The way it works now makes it a little more like property_exists().
That is, if the Row object has a property, even if its value is null,
then the method returns true. This is useful information, and should be
supported.

But I can understand that supporting this with the magic __isset()
method might not be the best solution, since isset() has pretty well
defined semantics, including returning false if the property has a null
value (I have issues with this behavior of isset(), but the fact is that
it does work that way).

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Wil Sinclair added a comment -

Please categorize/fix as needed.

Show
Wil Sinclair added a comment - Please categorize/fix as needed.
Hide
Permalink
Darby Felton added a comment -

If it is needed to know whether a property exists and is it is not of a null value, users simply can test against this, as follows:

if (isset($row->property) && $row->property !== null) { ... }

Perhaps another solution would be to augment the row abstract class with a method implementing this feature for simplicity's sake, since it is likely a common use case.

Other ideas for resolution?

Show
Darby Felton added a comment - If it is needed to know whether a property exists and is it is not of a null value, users simply can test against this, as follows: 
if (isset($row->property) && $row->property !== null) { ... }
Perhaps another solution would be to augment the row abstract class with a method implementing this feature for simplicity's sake, since it is likely a common use case. Other ideas for resolution?
Hide
Permalink
Jack Sleight added a comment -

I think the only resolution should be changing the current __isset() mefthod to return false when the value is null i.e. use isset() rather than array_key_exists(). As Bill said, isset() has very well defined semantics, it is expected to return false when the value is null. Zend_Db_Table_Row_Abstract shouldn't be any different.

If the current functionality is still required then that should be in a new, separate method.

Show
Jack Sleight added a comment - I think the only resolution should be changing the current __isset() mefthod to return false when the value is null i.e. use isset() rather than array_key_exists(). As Bill said, isset() has very well defined semantics, it is expected to return false when the value is null. Zend_Db_Table_Row_Abstract shouldn't be any different. If the current functionality is still required then that should be in a new, separate method.
Hide
Permalink
Darby Felton added a comment -

I'm not sure that breaking backward compatibility is worth the change.

Show
Darby Felton added a comment - I'm not sure that breaking backward compatibility is worth the change.
Hide
Permalink
Jack Sleight added a comment -

Hmm, I dunno. I cant see that anyone would be directly calling __isset(), it would of course be being called when people use the actual isset() PHP function on the object, and everyone knows that isset() returns false when the value's null. I don't see it being a major backward compatibility issue.

Although personally, I'd change it even if it did significantly break backward compatibility, simply because at present, it makes the isset() PHP function behave differently from the documented behaviour. It is essentially broken in its current state.

Just my two cents.

Show
Jack Sleight added a comment - Hmm, I dunno. I cant see that anyone would be directly calling __isset(), it would of course be being called when people use the actual isset() PHP function on the object, and everyone knows that isset() returns false when the value's null. I don't see it being a major backward compatibility issue. Although personally, I'd change it even if it did significantly break backward compatibility, simply because at present, it makes the isset() PHP function behave differently from the documented behaviour. It is essentially broken in its current state. Just my two cents.
Hide
Permalink
Darby Felton added a comment -

For what it's worth, I also agree that the change should be made. That is, I believe that __isset() should be changed to be consistent with PHP's isset() function. Another method could be added, like propertyExists($name), that would provide the ability to return true if and only if a property exists, even if it is null (the existing behavior of __isset()).

That said, I'm afraid that breaking backward compatibility is not possible for us before 2.0.0, so I mark this issue as to fix for next major release.

Show
Darby Felton added a comment - For what it's worth, I also agree that the change should be made. That is, I believe that __isset() should be changed to be consistent with PHP's isset() function. Another method could be added, like propertyExists($name), that would provide the ability to return true if and only if a property exists, even if it is null (the existing behavior of __isset()). That said, I'm afraid that breaking backward compatibility is not possible for us before 2.0.0, so I mark this issue as to fix for next major release.
Hide
Permalink
Jaka Jancar added a comment -

+1 for changing the behavior in the next major version. I just lost half an hour debugging a problem caused by this :/

While theoretically I think the current behavior is more logical and find it silly that $a = NULL; isset($a) returna false, ZF should be consistent with PHP's silliness.

Show
Jaka Jancar added a comment - +1 for changing the behavior in the next major version. I just lost half an hour debugging a problem caused by this :/ While theoretically I think the current behavior is more logical and find it silly that $a = NULL; isset($a) returna false, ZF should be consistent with PHP's silliness.
Hide
Permalink
Wil Sinclair added a comment -

No action on this issue for too long. I'm reassigning to Ralph for re-evaluation and categorization.

Show
Wil Sinclair added a comment - No action on this issue for too long. I'm reassigning to Ralph for re-evaluation and categorization.
Hide
Permalink
Ralph Schindler added a comment -

Will evaluate within 2 weeks

Show
Ralph Schindler added a comment - Will evaluate within 2 weeks
Hide
Permalink
Kirk Madera added a comment -

Strongly agreed that isset() should follow php behavior. This caused me to question my basic php knowledge for a sec until I realized what was going on. Also agreed that it could be added into the next major release to avoid backward compatibility problems.

Show
Kirk Madera added a comment - Strongly agreed that isset() should follow php behavior. This caused me to question my basic php knowledge for a sec until I realized what was going on. Also agreed that it could be added into the next major release to avoid backward compatibility problems.
Hide
Permalink
Ralph Schindler added a comment -

Given there is a massive use of isset($row->field) and an already expected behavior, we are gonna have to push this off till the next major release.

Show
Ralph Schindler added a comment - Given there is a massive use of isset($row->field) and an already expected behavior, we are gonna have to push this off till the next major release.

People

Vote (2)
Watch (3)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Zend Framework. Try JIRA - bug tracking software for your team.