Problem/Motivation

Import doesn't overwrite files. Because it doesn't overwrite, it leads to files proliferating like rabbits. And when later running drush decm example_module, all the exported files are obviously disambiguated with a sequential number. i.e. public://2021-02/officer_1.jpg when originally it was just officer.jpg. And I have to git checkout content/file/*.

Steps to reproduce

See problem description.

Proposed resolution

When importing files, always force overwrite.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#27 default_content-3200212-27.patch2.29 KBstephane aimar
#25 default_content-3200212-25.patch2.2 KBgrimreaper
#20 default_content-3200212-20.patch5.14 KBgrimreaper
#8 default_content-3200212.patch1.85 KBandrea.cividini

Issue fork default_content-3200212

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

heddn created an issue. See original summary.

herved made their first commit to this issue’s fork.

herved opened merge request !8

herved’s picture

herved commented

Hi, here's a first attempt which should fix the issue.
Should we add tests?

PS: Sorry I'm a bit lost with d.org patches vs merge requests.
Are all projects using gitlab's merge requests now?

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Active » Reviewed & tested by the community

This works. And there's test coverage. So LGTM.

kevinfunk’s picture

kevinfunk
he/him
commented

Thanks @herved. Tested and fixes the issues with files not being overridden.

lisa.rae’s picture

lisa.rae commented
Related issues: +#2698425: Do not re-import existing entities
andrea.cividini’s picture

andrea.cividini commented
StatusFileSize
new default_content-3200212.patch1.85 KB

I'm just attaching the patch here for those who may need it.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Title: Import doesn't overwrite files » Import should overwrite files
socialnicheguru’s picture

socialnicheguru commented

is this a duplicate of this issue as it is overwriting also, https://www.drupal.org/project/default_content/issues/2698425 ?

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

This is pretty tightly scoped to something different. It isn't about overwriting entities at all. You could even drop the DB and still experience this issue. Which is how I normally see it. I drop the db and reinstall the site and import default content. Without this patch, the files do not overwrite and I start seeing file_1.txt, file_2.txt, etc. This means that when I run the drush command to update the default content, the file entities always result in "changes" and all the files are named with some high numeric suffix.

#2698425: Do not re-import existing entities is at the entity level and is specific to re-running the installation of default on the same DB.

Rajeshreeputra made their first commit to this issue’s fork.

rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented

+1 RTBC

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Reviewed & tested by the community » Needs review

There's new patch after RTBC and MR

Can we make it configurable?

rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented

sure, will test this again and post the results

omarlopesino made their first commit to this issue’s fork.

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
Status: Needs review » Reviewed & tested by the community

AFAICT the proposed changes here are identical to the changes proposed as of #5, at which point there was test coverage and the behaviour was more consistent. I think this is good to merge.

This appears to work as it says on the tin, which is good - it eliminates a "round trip" issue where executing drush dcim example will generate a new filename for any files which already exist in the destination location, after which a drush dcem example will update the default content exports to append a _0 suffix ... then _1, then ...

👍

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Needs work

To early to require 10.3.

grimreaper made their first commit to this issue’s fork.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
StatusFileSize
new default_content-3200212-20.patch5.14 KB

Hi,

MR rebased to have a patch appliable on the 2.0.0-alpha3.

grimreaper changed the visibility of the branch 3200212-rebased-2.0.0-alpha3 to hidden.

grimreaper changed the visibility of the branch 3200212-rebased-2.0.0-alpha3 to active.

grimreaper opened merge request !54

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hum, strange, MR is rebased, I also tried another one with only one commit. But patch is not applicable

Idea, I think it is because of changes in .info.yml

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
StatusFileSize
new default_content-3200212-25.patch2.2 KB

Now applicable.

byrond’s picture

byrond commented

I don't think this sufficiently tests that additional files were not created:

    // Assert the files, since a file already existed at that location, one has
    // been overwritten.
    $files = \Drupal::entityTypeManager()->getStorage('file')->loadByProperties(['filename' => 'test-file.txt']);
    $this->assertCount(1, $files);

The file is created during the test with file_put_contents(), so an entity for it won't exist in the database. To confirm this, I removed all but the test from the patch, and it didn't fail.

Never mind. I tried on a clean site, and the patch does fail when the patch is not present and passes when it is. I misunderstood how the test was working.

stephane aimar’s picture

stephane aimar commented
StatusFileSize
new default_content-3200212-27.patch2.29 KB

2.0.0-beta1 patch compatibility

rosk0 made their first commit to this issue’s fork.

rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented
Status: Needs work » Reviewed & tested by the community

I believe it is fine to require 10.3 now, so back to RTBC based on #17.