Page MenuHomePhabricator

Introduce "phutil_string_cast()" to work around interesting choices in "(string)" behavior
ClosedPublic

Authored by epriestley on Feb 11 2019, 6:32 PM.

Details

Summary

Ref T13250. Two issues here:

  • When you (string)$x, if __toString() raises an exception: you get a fatal error, not an exception.
  • When you (string)$x, if $x is an array, you get the string "Array". This is almost certainly never desirable.

We can't fix this in the general case (and sometime string casts will still happen implicitly and there's nothing we can do about it), but we can implement a better version of (string) at the application layer for cases where we're doing an explicit cast.

(I believe there's a legitimate-ish reason that __toString() can't always raise exceptions, like if you call strlen($obj) and it needs to be implicitly cast inside parameter construction for the call or something? So this behavior isn't completely pointless, probably, just not-so-great.)

Test Plan

Replaced the culprit (string) in T13250 with phutil_string_cast(), with exception improvements applied but no HTTP query string fixes.

  • Before:

  • After:

Diff Detail

Repository
rPHU libphutil
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Feb 11 2019, 6:32 PM
epriestley requested review of this revision.Feb 11 2019, 6:32 PM
amckinley accepted this revision.Feb 11 2019, 7:06 PM
amckinley added inline comments.
src/utils/utils.php
1603

FYI, having tested this, the old behavior is the case for 5.5 through 7.0. Fixed in 7.0.1.

1614–1615

I had to read this several times and test it myself to believe it. Wow.

And maybe "However, this is not strictly true: a __toString method that is called directly by application code (instead of by PHP itself during, for example, a cast to string) will throw an exception which can be caught and handled as usual.

1620

"Any value which aspires to be represented as a string"?

1627–1628

I wouldn't mind if this did return phutil_json_encode($value), personally. Invoking phlog on arrays always surprises me. And wrap the encode attempt in a try block so we don't mysteriously return a JSON-related exception instead of this^^.

1632

And we're ok with the underlying exception from __toString() bubbling up here, instead of maybe wrapping it?

1638

I suppose it would be kind of self-defeating to attempt to put whatever __toString's actual return value was in this exception text...

This revision is now accepted and ready to land.Feb 11 2019, 7:06 PM
epriestley added inline comments.Feb 11 2019, 7:28 PM
src/utils/utils.php
1627–1628

I think adding magic to phlog() (or anywhere else where we sometimes show strings to users) is super reasonable, but it feels way too magical here to me. An array() may also contain objects with their own __toString() methods and so on, so even if we wanted to say "(string) means JSON" I think the patch has to get way more complicated. I'll see if I can fix phlog() though (see also below).

1632

Yeah, I think that's desirable. I suspect this is always an error in the application logic, not, say, users doing something silly.

1638

Right. It would be sort of nice to have a phutil_try_to_safely_describe_type($variable) (and maybe we already do, I've just forgotten about it?). phlog() needs an Exception -> Throwable adjustment anyway, so maybe I can clean it up a little.

Do you have an example of phlog() misbehaving on arrays? I get tailored behavior in the simple case:

phlog(array(1, 2))

[Mon Feb 11 13:12:20.515204 2019] [php7:notice] [pid 38236] [client 127.0.0.1:59585] [2019-02-11 13:12:20] PHLOG: Array of size 2 starting with: { 0 => 1 } at [/Users/epriestley/dev/core/lib/phabricator/webroot/index.php:16]

Or is there a different behavior you expect/want?

If you do something like this:

phlog('array value = '.array(1, 2));

...you get nothing useful:

[Mon Feb 11 13:15:02.014713 2019] [php7:notice] [pid 28797] [client 127.0.0.1:59653] [2019-02-11 13:15:02] PHLOG: 'array value = Array' at [/Users/epriestley/dev/core/lib/phabricator/webroot/index.php:16]

...but we can't do much about that -- you have to wrap the array() in some printing method or use sprintf() or pht() or something, and at that point these alternatives seem equally good:

phlog('array value = '.json_encode(array(1, 2)));
phlog('array value = '.phutil_string_cast(array(1, 2)));

...with the advantage that json_encode() is explicit/nonmagical.

Do you have an example of phlog() misbehaving on arrays? I get tailored behavior in the simple case:

I think I'm mostly just expecting printf/pht semantics by default. I'll just start using json_encode and phutil_string_cast.

  • Clarify some comments.
This revision was automatically updated to reflect the committed changes.