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

Check if property is set #3445

Merged
merged 1 commit into from
May 31, 2014
Merged

Check if property is set #3445

merged 1 commit into from
May 31, 2014

Conversation

alex-code
Copy link
Contributor

getValue didn't check if the property on an object exists so wouldn't return the default value.

```getValue``` didn't check if the property on an object exists so wouldn't return the default value.
@cebe cebe added this to the 2.0 RC milestone May 12, 2014
samdark added a commit that referenced this pull request May 31, 2014
@samdark samdark merged commit a5846fa into yiisoft:master May 31, 2014
@samdark
Copy link
Member

samdark commented May 31, 2014

Merged. Thank you.

@cebe
Copy link
Member

cebe commented May 31, 2014

okay, but if we want the null value instead of the default value? This behavior is now different from array.

@samdark
Copy link
Member

samdark commented May 31, 2014

@cebe we have 3 variants:

  1. Change it to property_exists. That will match behavior to array one but has issues: it's slow and may not work with magic properties.
  2. Leave as it's now. Nulls will be replaced with default value.
  3. Remove check. It will trigger error instead of using default value.

@qiangxue
Copy link
Member

IMO, 1 is out of question. 2 and 3 both have points, but I prefer 3 for the following reasons:

  • trying to get an undefined property of an object is a programming error. It's better to trigger the error than returning a default value.
  • it's a bit faster than 2 because the isset() check will cause evaluation of the property.

@cebe
Copy link
Member

cebe commented May 31, 2014

I'd vote for reverting the change too.

samdark added a commit that referenced this pull request May 31, 2014
This reverts commit a5846fa, reversing
changes made to 2a6e064.
@samdark
Copy link
Member

samdark commented May 31, 2014

Reverted 44273f0

@alex-code
Copy link
Contributor Author

That' a shame, It was useful for relations.
ArrayHelper::getValue($model, 'relation1.relation2.value'), $default)

Maybe a note should be added to the docs to say if it's an object $default doesn't apply.

tvdavid pushed a commit to tvdavid/yii2 that referenced this pull request Jul 24, 2014
tvdavid pushed a commit to tvdavid/yii2 that referenced this pull request Jul 24, 2014
This reverts commit a5846fa, reversing
changes made to 2a6e064.
@alex-code alex-code deleted the getValue branch July 28, 2014 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants