In various ways :) Probably critical...

Example query:

$query = Drupal::entityQuery('node');
$query->condition('type', 'article'); // Doesn't change anything.
$query->condition('field_image.entity.filename', $filename);
debug($query->execute());

There are a number of issues with the current code:
- It tries to create the entity without passing in the bundle, that explodes
- There's another place in the code where it just picks a random bundle, copied that for now.
- It still looks for 'entity keys', this has been renamed to entity_keys as part of the entity as plugins conversion.
- For nodes, it's additionally broken because it tries to work with the BC decorator.

Those things are fixed in the attached patch, but the main problem remains:

That nice comment about the the bundle not being relevant and just picking one is a lie. It matters very much, you need the right bundle to get the right fields on your $entity. Can't see an obvious way to get this information, maybe we have to look at which bundle has the field we're interested in and pick the first of those?

We sure have awesome test coverage for this ;)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
1.92 KB

Here's a starting point.

chx’s picture

> // This is an entity property (non-configurable field).

> If there are bundles, pick one. It does not matter which, properties exist on all bundles.

That code only fires for non-configurable fields. We use field_info_field_map() to find configurable fields. How is the comment incorrect then? Do nonconfigurable fields vary by bundle? Yes, we need better coverage with bundles.

chx’s picture

FileSize
4.89 KB

This patch removes deprecated calls and fixes bugs. No tests though yet.

Status: Needs review » Needs work

The last submitted patch, 2044841_3.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
6.54 KB
1.39 KB
Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Ok, this looks good to me. We have test coverage using the entity_test entity type, I confirmed manually with my initial example code that it also works right now for BC decorated node entities, which now returns the correct result with this patch.

To explain the test change a bit, entity_test has a default bundle, so you don't have to specific it (but you have to for nodes, terms, custom blocks, ...). So the old code left it out and used the default. The test changes add the field to the non-default bundle, so it doesn't find the field when that is used.

Thanks @chx!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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