Move node specific code out of entity.inc [#566940]

One of the followups from entity loading was moving the node specific stuff out of entity.inc into NodeController - this is the code hunk:

    if ($this->revisionKey) {
      // Add all fields from the {entity_revision} table.
      $entity_revision_fields = drupal_map_assoc(drupal_schema_fields_sql($this->revisionTable));
      // The id field is provided by entity, so remove it.
      unset($entity_revision_fields[$this->idKey]);

      // Change timestamp to revision_timestamp, and revision uid to
      // revision_uid before adding them to the query.
      // TODO: This is node specific and has to be moved into NodeController.
      unset($entity_revision_fields['timestamp']);
      $this->query->addField('revision', 'timestamp', 'revision_timestamp');
      unset($entity_revision_fields['uid']);
      $this->query->addField('revision', 'uid', 'revision_uid');

This needs to go into NodeController ->buildQuery().

Comment	File	Size	Author
#17	566940-cleanupcontroller.patch	2.08 KB	catch
#15	cleanupcontroller.patch	2 KB	catch
#11	entity_revisions.patch	2 KB	catch
#8	entity_revisions.patch	2 KB	catch
#6	entity_revisions.patch	1.59 KB	catch
#3	node.patch	3.66 KB	catch
#1	nodecontroller.patch	3.57 KB	catch

Comments

Comment #1

catch

he/him

English

commented 3 September 2009 at 03:17

Status:

Active

» Needs review

Status	File	Size
new	nodecontroller.patch	3.57 KB

This might be a bit of a sledgehammer, but on first look, there didn't seem to be an obvious way to remove fields from a query object, so just copied the whole of buildQuery for now.

Comment #2

3 September 2009 at 03:25

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Status	File	Size
new	node.patch	3.66 KB

Comment #4

Frando commented 26 September 2009 at 00:42

I think you should be able to remove fields by modifying the query's raw field property by reference. So $fields =& $this->query->getFields(); and then altering $fields as needed.

Comment #5

cburschka

they

commented 31 December 2009 at 19:01

Status:

Needs review

» Needs work

Should it be done that way?

Otherwise, set back to NR.

Comment #6

catch

he/him

English

commented 23 February 2010 at 01:42

Status:

Needs work

» Needs review

Status	File	Size
new	entity_revisions.patch	1.59 KB

Here's a patch using the method from #4. Much more concise doing it this way, not entirely sure about modifying the field array directly like that. Untested apart from clicking a couple of times.

This issue causes SQL errors if you create an entity with revision support which doesn't have {myentity_revision}.timestamp and {myentity_revision.uid} fields. So it's somewhat critical in the sense that it's broken, but not sure who's doing that apart from me last night.

Comment #7

23 February 2010 at 02:10

Status:

Needs review

» Needs work

The last submitted patch, entity_revisions.patch, failed testing.

Comment #8

catch

he/him

English

commented 1 March 2010 at 05:13

Status:

Needs work

» Needs review

Status	File	Size
new	entity_revisions.patch	2 KB

Fixed uid breakage.

Comment #9

cburschka

they

commented 1 March 2010 at 09:00

Two more code style suggestions, and this looks good to go.

+++ modules/node/node.module	1 Mar 2010 05:12:17 -0000
@@ -3604,4 +3604,15 @@ class NodeController extends DrupalDefau
+    // Ensure that uid is taken from the {node} table
+    // alias timestamp to revision_timestamp and add revision_uid.

A comma after "table" could make the comment clearer, since it lists three actions ("a, b and c" rather than "a b and c").

+++ modules/node/node.module	1 Mar 2010 05:12:17 -0000
@@ -3604,4 +3604,15 @@ class NodeController extends DrupalDefau
+    unset($fields['timestamp']);
+    $fields['uid']['table'] = 'base';
+    $this->query->addField('revision', 'timestamp', 'revision_timestamp');

Does the code have to make the uid table change between deleting the old timestamp field and adding the new one? If it's avoidable, then arranging the actions in the same order as the comment describes them (first the uid table, then delete and re-add timestamp, then add revision_uid) would make it more readable.

140 critical left. Go review some!