Remove entity_extract_ids() now that we have proper classed entity objects

We probably need a vid() funtion like we have with id() and bundle() that way we can reset it to a default we want (for each entity).

Right. I remember we left it out revisions for the EntityInterface in the first place. I've created #1612014: Create an interface for revisionable entities for that.

This is being discussed in #1551140: Remove stub entities, replace entity_create_stub_entity(), there are two working (although not finished/complete) approaches in there and a long-term proposal for the EFQ part. We need to decide how to continue there.

#15: 1374030-extract-ids-15.patch queued for re-testing.

Tired of looking at that title;).

Fatal error: Can't use method return value in write context in core/modules/field/field.api.php on line 1919

I just copied the actual field_sql_storage_field_storage_delete_revision() into hook_field_storage_delete_revision().

function entity_form_field_validate($entity_type, $form, &$form_state) {
  // All field attach API functions act on an entity object, but during form
  // validation, we don't have one. $form_state contains the entity as it was
  // prior to processing the current form submission, and we must not update it
  // until we have fully validated the submitted input. Therefore, for
  // validation, act on a pseudo entity created out of the form values.
  $pseudo_entity = (object) $form_state['values'];
  field_attach_form_validate($entity_type, $pseudo_entity, $form, $form_state);
}

Rly? U serious?

Worked around that for now, fixed lots of exceptions because of undefined $id's and added type hints for $entity in field.attach.inc, to easier see more of these weird calls if there are any. Book tests, which previously exploded, pass now, let's see about everything else...

Working on this, we'll have to fix the test_entity entity before we can do this but I'm making progress...

Go testbot, go!

This currently also contains a lot of code that is related to fixing test_entity. I'll extract it into the other issue later, just want to see how far we get with this.

Next try.

Ok, here we go, this should finally pass.

At this point, the conversion of the whole test_entity stuff and the removal of entity_extract_ids() is kinda mixed up because the former is required for this to pass but then required additional test and code changes on top of what's required here so I hope we can get this in together.

I think this is ready for reviews now.

I do believe this should be major. #1616930: Fix the field_test entity type is and is now kinda blocked on it and I think we all *really* want entity_extract_ids() to be a thing of the past.

#1723892: Support for revisions for entity save and delete operations would allow us to remove the custom storage controller code here...

Yes please.

Finally! Excellent clean-up.

+++ b/core/modules/field/field.attach.inc
@@ -658,9 +657,13 @@ function field_attach_load($entity_type, $entities, $age = FIELD_LOAD_CURRENT, $
+      if (!($entity instanceof EntityInterface)) {
+        debug(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS));
+        debug($entity);
+      }

Left-over debugging code?

This patch looks RTBC to me after removing that glitch.

Great clean up!

+++ b/core/modules/entity/entity.module
@@ -409,12 +371,11 @@ function entity_prepare_view($entity_type, $entities) {
   $info = entity_get_info($entity_type);
-  list($id, $vid, $bundle) = entity_extract_ids($entity_type, $entity);
 
   // A bundle-specific callback takes precedence over the generic one for the
   // entity type.
-  if (isset($info['bundles'][$bundle]['uri callback'])) {
-    $uri_callback = $info['bundles'][$bundle]['uri callback'];
+  if (isset($info['bundles'][$entity->bundle()]['uri callback'])) {
+    $uri_callback = $info['bundles'][$entity->bundle()]['uri callback'];
   }

+++ b/core/modules/field/field.api.php
@@ -584,15 +584,14 @@ function hook_field_storage_update_field($field, $prior_field, $has_data) {
+    $item['file_field_id'] = $entity->id();
+    file_field_delete_file($item, $field, $entity_type, $entity->id());

@@ -2631,17 +2628,15 @@ function hook_field_storage_purge_field_instance($instance) {
+    ->condition('entity_id', $entity->id())
     ->execute();
   db_delete($revision_name)
     ->condition('entity_type', $entity_type)
-    ->condition('entity_id', $id)
+    ->condition('entity_id', $entity->id())

+++ b/core/modules/field/modules/field_sql_storage/field_sql_storage.module
@@ -480,17 +480,15 @@ function field_sql_storage_field_storage_delete($entity_type, $entity, $fields)
     ->condition('entity_type', $entity_type)
-    ->condition('entity_id', $id)
+    ->condition('entity_id', $entity->id())
     ->execute();
   db_delete($revision_name)
     ->condition('entity_type', $entity_type)
-    ->condition('entity_id', $id)
+    ->condition('entity_id', $entity->id())

+++ b/core/modules/file/file.field.inc
@@ -231,13 +229,11 @@ function file_field_insert($entity_type, $entity, $field, $instance, $langcode,
+      file_usage_add(file_load($item['fid']), 'file', $entity_type, $entity->id());

@@ -273,11 +269,9 @@ function file_field_update($entity_type, $entity, $field, $instance, $langcode,
+    file_usage_delete(file_load($item['fid']), 'file', $entity_type, $entity->id(), 0);

+++ b/core/modules/rdf/rdf.module
@@ -395,8 +395,7 @@ function rdf_entity_info_alter(&$entity_info) {
+    $entity->rdf_mapping = rdf_mapping_load($type, $entity->bundle());

Minor thing: could we use a variable to avoid calling the methods multiple times?
(I wouldn't even ask if there wasn't @sun's remark above to fix :)

+    $item['file_field_id'] = $entity->id();
+    file_field_delete_file($item, $field, $entity_type, $entity->id());

Interesting, looks like I forgot to fix the hook documentation when I removed that function. (not related to this issue). Need to open an issue about that.

Removed the debug code. I added and removed that like 3 times and forgot it multiple times ;)

@plach: I'm not sure. I personally spent quite some time doing performance tests on Entity::id() and we decided to hardcode it, so it's fast and can be called multiple times. So there isn't a performance reason to do so and if it's just for two times, it's IMHO ok to call it directly. But I'm fine with changing it if others agree.

Updated patch attached. Manual interdiff looks bigger, but I verified that's just because code that I'm changing has been moved around.

Re-adding lost tag.

No problem let's go ahead with this :)

Thanks!

Also, I agree with @Berdir - it would be strange to "cache" the property values in local variables, for the sole purpose of avoiding the method calls. Those calls should be lightning fast, so I don't see an issue with that.

Actually, it wasn't a performance concern, just an esthetic one: in many places we have $id or $bundle holding the values returned by the related methods. I was arguing that having a consistent approach would be nice.

Nice clean-up. I still think id() should be getID() but we can worry about that later. Committed to 8.x. Thanks.

Wow, that was fast. Re-opening for change notice.

See: http://drupal.org/node/1724986

Linked those methods. I think if you see how id() is called, it's kinda obvious. But I really wanted to show that $entity_type no longer needs to be passed around and how function definitions should be updated, even though that's not directly related.

And yes, all these field methods still receive $entity_type, not sure if we want to do that to the field functions as well or wait for the field API rewrite based on plugins.

Looks good to me.