PHPDoc references CommentStorage::buildQuery on SqlContentEntityStorage::buildQuery. This method no longer exists nor there is any other example adding conditions in core, so I think it can just be removed.

/**
   * Builds the query to load the entity.
   *
   * This has full revision support. For entities requiring special queries,
   * the class can be extended, and the default query can be constructed by
   * calling parent::buildQuery(). This is usually necessary when the object
   * being loaded needs to be augmented with additional data from another
   * table, such as loading node type into comments or vocabulary machine name
   * into terms, however it can also support $conditions on different tables.
   * See Drupal\comment\CommentStorage::buildQuery() for an example.

Thanks

CommentFileSizeAuthor
#16 3052755-16.patch1.52 KBsam711
#12 interdiff_5_12.txt902 bytesmdupont
#12 3052755-12.patch1.07 KBmdupont
#7 3052755-5.patch805 bytesmdupont
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sam711 created an issue. See original summary.

valthebald’s picture

valthebald
Location Sofia
CreditAttribution: valthebald at Dropsolid commented
Issue tags: +Amsterdam2019, +Novice, +Documentation
mdupont’s picture

mdupont
Primary language French
Location Switzerland
CreditAttribution: mdupont as a volunteer commented

I am at DrupalCon Amsterdam 2019 and am working on this issue.

mdupont’s picture

mdupont
Primary language French
Location Switzerland
CreditAttribution: mdupont at United Nations commented
hanan alasari’s picture

hanan alasari
Primary language English
Location kassel
CreditAttribution: hanan alasari for AuPairWorld commented

Hi, im from Drupalcon in Amsterdam 2019, am working on this issue.

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
CreditAttribution: wengerk at Antistatique commented

Hey there ! I'm mentoring @hanan_apw and @mdupont at the Drupalcon in Amsterdam 2019.

Let's create a patch for this issue and having fun !

mdupont’s picture

mdupont
Primary language French
Location Switzerland
CreditAttribution: mdupont at United Nations commented
FileSize
3052755-5.patch805 bytes

CommentStorage::buildQuery() was removed in 2014 with #2292815, and there is no other example of extending the buildQuery() method in Core. Tested against 8.8.x branch.

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
CreditAttribution: wengerk at Antistatique commented
Status: Active » Needs review

Let's put this issue in review for now.

hanan alasari’s picture

hanan alasari
Primary language English
Location kassel
CreditAttribution: hanan alasari for AuPairWorld commented

Thank you, this patch worked for me too.

wengerk’s picture

wengerk
he/him
Primary language French
Location Lausanne
CreditAttribution: wengerk at Antistatique commented
Status: Needs review » Reviewed & tested by the community

All the team (@hanan_apw, @mdupont, @solide-echt & @gdejonghe) review the patch let's put in in the Reviewed & tested by the community.

Let's see if testbot make our patch pass (which should as we only change a comment).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -712,7 +712,6 @@ protected function buildPropertyQuery(QueryInterface $entity_query, array $value
    * table, such as loading node type into comments or vocabulary machine name
    * into terms, however it can also support $conditions on different tables.

So this shows that actually when need to update4 

such as loading node type into comments or vocabulary machine name
   * into terms, however it can also support $conditions on different tables.

As this such as is also no longer applicable.

FYI the comment storage override was removed in #2292815: Remove join comments on users table

mdupont’s picture

mdupont
Primary language French
Location Switzerland
CreditAttribution: mdupont at United Nations commented
Status: Needs work » Needs review
FileSize
3052755-12.patch1.07 KB
interdiff_5_12.txt902 bytes

Removed DocBlock reference to altering comments entities, as per @alexpott message in #3052755-11: Remove PHPDoc reference to CommentStorage::buildQuery() on SqlContentEntityStorage

mdupont’s picture

mdupont
Primary language French
Location Switzerland
CreditAttribution: mdupont at United Nations commented
Version: 8.7.0 » 9.0.x-dev

As the same code is present in the 9.0.x branch, I suggest to fix it there first. Same patch applies in 9.0.x, 8.8.x and 8.7.x AFAICS.

mdupont’s picture

mdupont
Primary language French
Location Switzerland
CreditAttribution: mdupont at United Nations commented
Version: 9.0.x-dev » 8.9.x-dev

After discussing on Slack #contribute, it turns out the preferred way forward is to fix in 8.9.x first.

init90’s picture

init90
Location Ukraine
CreditAttribution: init90 at ZANZARRA Drupal Agency, Drupal Ukraine Community commented

Before RTBC here needed one small change.

Instead of class reference in @return docblock:

   * @return \Drupal\Core\Database\Query\Select
   *   A SelectQuery object for loading the entity.

will be better use reference to interface:

   * @return \Drupal\Core\Database\Query\SelectInterface
   *   A Select query object.
sam711’s picture

sam711 CreditAttribution: sam711 as a volunteer commented
FileSize
3052755-16.patch1.52 KB

You're right, but I think the description was more accurate in first place.

Patch updated according to @init90 about the returned interface.

Thank you all!

init90’s picture

init90
Location Ukraine
CreditAttribution: init90 at ZANZARRA Drupal Agency, Drupal Ukraine Community commented
Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 9.0.x and to 8.9.x and to 8.8.x. Thanks!

Backported to 8.8.x as a docs fix.

  • alexpott committed f2c66f6 on 9.0.x 
    Issue #3052755 by mdupont, sam711, wengerk, hanan_apw, init90, alexpott...

  • alexpott committed d24eb2e on 8.9.x 
    Issue #3052755 by mdupont, sam711, wengerk, hanan_apw, init90, alexpott...

  • alexpott committed e5ed79c on 8.8.x 
    Issue #3052755 by mdupont, sam711, wengerk, hanan_apw, init90, alexpott...

Status: Fixed » Closed (fixed)

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