The query plugin should load entities after the query has been executed [#1758634]

Currently the entities are only loaded when they are needed, which is every time a field is used, or when an edit / delete link is shown (since the entity is needed to check access). This means that the entity is loaded in almost every case.

D8 will make entities needed for properties as well (especially if you want to have properties work across query backends, which is very desirable), since they will become very field-like (and trying to avoid loading entities for fields didn't go well for us).
So, I suggest always loading entities if the tables in the query belong to entity types, which allows us to cleanup the Field API and Entity field handlers, as well as allowing further improvements (handlers for properties).

Comment	File	Size	Author
#15	1758634-load-entities.patch	31.4 KB	bojanz
#15
#13	1758634-load-entities.patch	35.49 KB	bojanz
#13
#11	1758634-load-entities.patch	35.25 KB	bojanz
#11
#3	1758634-load-entities.patch	34.44 KB	bojanz
#3
#9	1758634-load-entities.patch	35.2 KB	bojanz
#9
#9	1758634-load-entities-interdiff.patch	5.9 KB	bojanz
#9

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 27 August 2012 at 09:58

Issue tags:

+VDC

Tagging.

Comment #2

dawehner

German

CreditAttribution: dawehner commented 27 August 2012 at 10:43

The really advantage of doing that would be that we could probably get rid A LOT OF code from the field field handler
which takes care of all the loading of the entities.

Comment #3

bojanz CreditAttribution: bojanz commented 28 August 2012 at 13:22

Status:	Needs work	» Active
Issue tags:	-Needs tests

File	Size
1758634-load-entities.patch	34.44 KB

8 files changed, 248 insertions(+), 445 deletions(-)

The Field API handler is 100 lines smaller alone.

Changelog:
1) Removed get_result_entities() from the query object. Added a load_entities() method (used by the query execute(), not the handlers!) that also supports revisions (like the Field API code, unlike the old get_result_entities()). Entities are stored either in $row->_entity or $row->_relationship_entities[$relationship_id].
2) Made the base fields of all entity tables always added to the query (so that we can actually load the entities), and because of that, removed "pure distinct".
2) Removed Field API post_execute() and half of the query() code. Removed the useless "add fields to query" flag and renamed get_value to process_entity for more clarity.
3) Removed the "Entity" field handler since now all handlers have access to the entities matching the row.
4) Added a convenience get_entity() method to the base field handler (FieldHandlerBase).

Comment #4

aspilicious CreditAttribution: aspilicious commented 28 August 2012 at 11:15

I'm a total noob on this but do we still need to implement the query method in "FieldPluginBase". It seems it can be removed. If thats the case you can also remove it from the classes inheriting FieldPluginBase

Comment #5

xjm

she/her

English

CreditAttribution: xjm commented 28 August 2012 at 12:32

Status:	Active	» Needs review
Issue tags:		+Needs tests

BOTIFY

Comment #6

dawehner

German

CreditAttribution: dawehner commented 28 August 2012 at 12:37

Issue tags:

-Needs tests

+++ b/lib/Drupal/views/Plugin/views/query/QueryPluginBase.phpundefined
@@ -158,10 +158,12 @@ abstract class QueryPluginBase extends PluginBase implements QueryInterface {
+  function load_entities(&$results) {}

As we talked before, we should document what people should do if there are no entities related to the query plugin.

+++ b/lib/Drupal/views/Plugin/views/query/Sql.phpundefined
@@ -1489,9 +1457,10 @@ class Sql extends QueryPluginBase {
+        // Load all entities contained in the results.
+        $this->load_entities($view->result);
 
         $view->pager->post_execute($view->result);

post_execute of the page is used by fast-pagers so let's allow them to control stuff before loading entities.

+++ b/lib/Drupal/views/Plugin/views/query/Sql.phpundefined
@@ -1513,6 +1482,118 @@ class Sql extends QueryPluginBase {
+   * @return

missing array

+++ b/lib/Drupal/views/Plugin/views/query/Sql.phpundefined
@@ -1513,6 +1482,118 @@ class Sql extends QueryPluginBase {
+   *   An array of table information, keyed by table alias.

what about describing the content of the table information as well?

+++ b/lib/Drupal/views/Plugin/views/query/Sql.phpundefined
@@ -1513,6 +1482,118 @@ class Sql extends QueryPluginBase {
+      $id_key = $table['revision'] ? $info['entity keys']['revision'] : $info['entity keys']['id'];

Just for readability we might turn around the logic in general. Most of the time people take care just about the normal entity handling.

+++ b/lib/Drupal/views/Plugin/views/query/Sql.phpundefined
@@ -1513,6 +1482,118 @@ class Sql extends QueryPluginBase {
+          $results[$index]->_entity = $entity;
...
+          $results[$index]->_relationship_entities[$relationship_id] = $entity;

Just in general is there any way we can document this that people are able to find that?

+++ b/lib/Views/comment/Plugin/views/field/Link.phpundefined
@@ -47,19 +43,16 @@ class Link extends Entity {
+    $comment = $this->get_entity($values);
+    return $this->render_link($comment, $values);
...
+    $comment = $data;

I'm not 100% sure about that, but it seems to be that render_link expects the cid here and get_entity returns the full objects.