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

[5.6] Implement Optional::__isset #24042

Merged
merged 2 commits into from Apr 29, 2018
Merged

[5.6] Implement Optional::__isset #24042

merged 2 commits into from Apr 29, 2018

Conversation

ghost
Copy link

@ghost ghost commented Apr 28, 2018

This pull request implemented __isset method on Optional class.
See #24031 for more detail.

@ghost ghost changed the title Implement Optional::__isset [5.6] Implement Optional::__isset Apr 28, 2018
@BrandonShar
Copy link
Contributor

I don't understand the example code from your issue. the argument to empty is going to be either a string or null, not an optional. Where is isset being called on the optional?

@ghost
Copy link
Author

ghost commented Apr 28, 2018

Because in my example code, the cellphone property is dynamically access from an Optional instace.
The empty structure will do an isset check first, then chek its value.

Check on PHP's document here:

Note:

When using empty() on inaccessible object properties, the __isset() overloading method will be called, if declared.

@BrandonSurowiec
Copy link
Contributor

You can clean up the assertions by using these methods:

$this->assertTrue($value);
$this->assertFalse($value);
$this->assertNull($value);

@ghost
Copy link
Author

ghost commented Apr 29, 2018

@BrandonShar Thank you for your advice! I have updated them!

@taylorotwell taylorotwell merged commit b4fdee2 into laravel:5.6 Apr 29, 2018
/**
* Dynamically check a property exists on the underlying object.
*
* @param $name
Copy link
Member

Choose a reason for hiding this comment

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

invalid syntax

Copy link
Author

Choose a reason for hiding this comment

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

This is auto generated by PhpStorm, I didn't notice that. Since it has been merged, should I open another PR?

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.

5 participants