Problem/Motivation

#2241655: EntityStorageInterface::create() should always create a "new" entity revealed some places where we have very interesting legacy code (and one new one!)

Part of this code can be found in file_copy() and file_save_data(), where a file or entity would always be created created even it already exists.

Proposed resolution

Stop doing that, in any way possible.

Remaining tasks

Find a way

User interface changes

API changes

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions
CommentFileSizeAuthor
#77 2241865-77-9.1.x.patch4.05 KBvoleger
#71 interdiff_69-71.txt816 bytesravi.shankar
#71 2241865-71.patch4.05 KBravi.shankar
#69 2241865-69.patch4.02 KBimclean
#63 interdiff-2241865-60-63.txt2.38 KBphenaproxima
#63 2241865-63.patch9.38 KBphenaproxima
#60 interdiff-2241865-57-60.txt2.54 KBphenaproxima
#60 2241865-60.patch7.3 KBphenaproxima
#57 interdiff-2241865-56-57.txt695 bytesphenaproxima
#57 2241865-57.patch5.62 KBphenaproxima
#56 2241865-56.patch5.67 KBphenaproxima
#51 file_uuid_test.tgz684 bytescweagans
#50 2241865-50.patch5.56 KBamateescu
#43 2241865-43.patch5.56 KBamateescu
#43 2241865-43-test-only.patch1.77 KBamateescu
#29 do_not_create_a_new-2241865-29.patch3.79 KBgeertvd
#21 do_not_create_a_new-2241865-21.patch3.78 KBjcnventura
#17 do_not_create_a_new-2241865-17.patch3.8 KBAjitS
#17 interdiff.txt1.13 KBAjitS
#15 do_not_create_a_new-2241865-15.patch4.92 KBAjitS
#5 drupal_2241865_5.patch4.75 KBXano
#2 entity-2241865-2.patch4.75 KBtim.plunkett
#2 entity-2241865-2-do-not-test.patch4.31 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +API clean-up

This temporary tweak should be removed as well:

   public function setOriginalId($id) {
...
+    // If the specified ID is anything except NULL, this should mark this entity
+    // as no longer new.
+    if ($id !== NULL) {
+      $this->enforceIsNew(FALSE);
+    }

Ideally, we should replace it with an exception, because that case makes no logical sense; cf.

#2241655-11: EntityStorageInterface::create() should always create a "new" entity

tim.plunkett’s picture

Status: Active » Needs review
FileSize
4.31 KB
4.75 KB

Not sure what to do about the two menu_link cases.
menu_link_rebuild_defaults explicitly bypasses entity_load:

      // For performance reasons, do a straight query now and convert to a menu
      // link entity later.

MenuLink::reset looks like we could just overwrite specific properties of the link (represented by $this), but I'm not sure.

file_save_data and file_copy were easy enough, having DUTB tests helped.
file_save_upload is a bit more messy, I didn't get into that.

And comment_prepare_author seemed easy enough.

Xano’s picture

2: entity-2241865-2.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2: entity-2241865-2.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
4.75 KB

Re-roll.

Xano’s picture

5: drupal_2241865_5.patch queued for re-testing.

Xano’s picture

5: drupal_2241865_5.patch queued for re-testing.

Xano queued 5: drupal_2241865_5.patch for re-testing.

Xano queued 5: drupal_2241865_5.patch for re-testing.

Xano queued 5: drupal_2241865_5.patch for re-testing.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.module
@@ -1286,11 +1286,9 @@ function comment_prepare_author(CommentInterface $comment) {
-    // @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);
+    $account = user_load(0);
+    $account->setUsername($comment->getAuthorName());
+    $account->homepage = $comment->getHomepage();
   }
   return $account;

This will likely not work correctly if you have multiple comments on a page from different anonymous authors. Make sure to clear the render cache, so all comments are rendered together. I'd expect weird stuff to happen with the displayed author name.

Maybe it is fine because nothing else happens between this and actually rendering out the comment, but because is by reference, we're always changing the same, statically cached object.

=> clone it.

That said, I think this might be a considerable performance improvement because entity_create() is dead-slow for content entities.

tim.plunkett’s picture

This is just about file.module now, the rest have been refactored away.

Berdir’s picture

Issue tags: +Needs reroll

It is, so we just need a reroll of that part :)

Berdir’s picture

Issue tags: +Novice
AjitS’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.92 KB

Plain reroll.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.module
@@ -598,6 +598,29 @@ function comment_preprocess_block(&$variables) {
+ *
+ * This helper handles anonymous authors in addition to registered comment
+ * authors.
+ *
+ * @param \Drupal\comment\CommentInterface $comment
+ *   The comment to which the author replied.
+ *
+ * @return \Drupal\user\UserInterface
+ *   A user account, for use with theme_username() or the user_picture template.
+ */
+function comment_prepare_author(CommentInterface $comment) {
+  // The account has been pre-loaded by CommentViewBuilder::buildComponents().
+  $account = $comment->getOwner();
+  if (empty($account->uid->value)) {
+    $account = user_load(0);
+    $account->setUsername($comment->getAuthorName());
+    $account->homepage = $comment->getHomepage();
+  }

this function does not exist anymore, remove it from the patch.

AjitS’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
3.8 KB

Removed the function as suggested.

joshi.rohit100’s picture

I think, we should use short array syntax.

AjitS’s picture

@joshi.rohit100 : I think that should be handled in a separate issue? I quickly checked the file and found that there are other places, apart from the ones modified in this patch, which has conventional array syntax.

joshi.rohit100’s picture

That was just my opinion. As we are already heading towards short array syntax, so I think it is better if we use in changes. Again just a thought.

jcnventura’s picture

FileSize
3.78 KB

If you're rerolling the patch, might as well reroll it with short array syntax. And the current patch policy is to slowly deploy the short array with each new patch.

I'm skipping the interdiff, as the only changes were really to replace array() with []

trevjs’s picture

Assigned: Unassigned » trevjs
trevjs’s picture

Assigned: trevjs » Unassigned
Status: Needs review » Needs work

Though it is a handy short cut, drupal_basename($destination) is marked as deprecated, and so it seems to me that we should probably replace all instances of it with "\Drupal::service('file_system')->basename($uri, $suffix);"

See https://api.drupal.org/api/drupal/core!includes!file.inc/function/drupal...

tim.plunkett’s picture

Status: Needs work » Needs review

No reason to do that here, we're not adding new calls to drupal_basename(), just moving/indenting them.

jmarkel’s picture

Issue summary: View changes
JulienF’s picture

Hey There,

I'm willing to help here (participating in the LA mentored core sprint) but after scratching my head I can't find a way to actually test that.

Do you specific steps to follow to be able to reproduce ?

Thanks

Status: Needs review » Needs work

The last submitted patch, 21: do_not_create_a_new-2241865-21.patch, failed testing.

geertvd’s picture

Status: Needs work » Needs review
FileSize
3.79 KB

Reroll

Nitebreed’s picture

Issue tags: -Novice

As a part of DrupalCon Barcelona I judged the Novice task together with my mentor legolasbo. Issues was tagged Novice because it needed a reroll. This is already done. I can still apply the latest patch to HEAD, therefor I'm removing the novice tag.

Nitebreed’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed the latest patch with my mentor and we both think this patch fundamentally doesn't change any behaviour except it removes the unnecessary entity_create() and $source->createDuplicate() calls. Therefor we believe this patch fixes the issue.

Nitebreed’s picture

Issue summary: View changes
YesCT’s picture

Issue tags: +Barcelona2015
alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

Is this testable? As a bug it sounds like it should be.

Berdir’s picture

Category: Bug report » Task
Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I don't think so. This is just clean-up and making the code a bit more performant. Changing to a normal task, I think it was a bigger problem initially but various things have been cleaned up in other issues.

If someone disagrees, feel free to set it back.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: do_not_create_a_new-2241865-29.patch, failed testing.

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC, was before, just a failed testrun.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: do_not_create_a_new-2241865-29.patch, failed testing.

AjitS’s picture

Status: Needs work » Reviewed & tested by the community

Setting to RTBC. Failure was at CI level.

alexpott’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +needs profiling

It'd be great to have some idea of the performance benefit. Based on @xjm's post release triage document and given that this is not fixing a bug I think the next available release for this is 8.1.x

catch’s picture

Status: Reviewed & tested by the community » Needs review

Sorry to bump this back after a long time at RTBC, but this looks testable to me - hook_entity_create() ought to fire with the existing code, and not fire after the patch.

amateescu’s picture

Wrote some test coverage. The test only patch is also the interdiff :)

The last submitted patch, 43: 2241865-43-test-only.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cweagans’s picture

Status: Needs review » Reviewed & tested by the community

LGTM.

amateescu’s picture

After all these months, the patch needed a serious reroll :)

cweagans’s picture

FileSize
684 bytes

Hm. Could have sworn the patch was recently tested. Maybe I was looking at something else.

Anywho, looks like just some line number changes. The meat of it didn't change, so I'm still happy with the patch. Attached is a test module - before the patch, refreshing /file_uuid_test repeatedly results in different UUIDs every time. After the patch, the UUID stays the same as expected.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/file/file.module
    +++ b/core/modules/file/file.module
    @@ -151,25 +151,21 @@ function file_copy(FileInterface $source, $destination = NULL, $replace = FILE_E
    

    I think we're missing test coverage of the changes to file_copy()

  2. +++ b/core/modules/file/file.module
    @@ -509,28 +505,23 @@ function file_save_data($data, $destination = NULL, $replace = FILE_EXISTS_RENAM
    -    $file = File::create([
    -      'uri' => $uri,
    -      'uid' => $user->id(),
    -      'status' => FILE_STATUS_PERMANENT,
    -    ]);
    ...
    +      $file = reset($existing_files);
    +      $file->setPermanent();
    

    Should we also not change the UID? I guess not. Since that's not what we do for a node.

  3. +++ b/core/modules/file/tests/file_test/file_test.module
    @@ -349,3 +349,10 @@ function file_test_entity_type_alter(&$entity_types) {
    +/**
    + * Implements hook_ENTITY_TYPE_create() for file entities.
    + */
    +function file_test_file_create() {
    +  \Drupal::state()->set('file_test.hook_entity_create', TRUE);
    +}
    
    +++ b/core/modules/file/tests/src/Kernel/SaveDataTest.php
    @@ -93,9 +93,13 @@ public function testExistingReplace() {
    +    \Drupal::state()->set('file_test.hook_entity_create', FALSE);
         $result = file_save_data($contents, $existing->getFileUri(), FILE_EXISTS_REPLACE);
         $this->assertTrue($result, 'File saved successfully.');
     
    +    $entity_create_hook_fired = \Drupal::state()->get('file_test.hook_entity_create');
    +    $this->assertFalse($entity_create_hook_fired, 'Entity create hook was not invoked for a file replace operation.');
    

    I would be great to have a positive test case to prove that file_test.hook_entity_create gets set to TRUE when expected to. Since ATM this test will pass if the hook was misnamed.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
5.67 KB

Rerolled against 8.7.x.

phenaproxima’s picture

Ah, forgot to remove some stray lines. Sorry about that!

amateescu’s picture

By comparing the patch from #50 with #57, it seems that we also need to remove the following lines from file_copy:

    $file = $source->createDuplicate();
    $file->setFileUri($uri);
    $file->setFilename($file_system->basename($uri));
amateescu’s picture

+++ b/core/modules/file/file.module
@@ -173,22 +173,21 @@ function file_copy(FileInterface $source, $destination = NULL, $replace = FILE_E
+        $file->setFilename(drupal_basename($uri));

We also need to use $file_system->basename() here.

phenaproxima’s picture

Fixed #58 and #59, and added a new test case per #52.3, asserting that a file entity is created if we try to replace a non-existing file.

The last submitted patch, 57: 2241865-57.patch, failed testing. View results

The last submitted patch, 56: 2241865-56.patch, failed testing. View results

phenaproxima’s picture

Took a shot at addressing #52.1.

Berdir’s picture

Title: Do not create a new entity in order to overwrite an existing entity » Do not create a new file entity in order to overwrite an existing entity
Component: entity system » file system
Status: Needs review » Needs work
Issue tags: -needs profiling

This conflicts/overlaps with #3027178: Properly deprecate entity_load_multiple_by_properties(), so lets remove those deprecated function calls it it will be easier to reroll if this lands first.

+++ b/core/modules/file/tests/src/Kernel/SaveDataTest.php
@@ -88,4 +89,29 @@
   /**
+   * Test file_save_data() when replacing an non-existing file.
+   */
+  public function testNonExistingReplace() {
+    $contents = $this->randomMachineName(8);
+

"replacing a non-existing" file isn't really a thing because then we don't actually do any replacing :)

"Test file_save_data() with a non-existing destination" or so?

Which we're actually testing already, so why not just extend testWithFilename in this class with the extra assertion?

I also don't see anything here that would need profiling.

Berdir’s picture

Component: file system » file.module

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

damontgomery’s picture

The patch in #63 does not apply to Drupal 8.8.0. Maybe because of the related issue, https://www.drupal.org/project/drupal/issues/3027178, that Berdir mentioned in #64.

EclipseGc’s picture

imclean’s picture

Status: Needs work » Needs review
FileSize
4.02 KB

Here's an 8.9.x version of #63. The interdiff failed for some reason but the changes are:

  1. Offset changes
  2. Use loadByProperties() instead of entity_load_multiple_by_properties()
  3. No tests. New tests have been added since #63 which might cover some of this.

So essentially just file.module. (Why is so much happening in this file instead of \Drupal\Core\File\FileSystemInterface or elsewhere?)

Status: Needs review » Needs work

The last submitted patch, 69: 2241865-69.patch, failed testing. View results

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
4.05 KB
816 bytes

Here this patch might fix failed tests.

imclean’s picture

I missed one. Thanks.

imclean’s picture

Here's a problem. There's no way to pass a file exists ($replace) option to file_managed_save_upload().

function file_managed_file_save_upload($element, FormStateInterface $form_state) {
  // ...
  if ($files_uploaded) {
    if (!$files = _file_save_upload_from_form($element, $form_state)) {
      \Drupal::logger('file')->notice('The file upload failed. %upload', ['%upload' => $upload_name]);
      return [];
    }
    //...
  }
  //...
}

The called internal function therefore uses the default:

function _file_save_upload_from_form(array $element, FormStateInterface $form_state, $delta = NULL, $replace = FileSystemInterface::EXISTS_RENAME) {
  // ...
}
imclean’s picture

This default ($replace = FileSystemInterface::EXISTS_RENAME) then gets passed down as an "option" to a chain of internal functions with no opportunity to actually change it.

imclean’s picture

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

hkirsman’s picture

In our contrib module we are generating path with createFilename(). It has hard-coded functionality to append number at the end if file exists. Shouldn't this method also have the $replace parameter?

Berdir’s picture

Why would you use createFilename() if not to have it return a unique filename? See \Drupal\Core\File\FileSystem::getDestinationFilename(), the only case core uses it outside of tests is if RENAME was provided and the file exists.

hkirsman’s picture

Need to overwrite the file if it's the same entity but I guess it could be fetched from the file entity.

larowlan’s picture

Status: Needs review » Closed (duplicate)

Closed this as a duplicate of #3223209: deprecate file_save_data, file_copy and file_move and replace with a service and transferred credit over there

Gung Wang’s picture

The patch 2241865-77-9.1.x.patch is not working on Drupal 9.3.9

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-02-14/2241865-71.patch
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-09-11/2241865-77-9.1.x.patch