I am being prompted by the following lint error

Method __toString is not implemented for the following objects. .... \Drupal\Core\Database\Query\SelectInterface

This is a child issue of #2344799: [Meta issue] Clear _toString is not implemented errors.

The idea is that we must enforce a __toString() method on all objects implementing

\Drupal\Core\Database\Query\SelectInterface

outputting a representation as a string is fundamental to how the query select in used.

This to my mind is a reasonable nit-picky improvement that not only clears a "Probably a bug" lint error, but is a DX thing as it will prompt developers into doing the right thing, quickly and all of the time.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it's a documentation error.
Issue priority Normal because nothing is broken other than docs.
Unfrozen changes Unfrozen because it documents an interface.
Prioritized changes Prioritized because it improves DX.
CommentFileSizeAuthor
#4 select-2344831-4.patch1.05 KBmartin107
#2 select-2344831-2.patch622 bytesmartin107
select-0.patch598 bytesmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

I don't think it's too late for this. As you say the method is already de facto required because we use (string) $query, so I don't see how you could use a query that doesn't have it. Enforcing this on the method is more a matter of documentation than an actual API change. I would even suggest backporting this.

I think the comment could be improved a bit to mention that the "string representation" of the query is the query itself, i.e. how it will be executed in SQL. also there should be a blank line after the method before the closing brace.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Title: Cleanup API to avoid future confusion and avoid lint errors » Enforce API to avoid future confusion and avoid lint errors
FileSize
select-2344831-2.patch622 bytes

I agree... thanks for the prompt reply... next iteration is less recursive, and to my eyes does read better.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: Enforce API to avoid future confusion and avoid lint errors » Document behavior of Drupal/Core/Database/Query/SelectInterface::__toString()
Category: Task » Bug report
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -

Drupal\Core\Database\Query\Select implements _toString() and documents it as {@inheritdoc}, so I'd say this is a good idea.

However, the patch doesn't apply.

$ git apply select-2344831-2.patch 
error: patch failed: core/lib/Drupal/Core/Database/Query/SelectInterface.php:535
error: core/lib/Drupal/Core/Database/Query/SelectInterface.php: patch does not apply
martin107’s picture

martin107 CreditAttribution: martin107 commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
select-2344831-4.patch1.05 KB

Rerolled and I have added an inheritdoc to SelectExtender.php

Plus fixed some of my broken english in the issue summary.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Related issues: +#2577487: \Drupal\Core\Database\Query\SelectExtender - Needs more @inheritdoc tags

While working on #4 I noticed SelectExtender was missing an @inheritdoc or ... two

So I opened an issue for that

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

This seems perfectly reasonable to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: select-2344831-4.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Reviewed & tested by the community

Some random test failure.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 8144cce on 8.0.x 
    Issue #2344831 by martin107, jhodgdon, tstoeckler, Mile23: Document...

Status: Fixed » Closed (fixed)

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