Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I'm pretty sure that on D7, EntityFieldQuery had a __toString() method which let you inspect the SQL it would run.
This seems to be missing from the D8 entity Query class.
Comment | File | Size | Author |
---|---|---|---|
#29 | add_tostring_to_the-2902311-29.patch | 2.97 KB | jhedstrom |
Comments
Comment #3
joachim CreditAttribution: joachim at Torchbox commentedI'm going to call this a bug, since the plain SQL Query class has this.
Comment #4
joachim CreditAttribution: joachim at Torchbox commentedStringification is obviously possible, since you can do this to it:
Comment #5
dawehnerI would so enjoy that!
Comment #6
BerdirAgreed, 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..
Comment #7
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedhttps://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.
Comment #8
dawehnerWe should make it clear that you shoudl rely on the output being an executable query, but I think this is totally fine.
Comment #9
joachim CreditAttribution: joachim as a volunteer commentedThinking 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):
Comment #10
joachim CreditAttribution: joachim as a volunteer commented> 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.)
Comment #11
joachim CreditAttribution: joachim at Torchbox commentedWe 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:
Can anyone confirm whether the cloning is necessary? If not, then the above can be simplified to a single method.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman as a volunteer commentedThis would indeed be helpful.
Comment #13
dawehner@joachim
Do you mind creating a patch?
Comment #14
joachim CreditAttribution: joachim as a volunteer commentedSure.
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.
Comment #15
daffie CreditAttribution: daffie commentedLet's combine the 2 methods. There is no more helper method that can be called.
Comment #16
dawehnerHere is some level of test coverage
Comment #17
borisson_This is a really awesome DX improvement. The patch has testcoverage and that looks to be sufficient.
Comment #18
joachim CreditAttribution: joachim as a volunteer commentedWe need to figure this out, surely?
Comment #19
dawehnerThis function is just called for debugging purposes. I guess it is fine to not optimize for performance.
Comment #21
jhedstromSetting to NW to either remove the TODO comment, or add a follow-up issue and add that issue to the code comment.
Comment #23
moshe weitzman CreditAttribution: moshe weitzman as a volunteer commentedIts worth quoting arguments so the sql is runnable (devel does this).
Comment #25
jhedstromThis 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.Comment #26
jhedstromOops, the interdiff was backwards.
Comment #27
daffie CreditAttribution: daffie commentedI have just one minor remark. For the rest is the patch RTBC.
Could it be possible to change the test a bit so that one of the arguments values is NULL?
Comment #28
longwaveThis is the magic __toString method.
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)
Comment #29
jhedstromGood catch!
This also addresses #28.
Comment #30
daffie CreditAttribution: daffie commentedThe patch looks great.
All review points are addressed.
The patch is RTBC for me.
Comment #31
alexpottCommitted 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.
Comment #36
alexpottI hotfixed the test because it was failing on PostgreSQL due to
Comment #37
mondrakeThis 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.
Comment #38
alexpott@mondrake can you file a follow-up for that? We have other tests of SQL query strings in views for example.
Comment #39
daffie CreditAttribution: daffie commentedThe patch looks good to me. I shall give it a RTBC.
Comment #40
mondrakeFiled #3098426: EntityQueryTest::testToString fails with non-core db drivers, thanks