Implement special handling for some entity type (and factor out the main business logic) [#1261856]

If you select a specific bundle that's referencable, EntityReference will do a entityCondition('bundle') in the EntityFieldQuerys at entityreference_get_referencable_count() and entityreference_get_referencable_entities().

However, taxonomy_terms and comments don't support that condition, because they have no exact bundle keys in the database.
So you get a crash.

For comments I suggest removing the ability to select a specific bundle in the field settings.

Taxonomy terms will be fixed by #1054162: Taxonomy bundles not supported by EntityFieldQuery (followup) in core, so we have several options:
1) Wait for core to be fixed and do nothing
2) Implement the same alter as the core patch does
3) Disallow specific bundles to be selected in field settings for terms as well.

Thoughts?

Comment	File	Size	Author
#11	1261856-entity-specific-handling.patch	48.84 KB	Damien Tournoud
#11
#10	1261856-entity-specific-handling.patch	48.88 KB	Damien Tournoud
#10
#8	1261856-entity-specific-handling.patch	48.88 KB	Damien Tournoud
#8
#7	1261856-entity-specific-handling.patch	36.1 KB	Damien Tournoud
#7
#6	1261856-entity-specific-handling.patch	34.77 KB	Damien Tournoud
#6
#5	1261856-core-oh-core.patch	16.01 KB	Damien Tournoud
#5
#2	bundles-1054162.patch	2.71 KB	bojanz
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

bojanz CreditAttribution: bojanz commented 27 August 2011 at 13:02

Forgot to mention, in regards to taxonomy terms, I'm for #2. It's a simple copy-paste from the core patch, and it won't break after the core patch lands.

Comment #2

bojanz CreditAttribution: bojanz commented 27 August 2011 at 13:13

Status:

Active

» Needs review

File	Size
bundles-1054162.patch	2.71 KB

Bug reports are always more fun when they have patches attached. Here's one as an example.

Comment #3

Damien Tournoud CreditAttribution: Damien Tournoud commented 27 August 2011 at 15:52

Status:

Needs review

» Needs work

Given the sad state of Drupal core, it seems to be a pipe dream to be able to support all the entity types consistently. I suggest we implement those special cases properly. I envision a generic entity handler (as a class) + specific versions for core entities (as child classes). I think we need the following methods:

- Give me the bundles you control: special case for comments and taxonomy terms
- Get referencable entities (same as the current entityreference_get_referencable_entities): special case for access control for nodes (+status = 1), users (+ status = 1), comments (+has access to the parent node), files (access control is very messy for this thing)
- Get label: user needs a special case because it is brain-dead

Comment #4

bojanz CreditAttribution: bojanz commented 27 August 2011 at 22:00

We should see how much we can get into Entity API. These are common problems (I have special case code for user label handling in VBO as well).

Comment #5

Damien Tournoud CreditAttribution: Damien Tournoud commented 28 August 2011 at 19:23

Status:

Needs work

» Needs review

File	Size
1261856-core-oh-core.patch	16.01 KB

This looks like it could do the trick. Part of the refactoring is to extract the business logic into a class, so that we can more easily swap it (for example for the Views-based selection of entities).

Comment #6

Damien Tournoud CreditAttribution: Damien Tournoud commented 13 September 2011 at 13:15

Title:

Can't reference comments and taxonomy_terms of specific bundles (fatal error)

» Implement special handling for some entity type (and factor out the main business logic)

File	Size
1261856-entity-specific-handling.patch	34.77 KB

New patch up for review.

Comment #7

Damien Tournoud CreditAttribution: Damien Tournoud commented 13 September 2011 at 13:16

File	Size
1261856-entity-specific-handling.patch	36.1 KB

With the upgrade path.

Comment #8

Damien Tournoud CreditAttribution: Damien Tournoud commented 13 September 2011 at 13:22

File	Size
1261856-entity-specific-handling.patch	48.88 KB

And with the missing files.

Comment #9

bojanz CreditAttribution: bojanz commented 13 September 2011 at 13:31

+  /**
+   * Helper method: pass a query to the alteration system again.
+   */
+  protected function reAlterQuery(SelectQueryInterface $query, $tag, $base_table) {

Can we have a comment explaining why we need to realter the query? It's not immediately clear from reading the code
(saying something like "EntityReference alters the query first, then the realter fires so that everyone else can do their altering")

+  public function getLabel($entity) {
+    // entity_label() doesn't work at all for users.
+    // @see http://drupal.org/1261918
+    return format_username($entity);
+  }

The link is wrong, and this is only true for D8, not for D7. We can just kill the whole method, I guess.

In general the new patch is much nicer, cleaner, I love the interface and the usage of ctools plugins. I will take it for a spin once I finish work for today.