Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
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 |
Comment | File | Size | Author |
---|---|---|---|
#77 | 2241865-77-9.1.x.patch | 4.05 KB | voleger |
Comments
Comment #1
sunThis temporary tweak should be removed as well:
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
Comment #2
tim.plunkettNot sure what to do about the two menu_link cases.
menu_link_rebuild_defaults explicitly bypasses entity_load:
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.
Comment #3
Xano2: entity-2241865-2.patch queued for re-testing.
Comment #5
XanoRe-roll.
Comment #6
Xano5: drupal_2241865_5.patch queued for re-testing.
Comment #7
Xano5: drupal_2241865_5.patch queued for re-testing.
Comment #11
BerdirThis 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.
Comment #12
tim.plunkettThis is just about file.module now, the rest have been refactored away.
Comment #13
BerdirIt is, so we just need a reroll of that part :)
Comment #14
BerdirComment #15
AjitSPlain reroll.
Comment #16
Berdirthis function does not exist anymore, remove it from the patch.
Comment #17
AjitSRemoved the function as suggested.
Comment #18
joshi.rohit100I think, we should use short array syntax.
Comment #19
AjitS@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.
Comment #20
joshi.rohit100That 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.
Comment #21
jcnventura CreditAttribution: jcnventura commentedIf 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 []
Comment #22
trevjs CreditAttribution: trevjs commentedComment #23
trevjs CreditAttribution: trevjs commentedThough 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...
Comment #24
tim.plunkettNo reason to do that here, we're not adding new calls to drupal_basename(), just moving/indenting them.
Comment #25
jmarkel CreditAttribution: jmarkel as a volunteer commentedComment #26
JulienF CreditAttribution: JulienF commentedHey 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
Comment #29
geertvd CreditAttribution: geertvd at XIO commentedReroll
Comment #30
NitebreedAs 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.
Comment #31
NitebreedI 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.Comment #32
NitebreedComment #33
YesCT CreditAttribution: YesCT commentedComment #34
alexpottIs this testable? As a bug it sounds like it should be.
Comment #35
BerdirI 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.
Comment #38
BerdirSetting back to RTBC, was before, just a failed testrun.
Comment #40
AjitSSetting to RTBC. Failure was at CI level.
Comment #41
alexpottIt'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
Comment #42
catchSorry 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.
Comment #43
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWrote some test coverage. The test only patch is also the interdiff :)
Comment #49
cweagansLGTM.
Comment #50
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAfter all these months, the patch needed a serious reroll :)
Comment #51
cweagansHm. 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.
Comment #52
alexpottI think we're missing test coverage of the changes to file_copy()
Should we also not change the UID? I guess not. Since that's not what we do for a node.
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.Comment #56
phenaproximaRerolled against 8.7.x.
Comment #57
phenaproximaAh, forgot to remove some stray lines. Sorry about that!
Comment #58
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedBy comparing the patch from #50 with #57, it seems that we also need to remove the following lines from
file_copy
:Comment #59
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWe also need to use
$file_system->basename()
here.Comment #60
phenaproximaFixed #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.
Comment #63
phenaproximaTook a shot at addressing #52.1.
Comment #64
BerdirThis 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.
"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.
Comment #65
BerdirComment #67
damontgomery CreditAttribution: damontgomery at Palantir.net commentedThe 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.
Comment #68
EclipseGc CreditAttribution: EclipseGc commentedAcquia's ContentHub 2.x has a bug that relates to this issue #3075535: Errors on inserting duplicate entries into acquia_contenthub_publisher_export_tracking table.
Comment #69
imclean CreditAttribution: imclean at Digital Ink commentedHere's an 8.9.x version of #63. The interdiff failed for some reason but the changes are:
loadByProperties()
instead ofentity_load_multiple_by_properties()
So essentially just
file.module
. (Why is so much happening in this file instead of\Drupal\Core\File\FileSystemInterface
or elsewhere?)Comment #71
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere this patch might fix failed tests.
Comment #72
imclean CreditAttribution: imclean at Digital Ink commentedI missed one. Thanks.
Comment #73
imclean CreditAttribution: imclean at Digital Ink commentedHere's a problem. There's no way to pass a file exists (
$replace
) option tofile_managed_save_upload()
.The called internal function therefore uses the default:
Comment #74
imclean CreditAttribution: imclean at Digital Ink commentedThis 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.Comment #75
imclean CreditAttribution: imclean at Digital Ink commentedComment #77
volegerJust reroll
Comment #80
hkirsman CreditAttribution: hkirsman commentedIn 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?
Comment #81
BerdirWhy 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.
Comment #82
hkirsman CreditAttribution: hkirsman commentedNeed to overwrite the file if it's the same entity but I guess it could be fetched from the file entity.
Comment #83
larowlanClosed 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
Comment #84
Gung Wang CreditAttribution: Gung Wang commentedThe patch 2241865-77-9.1.x.patch is not working on Drupal 9.3.9