I was just talking to klausi and Berdir, and none of us could figure out why \Drupal\Core\Database\Query\SelectInterface doesn't specify a execute() method. It makes all executions of select statements (when not using concrete classes as type hints) report warnings in IDEs.
The problem is also present in Drupal 7.

So, does anyone know why this was omitted? It doesn't seem likely that this was done by accident, but on the other hand I really have no explanation.

Comments

drunken monkey’s picture

Status: Active » Needs review
StatusFileSize
new779 bytes

So, here would be a patch to add that method. (If the test bot complains, at least we might get an idea what's wrong with doing this / why it wasn't done before.)

drunken monkey’s picture

Issue tags: +Stalking Crell

Seems this change is perfectly acceptable, at least as far as the test bot is concerned. So, does anyone now why this is missing?

nick_vh’s picture

Makes total sense to have it in the interface. I also do not know of exceptions so please enlighten us or get it in :)

Let's avoid the following :

nick_vh’s picture

Issue tags: +Database API
drunken monkey’s picture

Just for the record, in case someone else wonders: I noticed some other missing interface methods, in \Drupal\Core\Database\StatementInterface. There, only half of the fetch*() methods are defined, most notably fetch() itself is missing.
The methods are there, though, just commented out, and the interface comment actually explains exactly why that was done:

Some methods in that class are purposefully commented out. Due to a change in how PHP defines PDOStatement, we can't define a signature for those methods that will work the same way between versions older than 5.2.6 and later versions. See http://bugs.php.net/bug.php?id=42452 for more details.

However, this isn't the case here, so we are still left to wonder. (Although, since Drupal 8 won't support "versions older than 5.2.6" anyways, maybe we can change that now, too?)

Crell’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Stalking Crell +Needs backport to D7

I believe it was left off because execute() is a method on the Query base class, which all query builders extend. In hindsight, that seems like a rather stupid reason. :-)

This is *probably* D7-backport-safe. (It only would break on a select object that doesn't implement execute(), which would already be pointless).

donquixote’s picture

Crell’s picture

Sigh PHP...

donquixote’s picture

Btw, DatabaseStatement::execute() is already covered in the other issue #2168241: Type hints for optional methods in StatementInterface (D8) / DatabaseStatementInterface (D7) (although not exactly like here), but this other issue is doing too many things at once. So probably a good idea to get this in first, and then slice up the other issue into smaller ones.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit d12472b on 8.x by catch:
    Issue #2226501 by drunken monkey: Add execute() to the SelectInterface.
    
donquixote’s picture

What about http://3v4l.org/viggJ


interface QueryInterface {
    function execute();
}

interface SelectInterface extends QueryInterface {}

abstract class Query implements QueryInterface {}

class Select extends Query implements SelectInterface {
    function execute() {}
}

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.