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

Return empty string instead of null in AbstractList::__toString #25

Merged
merged 2 commits into from Nov 9, 2014

Conversation

Avaq
Copy link
Contributor

@Avaq Avaq commented Oct 22, 2014

In PHP, __toString must return a string, or an error is raised.

In PHP, __toString *must* return a string, or an error is raised.
@harikt
Copy link
Member

harikt commented Oct 22, 2014

The build fails, I assume it can be returned null than empty string will help.

@harikt
Copy link
Member

harikt commented Oct 22, 2014

@Avaq
Copy link
Contributor Author

Avaq commented Oct 22, 2014

Ah there is a unit-test testing for specifically null. Very odd because when null is returned from __toString on my PHP version, I get:

Catchable fatal error: Method Aura\Html\Helper\Ul::__toString() must return a string value in ...

@Avaq
Copy link
Contributor Author

Avaq commented Oct 22, 2014

Shall I modify the unit-test or is this intended behaviour?

@harikt
Copy link
Member

harikt commented Oct 22, 2014

You should ping @pmjones then.

Hari K T

You can ring me : +91 9388 75 8821

http://harikt.com , https://github.com/auraphp ,
http://www.linkedin.com/in/harikt , http://www.xing.com/profile/Hari_KT

Skype : kthari85
Twitter : harikt

On Wed, Oct 22, 2014 at 8:45 PM, Aldwin Vlasblom notifications@github.com
wrote:

Shall I modify the unit-test or is this intended behaviour?


Reply to this email directly or view it on GitHub
#25 (comment).

@Avaq
Copy link
Contributor Author

Avaq commented Oct 22, 2014

The problem is as follows: When PHP uses the __toString() method to cast an object into a string, it forces the method to return a string, as demonstrated by this test. The unit-tests never test for this, because they always call $ol()->__toString() directly.

I will modify the unit-test accordingly and push to this branch.

@harikt
Copy link
Member

harikt commented Oct 22, 2014

@Avaq may be branch out from this and send a different PR .

@harikt
Copy link
Member

harikt commented Oct 23, 2014

I am ok with this. Assuming no one tried to compare the return of __toString to null as in unit tests ;-) .

@Avaq
Copy link
Contributor Author

Avaq commented Oct 23, 2014

People who expect the return value of __toString to equal null are asking for their code to break anyway. 😈

pmjones pushed a commit that referenced this pull request Nov 9, 2014
Return empty string instead of null in AbstractList::__toString
@pmjones pmjones merged commit fabe399 into auraphp:develop-2 Nov 9, 2014
@Avaq
Copy link
Contributor Author

Avaq commented Nov 9, 2014

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

3 participants