Re: [PHP-DEV] Stop Exceptions capturing object references for tracearguments

This is only part of a thread. view whole thread
  100193
August 12, 2017 14:37 rowan.collins@gmail.com (Rowan Collins)
On 12/08/2017 04:25, Stanislav Malyshev wrote:
> Hi! > >> The "args" part of this contains full object references to anything that >> happens to have been a function argument in the stack, and causes two >> problems: > I think it makes sense to make exception not to collect args. In fact, I > think this may also be one of rare cases where new ini setting would be > appropriate, where the default could be the old way (at least for now, > for BC), and recommended production setting would be off. >
My only concern is if anyone is using this data for anything other than debugging - e.g. if they were somehow extracting extra context about the exception by traversing the backtrace to a particular point and grabbing the arguments. That feels like a horrible hack to me, but that doesn't mean someone isn't relying on it. An interesting side effect of an ini setting, though, is that it could in turn be made to raise deprecation warnings at some point, whereas just accessing the 'args' element of an array is pretty hard to detect. So for instance, we could theoretically have: 7.3, add ini setting defaulting to on; 7.4, change default to off and raise deprecation notice if it's turned on; 8.0, remove feature, error if ini setting is not set to on (ignore if set to off for smoother upgrades). Regards, -- Rowan Collins [IMSoP]
  100194
August 12, 2017 15:54 smalyshev@gmail.com (Stanislav Malyshev)
Hi!

> My only concern is if anyone is using this data for anything other than > debugging - e.g. if they were somehow extracting extra context about the > exception by traversing the backtrace to a particular point and grabbing > the arguments. That feels like a horrible hack to me, but that doesn't > mean someone isn't relying on it.
Sure, that's why I propose to keep the option. If you use the horrible hack, I think it wouldn't be too much to ask to set an INI variable. Since it's rather local, it can be INI_ALL so you could do it in the script even.
> An interesting side effect of an ini setting, though, is that it could > in turn be made to raise deprecation warnings at some point, whereas > just accessing the 'args' element of an array is pretty hard to detect. > So for instance, we could theoretically have: 7.3, add ini setting > defaulting to on; 7.4, change default to off and raise deprecation > notice if it's turned on; 8.0, remove feature, error if ini setting is > not set to on (ignore if set to off for smoother upgrades).
Yep, sounds like a plan. Not sure if we need to drop it for 8.0 even, but we can vote on that. Anybody to make an RFC for this? :) -- Stas Malyshev smalyshev@gmail.com
  100198
August 13, 2017 14:31 me@kelunik.com (Niklas Keller)
2017-08-12 17:54 GMT+02:00 Stanislav Malyshev <smalyshev@gmail.com>:

> Hi! > > > My only concern is if anyone is using this data for anything other than > > debugging - e.g. if they were somehow extracting extra context about the > > exception by traversing the backtrace to a particular point and grabbing > > the arguments. That feels like a horrible hack to me, but that doesn't > > mean someone isn't relying on it. > > Sure, that's why I propose to keep the option. If you use the horrible > hack, I think it wouldn't be too much to ask to set an INI variable. > Since it's rather local, it can be INI_ALL so you could do it in the > script even. > > > An interesting side effect of an ini setting, though, is that it could > > in turn be made to raise deprecation warnings at some point, whereas > > just accessing the 'args' element of an array is pretty hard to detect. > > So for instance, we could theoretically have: 7.3, add ini setting > > defaulting to on; 7.4, change default to off and raise deprecation > > notice if it's turned on; 8.0, remove feature, error if ini setting is > > not set to on (ignore if set to off for smoother upgrades). > > Yep, sounds like a plan. Not sure if we need to drop it for 8.0 even, > but we can vote on that. Anybody to make an RFC for this? :) >
If it's explicitly needed, someone could still just call `debug_backtrace()` and manually store the args in the exception constructor, no? Would the args still be available in the stringified version? Because they're quite useful for debugging there. Regards, Niklas
  100207
August 13, 2017 20:44 rowan.collins@gmail.com (Rowan Collins)
On 13/08/2017 15:31, Niklas Keller wrote:
> > If it's explicitly needed, someone could still just call > `debug_backtrace()` and manually store the args in the exception > constructor, no?
That depends exactly how it's being used, but yes if you control the throw site or Exception constructor you could grab the backtrace manually.
> > Would the args still be available in the stringified version? Because > they're quite useful for debugging there.
Hm, I guess we'd need to store the string representation (the class name, basically) somewhere so we could keep that. Doesn't seem like it ought to be too hard, unless there's some hidden complexity or performance impact there. Regards, -- Rowan Collins [IMSoP]
  100352
September 2, 2017 11:52 me@kelunik.com (Niklas Keller)
> > Would the args still be available in the stringified version? Because >> they're quite useful for debugging there. >> > > Hm, I guess we'd need to store the string representation (the class name, > basically) somewhere so we could keep that. Doesn't seem like it ought to > be too hard, unless there's some hidden complexity or performance impact > there.
Not sure whether there will be a noticeable performance impact. Formatting doesn't have to take place on collection, but a replacement of object references to some representation that can later be formatted. A refcount increase can be saved, but that's probably totally trivial performance wise. Related issue for ReactPHP's promises: https://github.com/reactphp/promise/issues/46#issuecomment-326739299 Regards, Niklas