# ArrayAccess::offsetGet not returning a ref - Thread: https://externals.io/message/48105 - Original archive: https://news-web.php.net/php.internals/48105 - Started by Etienne Kneuss on 2010-04-27T08:17:42+00:00 - Messages: 12 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #48105 — Etienne Kneuss — 2010-04-27T08:17:42+00:00 URL: https://externals.io/message/48105#48105 Hi, it looks like the fact that ArrayAccess::offsetGet is not returning a reference is a recurrent problem, I see basically 4 options: a) Ignore the issue, change nothing b) Rewrite offsetGet to return a ref, breaking BC c) Create a new ArrayAccess interface where it does return a ref d) Relax prototype checks so that both are allowed. Personally, I believe that a fatal on such prototypes error is not warranted, so I'd prefer (d). What do you think would be the best option? Can you think of another? Best, -- Etienne Kneuss --- ## #48106 — Peter Cowburn — 2010-04-27T08:51:52+00:00 URL: https://externals.io/message/48105#48106 · In reply to: #48105 > Hi, > > it looks like the fact that ArrayAccess::offsetGet is not returning a > reference is a recurrent problem, I see basically 4 options: > > a) Ignore the issue, change nothing > > b) Rewrite offsetGet to return a ref, breaking BC -1,000,000 > > c) Create a new ArrayAccess interface where it does return a ref > > d) Relax prototype checks so that both are allowed. Of the options presented, I think d) would be the best of the bunch. Folks seem to expect the ability to get references so any solution that gives them that would be better IMO than keeping the interface as-is. > > Personally, I believe that a fatal on such prototypes error is not > warranted, so I'd prefer (d). > > What do you think would be the best option? Can you think of another? > > Best, > > -- > Etienne Kneuss --- ## #48107 — Daniel Egeberg — 2010-04-27T09:13:40+00:00 URL: https://externals.io/message/48105#48107 · In reply to: #48106 > > > […] > -1,000,000 > > […] > > Of the options presented, I think d) would be the best of the bunch. > Folks seem to expect the ability to get references so any solution > that gives them that would be better IMO than keeping the interface > as-is. Without having the technical insight in how it works internally, I would also say that from an end-user perspective, option (d) makes most sense. > > […] -- Daniel Egeberg --- ## #48116 — johannes@schlueters.de — 2010-04-27T16:45:38+00:00 URL: https://externals.io/message/48105#48116 · In reply to: #48105 Hi, > it looks like the fact that ArrayAccess::offsetGet is not returning a > reference is a recurrent problem, I see basically 4 options: The main use case is some nested structure like $o = new ArrayObject(); /*...*/ $o[23][42] = "foobar"; right? > a) Ignore the issue, change nothing > > b) Rewrite offsetGet to return a ref, breaking BC > > c) Create a new ArrayAccess interface where it does return a ref > > d) Relax prototype checks so that both are allowed. If the above case is correct and due to me not liking references I wonder whether there is a way to for an option e) which adds support for this in some way to the engine. johannes --- ## #49205 — Ralph Schindler — 2010-08-04T16:57:59+00:00 URL: https://externals.io/message/48105#49205 · In reply to: #48116 Hey all, Was there any more discussion on this? I've been bit by this as well, several times over the past 3 years. I'd opt for option (d) for all prototype/signature checking. Here's why: I don't think the return "type" should be considered part of the signature when enforcing LSP. PHP (currently), does not care what the type of the return value is, much less if there is even one to be returned. Whether or not it is a reference or a value should not impact the conditions presented to the caller, in other words "the preconditions are not strengthened in a subtype." Since PHP does no enforcement of return values, postconditions don't seem to apply. Also, relaxing the checks would be backwards compatible. Since no code should currently exist that does this (its an E_FATAL currently if the signature originates in an interface, `E_STRICT` notice when the signature originates in an abstract/base class overridden in a subclass). Thoughts? Ralph Schindler Johannes Schlüter wrote: > Hi, > > > > […] > > The main use case is some nested structure like > > $o = new ArrayObject(); > /*...*/ > $o[23][42] = "foobar"; > > right? > > > […] > > If the above case is correct and due to me not liking references I > wonder whether there is a way to for an option e) which adds support for > this in some way to the engine. > > johannes --- ## #49208 — Stas Malyshev — 2010-08-05T22:28:47+00:00 URL: https://externals.io/message/48105#49208 · In reply to: #49205 Hi! > I'd opt for option (d) for all prototype/signature checking. Here's why: I think relaxing the check may make sense. Do you have some code example that doesn't work and you want it to work? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #49211 — Derick Rethans — 2010-08-06T11:48:38+00:00 URL: https://externals.io/message/48105#49211 · In reply to: #48105 > Hi, > > it looks like the fact that ArrayAccess::offsetGet is not returning a > reference is a recurrent problem, I see basically 4 options: > > a) Ignore the issue, change nothing > > b) Rewrite offsetGet to return a ref, breaking BC > > c) Create a new ArrayAccess interface where it does return a ref > > d) Relax prototype checks so that both are allowed. > > Personally, I believe that a fatal on such prototypes error is not > warranted, so I'd prefer (d). Can you give an example of this one? Derick -- http://derickrethans.nl | http://xdebug.org Like Xdebug? Consider a donation: http://xdebug.org/donate.php twitter: @derickr and @xdebug --- ## #49212 — Ralph Schindler — 2010-08-06T14:22:30+00:00 URL: https://externals.io/message/48105#49212 · In reply to: #49208 I can give 2 examples, one that triggers the problem, the other that is a real world issue: --- Simple: array(1,2,3)); public function & offsetGet($name) { $r = & $this->_data['foo']; return $r; } public function offsetSet($name, $value) {} public function offsetExists($name) {} public function offsetUnset($name) {} } $b = new Bar(); $b['foo'][3] = 4; // implies Bar::offsetGet() happens before assign running this produces: $ php -d error_reporting=32767 test-reference-in-arrayaccess.php PHP Fatal error: Declaration of SomeContainer::offsetGet() must be compatible with that of ArrayAccess::offsetGet() in path/to/test-reference-in-arrayaccess.php on line 3 -ralph Stas Malyshev wrote: > Hi! > > > […] > > I think relaxing the check may make sense. Do you have some code example > that doesn't work and you want it to work? --- ## #49216 — Ralph Schindler — 2010-08-06T14:50:33+00:00 URL: https://externals.io/message/48105#49216 · In reply to: #49212 The attached patch is the suggested fix. I made this against master on github. -ralph Ralph Schindler wrote: > I can give 2 examples, one that triggers the problem, the other that is > a real world issue: > > --- > > Simple: > > > interface I { > public function foo($name); > } > > class C implements I { > public function & foo($name) {} > } > > $c = new Bar(); > > > running this produces: > > $ php -d error_reporting=32767 test-reference-in-signature.php > PHP Fatal error: Declaration of C::foo() must be compatible with that > of I::foo() in path/to/test-reference-in-signature.php on line 8 > > ---- > > Real world issue with ArrayAccess: > > > > class SomeContainer implements ArrayAccess { > protected $_data = array('foo' => array(1,2,3)); > public function & offsetGet($name) { > $r = & $this->_data['foo']; > return $r; > } > public function offsetSet($name, $value) {} > public function offsetExists($name) {} > public function offsetUnset($name) {} > } > > $b = new Bar(); > $b['foo'][3] = 4; // implies Bar::offsetGet() happens before assign > > > running this produces: > > $ php -d error_reporting=32767 test-reference-in-arrayaccess.php > PHP Fatal error: Declaration of SomeContainer::offsetGet() must be > compatible with that of ArrayAccess::offsetGet() in > path/to/test-reference-in-arrayaccess.php on line 3 > > > -ralph > > Stas Malyshev wrote: > > […] --- ## #49218 — Gustavo Lopes — 2010-08-06T15:24:22+00:00 URL: https://externals.io/message/48105#49218 · In reply to: #49216 On Fri, 06 Aug 2010 15:50:33 +0100, Ralph Schindler wrote: > The attached patch is the suggested fix. I made this against master on > github. > In my opinion, it would make more sense, as was already suggested before, to require the return to be passed by reference only if the prototype specifies it should be passed by reference. This could be argued to be a form of return contravariance. As a bonus, it would be consistent with the "pass_rest_by_reference" flag check: if (proto->common.pass_rest_by_reference && !fe->common.pass_rest_by_reference) { return 0; } //if (fe->common.return_reference != proto->common.return_reference) { if (proto->common.return_reference && !fe->common.return_reference) { return 0; } -- Gustavo Lopes --- ## #49219 — Stas Malyshev — 2010-08-06T17:41:26+00:00 URL: https://externals.io/message/48105#49219 · In reply to: #49218 Hi! > In my opinion, it would make more sense, as was already suggested before, > to require the return to be passed by reference only if the prototype > specifies it should be passed by reference. This could be argued to be a > form of return contravariance. Yes, this makes sense. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #49221 — Gustavo Lopes — 2010-08-07T03:25:38+00:00 URL: https://externals.io/message/48105#49221 On Sat, 07 Aug 2010 03:55:25 +0100, Etienne Kneuss wrote: > > […] > Yes, you are right. To put it another way: a subclass should be able to * Relax the preconditions * Strengthen the post-conditions I actually noticed this shortly after sending the e-mail, but hoped no one else would :p > Actually, after rereading what you said and the proposed diff, I'm not > sure it makes sense. It should _not_ be consistent with arguments. > > A method subtype may relax the parent prototype by not requiring a > ref for an argument. This is fine and it's indeed contra-variant. Again, my mistake. The if before was indeed about arguments... > However, the return value of a method subtype should be able to return a > ref even if the parent prototype doesnt. However, if the parent requires > a ref, the child should too. Which is covariant. > I'm confused, this was what exactly what I suggested. The condition I suggested is this (proto is the parent, fe the child): if (proto->common.return_reference && !fe->common.return_reference) { return 0; } So it returns an error condition if the parent returns a reference and the child doesn't. It's the check on the arguments that's wrong in this sense. However, I think invariance should be required here. The thing is, passing an argument by reference is actually also a post-condition -- if we pass an argument by reference, we're going to do something with it. In fact, invariance is required for the non-"rest" part of the arguments: if (fe->common.arg_info[i].pass_by_reference != proto->common.arg_info[i].pass_by_reference) { Again, sorry for the confusion. -- Gustavo Lopes