Bug #62328 (implementing __toString and a cast to string fails) #157

Merged
merged 2 commits into from Aug 12, 2012

Conversation

Projects
None yet
4 participants
@lt
Contributor

lt commented Aug 10, 2012

https://bugs.php.net/bug.php?id=62328

When extending a built-in class that has a cast_object function, and specifying a __toString method, the __toString method is ignored.

I have added a check to see if the object implements a __toString magic method, and call it when it does, leaving the check for cast_object as the next thing to check.

No new tests are failing with this change.

zend_make_printable_zval choses cast_object over __toString
https://bugs.php.net/bug.php?id=62328

Added a check to see if the object implements a __toString magic
method. This should be called instead of the cast_object method of
built-in classes when defined.
@reeze

This comment has been minimized.

Show comment Hide comment
@reeze

reeze Aug 10, 2012

Contributor

Maybe one more test case for this :)

Contributor

reeze commented Aug 10, 2012

Maybe one more test case for this :)

@lt

This comment has been minimized.

Show comment Hide comment
@lt

lt Aug 10, 2012

Contributor

@reeze The specific test case used SimpleXmlElement. Is it OK to create a test using this and skip it if the module is disabled? Or should I look for a better example class that handles it's own casting?

Edit

SplFileInfo looks like a good candidate.

Contributor

lt commented Aug 10, 2012

@reeze The specific test case used SimpleXmlElement. Is it OK to create a test using this and skip it if the module is disabled? Or should I look for a better example class that handles it's own casting?

Edit

SplFileInfo looks like a good candidate.

@reeze

This comment has been minimized.

Show comment Hide comment
@reeze

reeze Aug 10, 2012

Contributor

maybe you could find a class from spl classes :)

Contributor

reeze commented Aug 10, 2012

maybe you could find a class from spl classes :)

@php-pulls php-pulls merged commit 222ab9d into php:master Aug 12, 2012

@laruence

This comment has been minimized.

Show comment Hide comment
@laruence

laruence Aug 12, 2012

Owner

sorry, I merged this for reviewing, but push to master by accident, anyway, I got a different fix here: 7b307fb thanks .

Owner

laruence commented Aug 12, 2012

sorry, I merged this for reviewing, but push to master by accident, anyway, I got a different fix here: 7b307fb thanks .

@lt

This comment has been minimized.

Show comment Hide comment
@lt

lt Aug 12, 2012

Contributor

Hi Laruence.

I didn't fully understand the impact of removing the lower block of code. I did the same as you originally, but in the end I left it in simply because I didn't understand what other cases it might cater for.

Please consider using SplFileInfo instead of SimpleXmlElement in the test for this. XML libraries may be disabled, but SPL will not.

Contributor

lt commented Aug 12, 2012

Hi Laruence.

I didn't fully understand the impact of removing the lower block of code. I did the same as you originally, but in the end I left it in simply because I didn't understand what other cases it might cater for.

Please consider using SplFileInfo instead of SimpleXmlElement in the test for this. XML libraries may be disabled, but SPL will not.

@laruence

This comment has been minimized.

Show comment Hide comment
@laruence

laruence Aug 13, 2012

Owner

leight, hmm, okey, I think this test can be merged into ext/spl folder too , will do it . thanks

Owner

laruence commented Aug 13, 2012

leight, hmm, okey, I think this test can be merged into ext/spl folder too , will do it . thanks

@laruence

This comment has been minimized.

Show comment Hide comment
@laruence

laruence Aug 13, 2012

Owner

test script merged e51acee thanks

Owner

laruence commented Aug 13, 2012

test script merged e51acee thanks

@lt lt deleted the lt:Bug-62328 branch Dec 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment