Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

Category: Task » Bug report
Issue tags: +DX (Developer Experience)

I'm going to call this a bug, since the plain SQL Query class has this.

joachim’s picture

Stringification is obviously possible, since you can do this to it:

$query = \Drupal::entityQuery('node')
  ->addTag('debug')
  ->execute();
dawehner’s picture

I would so enjoy that!

Berdir’s picture

Agreed, that would be nice, but:

> I'm pretty sure that on D7, EntityFieldQuery had a __toString() method which let you inspect the SQL it would run.

Nope, I'm pretty sure that didn't exist and 7.x was just as annoying to debug them.

Point being that I think this is a feature/task, not a bug. Yes, the Select query builder has this, but that's part of it's design.

While it might be fairly simple to add it for content entity queries, it's not simple at all for config entities for example, there is no simple query string there that we can build. So it's definitely not something we can add to the interface, only a specific implementation and then it would be pretty magically hidden away, sometimes working and sometimes not..

navneet0693’s picture

https://api.drupal.org/api/drupal/includes!entity.inc/class/EntityFieldQ... doesn't have one such function, that means we will have to work on it.

dawehner’s picture

We should make it clear that you shoudl rely on the output being an executable query, but I think this is totally fine.

joachim’s picture

Thinking about it some more, the big difference between a __toString() and doing Devel module's ->addTag('debug') is that adding the tag gets you an output of the query when it's complete and about to be executed.

__toString() would output the query you have so far. That's a much more powerful tool, because it helps you understand how adding something to the query changes is.

Eg (pseudocode):

$q = new query;
$q->condition(foo);
dump((string) $q; // see the SQL built up so far.
$q->condition(bar);
dump((string) $q; // see how the new condition has changed the SQL.
joachim’s picture

> We should make it clear that you shoudl rely on the output being an executable query, but I think this is totally fine.

I think you meant to say 'should NOT rely', right?

I'd say it's fairly clear that the output isn't executable -- the __toString() on a regular query gives you something that has the {} around table names and the placeholder tokens.

(I think making it so it *can* run would be nicer, but let's keep this issue about getting parity with regular queries.)

joachim’s picture

Status: Active » Needs work

We basically need to call prepare() on the EntityQuery, and then get the stringified $this->sqlQuery.

But I don't know whether calling prepare() again later on when the query is executed would cause problems: it's written to be called only once, but execute() AFAICT.

In which case, safest thing might be for __toString() to clone the object, and call a helper method on it. This is working nicely for me:

  public function __toString() {
    // Clone the query so the prepare and compile stuff doesn't get repeated.
    $clone = clone($this);
    
    
    return $clone->toString();
  }
  
  // helper for __toString(). Should only be called on a cloned copy of the query.
  public function toString() {
    $this->prepare()
      ->compile()
      ->addSort()
      ->finish();
    
    return (string) $this->sqlQuery;
  }

Can anyone confirm whether the cloning is necessary? If not, then the above can be simplified to a single method.

moshe weitzman’s picture

This would indeed be helpful.

dawehner’s picture

@joachim
Do you mind creating a patch?

joachim’s picture

Status: Needs work » Needs review
FileSize
969 bytes

Sure.

This is just a proof of concept, but setting to NR because it needs input from someone who knows the entity query system better than I do.

daffie’s picture

Issue tags: +Needs tests coverage
FileSize
872 bytes

Let's combine the 2 methods. There is no more helper method that can be called.

dawehner’s picture

Issue tags: -Needs tests coverage
FileSize
2.41 KB
1.97 KB

Here is some level of test coverage

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This is a really awesome DX improvement. The patch has testcoverage and that looks to be sufficient.

joachim’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Query.php
@@ -316,4 +316,22 @@ public function getTables(SelectInterface $sql_query) {
+    // Clone the query so the prepare and compile stuff doesn't get repeated.
+    // TODO: figure out if this is necessary -- is it ok to call compile() more
+    // than once, e.g. if the query is dumped with this, then more is added to
+    // it?

We need to figure this out, surely?

dawehner’s picture

This function is just called for debugging purposes. I guess it is fine to not optimize for performance.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Status: Needs review » Needs work

Setting to NW to either remove the TODO comment, or add a follow-up issue and add that issue to the code comment.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

moshe weitzman’s picture

Its worth quoting arguments so the sql is runnable (devel does this).

$connection = Database::getConnection();
foreach ((array) $query->arguments() as $key => $val) {
    $quoted[$key] = is_null($val) ? 'NULL' : $connection->quote($val);
}

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jhedstrom’s picture

This removes the @TODO since as noted in #19 cloning here is ok as this is for debugging. It also quotes values as suggested in #23.

jhedstrom’s picture

FileSize
2.35 KB

Oops, the interdiff was backwards.

daffie’s picture

I have just one minor remark. For the rest is the patch RTBC.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Query.php
@@ -326,4 +326,25 @@ public function getTables(SelectInterface $sql_query) {
+    // Quote arguments so query is able to be run.
+    $quoted = [];
+    foreach ($clone->sqlQuery->getArguments() as $key => $value) {
+      $quoted[$key] = is_null($value) ? 'NULL' : $this->connection->quote($value);
+    }

Could it be possible to change the test a bit so that one of the arguments values is NULL?

longwave’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Query.php
    @@ -326,4 +326,25 @@ public function getTables(SelectInterface $sql_query) {
    +   * Implements the magic __clone method.
    

    This is the magic __toString method.

  2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Query.php
    @@ -326,4 +326,25 @@ public function getTables(SelectInterface $sql_query) {
    +    // Quote arguments so query is able to be run.
    

    The query isn't really able to run while the table names are surrounded by curly braces - should we remove them and add the table prefix (if any)? (This has always been a bugbear when using similar debug techniques on queries in the past)

jhedstrom’s picture

FileSize
2.86 KB
2.97 KB

Could it be possible to change the test a bit so that one of the arguments values is NULL?

Good catch!

This also addresses #28.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks great.
All review points are addressed.
The patch is RTBC for me.

alexpott’s picture

Category: Bug report » Task
Status: Reviewed & tested by the community » Fixed

Committed and pushed d86ce7363b to 9.0.x and 832b5b5cc4 to 8.9.x. Thanks!

Crediting issue reviews.

Classifying as a task because this is not fixing a bug.

  • alexpott committed d86ce73 on 9.0.x
    Issue #2902311 by jhedstrom, dawehner, joachim, daffie, moshe weitzman,...

  • alexpott committed 832b5b5 on 8.9.x
    Issue #2902311 by jhedstrom, dawehner, joachim, daffie, moshe weitzman,...

  • alexpott committed ef1d9ac on 9.0.x
    Issue #2902311 hotfix by alexpott: add __toString() to the entity Query...

  • alexpott committed f09913a on 8.9.x
    Issue #2902311 hotfix by alexpott: add __toString() to the entity Query...
alexpott’s picture

I hotfixed the test because it was failing on PostgreSQL due to

Testing Drupal\KernelTests\Core\Entity\EntityQueryTest
..............F                                                   15 / 15 (100%)

Time: 54.31 seconds, Memory: 4.00 MB

There was 1 failure:

1) Drupal\KernelTests\Core\Entity\EntityQueryTest::testToString
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 INNER JOIN test59252575entity_test_mulrev__xvskkp00 entity_test_mulrev__xvskkp00_2 ON entity_test_mulrev__xvskkp00_2.entity_id = base_table.id\n
 LEFT JOIN test59252575entity_test_mulrev__xvskkp00 entity_test_mulrev__xvskkp00_3 ON entity_test_mulrev__xvskkp00_3.entity_id = base_table.id\n
 WHERE (entity_test_mulrev__xvskkp00.xvskkp00_color IN ('blue')) AND (entity_test_mulrev__xvskkp00_2.xvskkp00_color IN ('red')) AND (entity_test_mulrev__xvskkp00_3.xvskkp00_color IS NULL)\n
-ORDER BY base_table.id ASC'
+ORDER BY base_table.id ASC NULLS FIRST'

/var/www/html/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit7/TestCompatibilityTrait.php:53
/var/www/html/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php:1233
mondrake’s picture

This patch is very strict wrt the SQL syntax produced by the __toString method, as it assumes it is produced by Drupal\Core\Database\Query\Select. If a contrib driver overrides that method, it may fail. I suggest we just skip the test if not run against one of the core's provided db drivers.

alexpott’s picture

@mondrake can you file a follow-up for that? We have other tests of SQL query strings in views for example.

daffie’s picture

The patch looks good to me. I shall give it a RTBC.

mondrake’s picture

Status: Fixed » Closed (fixed)

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