This is a single minded issue.....

The application of @inheritdoc was a bit patchy.

CommentFileSizeAuthor
selectExtender-1.patch9.06 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

cilefen’s picture

cilefen CreditAttribution: cilefen commented 
+++ b/core/lib/Drupal/Core/Database/Query/SelectExtender.php
@@ -342,6 +478,9 @@ public function __toString() {
+  /**
+   * {@inheritdoc}
+   */
   public function __clone() {
     $this->uniqueIdentifier = uniqid('', TRUE);

Do we put @inheritdoc on things like the built-in __clone() method?

jhodgdon’s picture

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

I don't think we usually do, but in this case it is probably OK because the method is declared on SelectInterface for some reason.

This looks good to me, thanks!

martin107’s picture

martin107 CreditAttribution: martin107 commented

in this case __clone() is explicitly listed on the SelectInterface

  /**
   * Clone magic method.
   *
   * Select queries have dependent objects that must be deep-cloned.  The
   * connection object itself, however, should not be cloned as that would
   * duplicate the connection itself.
   */
  public function __clone();

Just to add a little context...

For me this is an issue split off from

#2344831: Document behavior of Drupal/Core/Database/Query/SelectInterface::__toString()

Where I wanted to add the conventional @inheritdoc tag to a another magic method.... __toString()

That issue got started as a DX issue ... I was having trouble locating a bug of my own making and things would have been much simpler if my IDE had drawn my attention to the missing __toString().

Status: Reviewed & tested by the community » Needs work

The last submitted patch, selectExtender-1.patch, failed testing.

martin107’s picture

martin107 CreditAttribution: martin107 commented

looking at fails in "installer test"

it looks like mysql was not present ...
this issue is a documentation only patch...

I think this is a testbot fail... I will retest in a few hours and see what gives.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs work » Needs review

triggering test.

martin107’s picture

martin107 CreditAttribution: martin107 commented

On my local machine I don't see these fails.

As a double check

I used simplytest.me to create a 8.0.x sandbox with the patch applied.

I picked two of the failing tests more or less at random

Installer Test
SingleVisibleProfileTest

both tests passed.

I am not sure why our test infrastructure is the odd one out ...hmm

Any suggestions welcome.

jhodgdon’s picture

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

Yeah this is a docs patch. Test fails that are valid are only "the patch doesn't apply". It was a test bot problem. Back to RTBC.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Also more cowbell!

I reviewed all these methods and confirmed they're all implementations of something-or-other and can therefore @inheritoc. Committed and pushed to 8.0.x. Thanks!

  • xjm committed 5b8b5aa on 8.0.x 
    Issue #2577487 by martin107, jhodgdon, cilefen: \Drupal\Core\Database\...

Status: Fixed » Closed (fixed)

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