facebook / hhvm

A virtual machine for executing programs written in Hack.
https://hhvm.com
Other
18.18k stars 2.99k forks source link

PDOStatement may not implement Iterator #3536

Open SiebelsTim opened 10 years ago

SiebelsTim commented 10 years ago

I hope to reach a bigger audience with this. I need someones opinion.

Problem

Currently PDOStatement implements Iterator which differs from PHP. Inheriting from PDOStatement and implementing IteratorAggregate leads to undefined behaviour in HHVM.

Reproduction

// What is supposed to traversed over? The Iterator or the Aggregate
// PHP fails on this, hhvm doesn't.
class test implements Iterator, IteratorAggregate {}

http://3v4l.org/3J7pt

// This is totally valid in PHP to have a PDOStmt that can be iterated over
// the way the user wants to. 
// HHVM implements both Iterator and IteratorAggregate => Undefined
// (HHVM checks first for Iterator, than IteratorAggregate so Iterator's behaviour will win)
class PDOStmt extends PDOStatement implements IteratorAggregate {
  public function getIterator() {
    return new ArrayIterator([1,2,3]);
  }
}

foreach(new PDOStmt() as $stmt) { // I could do  (new PDOStmt())->getIterator() to resolve this
  echo $stmt;
}

http://3v4l.org/A2ifd

PDOStatement implements Iterator so you can do the following

foreach($pdo->query("SELECT 3;") as $r) {
  echo $r[0];
}

If it implements Traversable like it should it'll only output the member variables (the query itself).

Reason

(Thank you @auroraeosrose) This is due to the design difference in hhvm and PHP. PHP has two layers. The userland layer and the C layer. Both can have the magic hooks without overwriting them. The Traversable interface only exists to describes an object than can be iterated over, but doesn't implement iterator (thank you php). Only for PDO. HHVM can't have it be Traversable and still iterate it over like an Iterator

Solution?

Do we need to special case this? Do we want to live with this difference? I think we should fatal on having both IteratorAggregate and Iterator implemented like php does.

\cc: @auroraeosrose @Ocramius @macnibblet Relevant: https://github.com/Ocramius/crate-pdo/commit/105677234aea9703558955d175156961aa4a3fdb

auroraeosrose commented 10 years ago

This is a major design difference between hhvm and PHP and objects - the concept of "two layers" of object handlers - one accessible by userland, and one accessible only by extensions and not override-able by userland

There are going to be other "magic features" that can (and are) implemented by PHP extensions that hook into the C/C++ level - non-user-reachable, non-override-able hooks for PHP objects (most relevant is a ustring class that is being pitched for PHP 7)

Although some of the hooks will make no sense for hhvm ever and can be ignored - we should really take a look and at least implement the ones that do make sense

http://lxr.php.net/xref/PHP_5_6/Zend/zend_object_handlers.h#119

Here is the "internal" iterator implemented by PDO http://lxr.php.net/xref/PHP_5_6/ext/pdo/pdo_stmt.c#2527 hooked here into "iterator_get" object handler

http://lxr.php.net/xref/PHP_5_6/ext/pdo/pdo_stmt.c#2338

paulbiss commented 10 years ago

I'd be in favor of adding the fatal, I'm not sure how to solve the broader problem though.

cc @AlphaStream who's working on PDO.