Issue #2241655 by tim.plunkett: EntityStorageInterface::create() should always...

Issue #2241655 by tim.plunkett: EntityStorageInterface::create() should always create a "new" entity.

core/includes/menu.inc

@@ -823,6 +823,9 @@ function menu_link_rebuild_defaults() {
         $link['customized'] = $existing_item->customized;
         $link['updated'] = $existing_item->updated;
         $menu_link = $menu_link_storage->createFromDefaultLink($link);
+        // @todo Do not create a new entity in order to update it, see
+        //   https://drupal.org/node/2241865
+        $menu_link->setOriginalId($existing_item->mlid);
 
         // Convert the existing item to a typed object.
         /** @var \Drupal\menu_link\MenuLinkInterface $existing_item */

core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php

@@ -114,7 +114,7 @@ public function getOriginalId() {
   public function setOriginalId($id) {
     $this->originalId = $id;
 
-    return $this;
+    return parent::setOriginalId($id);
   }
 
   /**

core/lib/Drupal/Core/Entity/ContentEntityBase.php

@@ -925,6 +925,7 @@ public function createDuplicate() {
     $duplicate = clone $this;
     $entity_type = $this->getEntityType();
     $duplicate->{$entity_type->getKey('id')}->value = NULL;
+    $duplicate->enforceIsNew();
 
     // Check if the entity type supports UUIDs and generate a new one if so.
     if ($entity_type->hasKey('uuid')) {

core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php

@@ -67,6 +67,7 @@ public function create(array $values = array()) {
       $bundle = $values[$this->bundleKey];
     }
     $entity = new $entity_class(array(), $this->entityTypeId, $bundle);
+    $entity->enforceIsNew();
 
     foreach ($entity as $name => $field) {
       if (isset($values[$name])) {

core/lib/Drupal/Core/Entity/Entity.php

@@ -502,6 +502,12 @@ public function getOriginalId() {
    */
   public function setOriginalId($id) {
     // By default, entities do not support renames and do not have original IDs.
+    // If the specified ID is anything except NULL, this should mark this entity
+    // as no longer new.
+    if ($id !== NULL) {
+      $this->enforceIsNew(FALSE);
+    }
+
     return $this;
   }
 

core/lib/Drupal/Core/Entity/EntityDatabaseStorage.php

@@ -238,6 +238,7 @@ public function create(array $values = array()) {
     $entity_class::preCreate($this, $values);
 
     $entity = new $entity_class($values, $this->entityTypeId);
+    $entity->enforceIsNew();
 
     // Assign a new UUID if there is none yet.
     if ($this->uuidKey && !isset($entity->{$this->uuidKey})) {

core/modules/comment/comment.module

@@ -1281,7 +1281,11 @@ function comment_prepare_author(CommentInterface $comment) {
   // The account has been pre-loaded by CommentViewBuilder::buildContent().
   $account = $comment->getOwner();
   if (empty($account->uid->value)) {
+    // @todo Avoid creating a new entity by just creating a new instance
+    //   directly, see https://drupal.org/node/1867228.
     $account = entity_create('user', array('uid' => 0, 'name' => $comment->getAuthorName(), 'homepage' => $comment->getHomepage()));
+    // The anonymous user is not a new account, do not treat it as one.
+    $account->enforceIsNew(FALSE);
   }
   return $account;
 }

core/modules/field/lib/Drupal/field/Tests/FieldAttachOtherTest.php

@@ -226,10 +226,6 @@ function testFieldAttachCache() {
 
     // Update with different values, and check that the cache entry is wiped.
     $values = $this->_generateTestFieldValues($this->field_2->getCardinality());
-    $entity = entity_create($entity_type, array(
-      'type' => $entity_type,
-      'id' => $entity->id(),
-    ));
     $entity->{$this->field_name_2} = $values;
     $entity->save();
     $this->assertFalse(\Drupal::cache('entity')->get($cid), 'Cached: no cache entry on update');
@@ -241,10 +237,6 @@ function testFieldAttachCache() {
     $this->assertEqual($cache->data[$langcode][$this->field_name_2], $values, 'Cached: correct cache entry on load');
 
     // Create a new revision, and check that the cache entry is wiped.
-    $entity = entity_create($entity_type, array(
-      'type' => $entity_type,
-      'id' => $entity->id(),
-    ));
     $values = $this->_generateTestFieldValues($this->field_2->getCardinality());
     $entity->{$this->field_name_2} = $values;
     $entity->setNewRevision();

core/modules/field/lib/Drupal/field/Tests/FieldAttachStorageTest.php

@@ -190,6 +190,9 @@ function testFieldAttachSaveEmptyData() {
     $entity = $this->entitySaveReload($entity);
     $this->assertTrue($entity->{$this->field_name}->isEmpty(), 'Insert: NULL field results in no value saved');
 
+    // All saves after this point should be updates, not inserts.
+    $entity_init->enforceIsNew(FALSE);
+
     // Add some real data.
     $entity = clone($entity_init);
     $values = $this->_generateTestFieldValues(1);

core/modules/file/file.module

@@ -173,11 +173,14 @@ function file_copy(File $source, $destination = NULL, $replace = FILE_EXISTS_REN
     $file->setFileUri($uri);
     $file->setFilename(drupal_basename($uri));
     // If we are replacing an existing file re-use its database record.
+    // @todo Do not create a new entity in order to update it, see
+    //   https://drupal.org/node/2241865
     if ($replace == FILE_EXISTS_REPLACE) {
       $existing_files = entity_load_multiple_by_properties('file', array('uri' => $uri));
       if (count($existing_files)) {
         $existing = reset($existing_files);
         $file->fid = $existing->id();
+        $file->setOriginalId($existing->id());
         $file->setFilename($existing->getFilename());
       }
     }
@@ -254,6 +257,7 @@ function file_move(File $source, $destination = NULL, $replace = FILE_EXISTS_REN
         $existing = reset($existing_files);
         $delete_source = TRUE;
         $file->fid = $existing->id();
+        $file->uuid = $existing->uuid();
       }
     }
     // If we are renaming around an existing file (rather than a directory),
@@ -520,11 +524,14 @@ function file_save_data($data, $destination = NULL, $replace = FILE_EXISTS_RENAM
       'status' => FILE_STATUS_PERMANENT,
     ));
     // If we are replacing an existing file re-use its database record.
+    // @todo Do not create a new entity in order to update it, see
+    //   https://drupal.org/node/2241865
     if ($replace == FILE_EXISTS_REPLACE) {
       $existing_files = entity_load_multiple_by_properties('file', array('uri' => $uri));
       if (count($existing_files)) {
         $existing = reset($existing_files);
         $file->fid = $existing->id();
+        $file->setOriginalId($existing->id());
         $file->setFilename($existing->getFilename());
       }
     }
@@ -946,11 +953,14 @@ function file_save_upload($form_field_name, $validators = array(), $destination
     drupal_chmod($file->getFileUri());
 
     // If we are replacing an existing file re-use its database record.
+    // @todo Do not create a new entity in order to update it, see
+    //   https://drupal.org/node/2241865
     if ($replace == FILE_EXISTS_REPLACE) {
       $existing_files = entity_load_multiple_by_properties('file', array('uri' => $file->getFileUri()));
       if (count($existing_files)) {
         $existing = reset($existing_files);
         $file->fid = $existing->id();
+        $file->setOriginalId($existing->id());
       }
     }
 

core/modules/file/lib/Drupal/file/Tests/FileManagedTestBase.php

@@ -150,22 +150,19 @@ function assertSameFile(FileInterface $file1, FileInterface $file2) {
    *   File entity.
    */
   function createFile($filepath = NULL, $contents = NULL, $scheme = NULL) {
-    $file = new \stdClass();
-    $file->uri = $this->createUri($filepath, $contents, $scheme);
-    $file->filename = drupal_basename($file->uri);
-    $file->filemime = 'text/plain';
-    $file->uid = 1;
-    $file->created = REQUEST_TIME;
-    $file->changed = REQUEST_TIME;
-    $file->filesize = filesize($file->uri);
-    $file->status = 0;
+    // Don't count hook invocations caused by creating the file.
+    \Drupal::state()->set('file_test.count_hook_invocations', FALSE);
+    $file = entity_create('file', array(
+      'uri' => $this->createUri($filepath, $contents, $scheme),
+      'uid' => 1,
+    ));
+    $file->save();
     // Write the record directly rather than using the API so we don't invoke
     // the hooks.
-    $file = (array) $file;
-    $file['fid'] = db_insert('file_managed')
-      ->fields($file)
-      ->execute();
-    return entity_create('file', $file);
+    $this->assertTrue($file->id() > 0, 'The file was added to the database.', 'Create test file');
+
+    \Drupal::state()->set('file_test.count_hook_invocations', TRUE);
+    return $file;
   }
 
   /**

core/modules/file/lib/Drupal/file/Tests/FileManagedUnitTestBase.php

@@ -161,20 +161,19 @@ function assertSameFile(FileInterface $file1, FileInterface $file2) {
    *   File entity.
    */
   function createFile($filepath = NULL, $contents = NULL, $scheme = NULL) {
-    $file = new \stdClass();
-    $file->uri = $this->createUri($filepath, $contents, $scheme);
-    $file->filename = drupal_basename($file->uri);
-    $file->filemime = 'text/plain';
-    $file->uid = 1;
-    $file->created = REQUEST_TIME;
-    $file->changed = REQUEST_TIME;
-    $file->filesize = filesize($file->uri);
-    $file->status = 0;
+    // Don't count hook invocations caused by creating the file.
+    \Drupal::state()->set('file_test.count_hook_invocations', FALSE);
+    $file = entity_create('file', array(
+      'uri' => $this->createUri($filepath, $contents, $scheme),
+      'uid' => 1,
+    ));
+    $file->save();
     // Write the record directly rather than using the API so we don't invoke
     // the hooks.
-    $this->assertNotIdentical(drupal_write_record('file_managed', $file), FALSE, 'The file was added to the database.', 'Create test file');
+    $this->assertTrue($file->id() > 0, 'The file was added to the database.', 'Create test file');
 
-    return entity_create('file', (array) $file);
+    \Drupal::state()->set('file_test.count_hook_invocations', TRUE);
+    return $file;
   }
 
   /**

core/modules/file/tests/file_test/file_test.module

@@ -108,9 +108,11 @@ function file_test_get_all_calls() {
  * @see file_test_reset()
  */
 function _file_test_log_call($op, $args) {
-  $results = \Drupal::state()->get('file_test.results') ?: array();
-  $results[$op][] = $args;
-  \Drupal::state()->set('file_test.results', $results);
+  if (\Drupal::state()->get('file_test.count_hook_invocations', TRUE)) {
+    $results = \Drupal::state()->get('file_test.results') ?: array();
+    $results[$op][] = $args;
+    \Drupal::state()->set('file_test.results', $results);
+  }
 }
 
 /**

core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php

@@ -369,7 +369,10 @@ public function reset() {
     $original['machine_name'] = $this->machine_name;
     /** @var \Drupal\menu_link\MenuLinkStorageInterface $storage */
     $storage = \Drupal::entityManager()->getStorage($this->entityTypeId);
+    // @todo Do not create a new entity in order to update it, see
+    //   https://drupal.org/node/2241865
     $new_link = $storage->createFromDefaultLink($original);
+    $new_link->setOriginalId($this->id());
     // Allow the menu to be determined by the parent
     if (!empty($new_link['parent']) && !empty($all_links[$new_link['parent']])) {
       // Walk up the tree to find the menu name.

core/modules/menu_ui/menu_ui.module

@@ -564,7 +564,14 @@ function menu_ui_form_node_form_alter(&$form, $form_state) {
  */
 function menu_ui_node_submit(EntityInterface $node, $form, $form_state) {
   if (!empty($form_state['values']['menu'])) {
+    $original_menu_id = !empty($node->menu) ? $node->menu->id() : NULL;
     $node->menu = entity_create('menu_link', $form_state['values']['menu']);
+    // @todo Do not create a new entity in order to update it, see
+    //   https://drupal.org/node/2241865
+    // If this menu had a previous menu link associated, mark it as not new.
+    if ($original_menu_id) {
+      $node->menu->setOriginalId($original_menu_id);
+    }
     // Decompose the selected menu parent option into 'menu_name' and 'plid', if
     // the form used the default parent selection widget.
     if (!empty($form_state['values']['menu']['parent'])) {

core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php

@@ -515,8 +515,8 @@ public function testSaveNoMismatch() {
       ->will($this->returnValue(array('baz')));
 
     $entity = $this->getMockEntity(array('id' => 'foo'));
-    $entity->enforceIsNew();
     $entity->setOriginalId('baz');
+    $entity->enforceIsNew();
     $this->entityStorage->save($entity);
   }
 

core/tests/Drupal/Tests/Core/Entity/ContentEntityDatabaseStorageTest.php

@@ -15,7 +15,7 @@
 /**
  * Tests the fieldable database storage.
  *
- * @see \Drupal\Core\Entity\ContentEntityDatabaseStorage
+ * @coversDefaultClass \Drupal\Core\Entity\ContentEntityDatabaseStorage
  *
  * @group Drupal
  * @group Entity
@@ -36,7 +36,7 @@ public static function getInfo() {
   /**
    * Tests field SQL schema generation for an entity with a string identifier.
    *
-   * @see \Drupal\Core\Entity\Controller\ContentEntityDatabaseStorage::_fieldSqlSchema()
+   * @covers ::_fieldSqlSchema()
    */
   public function testFieldSqlSchemaForEntityWithStringIdentifier() {
     $field_type_manager = $this->getMock('Drupal\Core\Field\FieldTypePluginManagerInterface');
@@ -112,4 +112,61 @@ public function testFieldSqlSchemaForEntityWithStringIdentifier() {
     $this->assertEquals($schema['test_entity__test_field']['fields']['revision_id']['type'], 'varchar');
   }
 
+  /**
+   * @covers ::create()
+   */
+  public function testCreate() {
+    $language_manager = $this->getMock('Drupal\Core\Language\LanguageManagerInterface');
+    $module_handler = $this->getMock('Drupal\Core\Extension\ModuleHandlerInterface');
+    $entity_manager = $this->getMock('Drupal\Core\Entity\EntityManagerInterface');
+    // @todo Add field definitions to test default values of fields.
+    $entity_manager->expects($this->atLeastOnce())
+      ->method('getFieldDefinitions')
+      ->will($this->returnValue(array()));
+
+    $container = new ContainerBuilder();
+    $container->set('language_manager', $language_manager);
+    $container->set('entity.manager', $entity_manager);
+    $container->set('module_handler', $module_handler);
+    \Drupal::setContainer($container);
+
+    $entity = $this->getMockForAbstractClass('Drupal\Core\Entity\ContentEntityBase', array(), '', FALSE, TRUE, TRUE, array('id'));
+    $entity_type = $this->getMock('Drupal\Core\Entity\EntityTypeInterface');
+    $entity_type->expects($this->atLeastOnce())
+      ->method('id')
+      ->will($this->returnValue('test_entity_type'));
+    $entity_type->expects($this->atLeastOnce())
+      ->method('getClass')
+      ->will($this->returnValue(get_class($entity)));
+    $entity_type->expects($this->atLeastOnce())
+      ->method('getKeys')
+      ->will($this->returnValue(array('id' => 'id')));
+    $entity_type->expects($this->atLeastOnce())
+      ->method('hasKey')
+      ->will($this->returnCallback(function ($key) {
+        return $key == 'id';
+      }));
+    $entity_manager->expects($this->atLeastOnce())
+      ->method('getDefinition')
+      ->with('test_entity_type')
+      ->will($this->returnValue($entity_type));
+
+    $connection = $this->getMockBuilder('Drupal\Core\Database\Connection')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $field_info = $this->getMockBuilder('\Drupal\field\FieldInfo')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $entity_storage = new ContentEntityDatabaseStorage($entity_type, $connection, $field_info);
+
+    $entity = $entity_storage->create();
+    $entity->expects($this->atLeastOnce())
+      ->method('id')
+      ->will($this->returnValue('foo'));
+
+    $this->assertInstanceOf('Drupal\Core\Entity\EntityInterface', $entity);
+    $this->assertSame('foo', $entity->id());
+    $this->assertTrue($entity->isNew());
+  }
+
 }