Problem/Motivation

Drupal 7 supports differing public and private file directories. The file source plugin has a single source_base_path, so a given file migration can only migrate one or the other. The front end can easily instantiate two file migrations, one for public files and one for private files, with distinct source_base_paths, but each needs a different condition on the query (e.g., uri LIKE 'public://%').

Proposed resolution

Add an option to the file source plugin allowing filtering. To help support other file schemes from contrib, I would suggest it take a string value for the scheme to filter on ('public', 'private', 'youtube', etc.).

Remaining tasks

Submit a patch.

User interface changes

N/A

API changes

New configuration option on file source plugins.

Data model changes

N/A

CommentFileSizeAuthor
#19 2547125-19.patch4.08 KBphenaproxima
#16 interdiff-2547125-8-16.txt1.27 KBphenaproxima
#16 2547125-16.patch4.03 KBphenaproxima
#8 2547125-8.patch3.31 KBphenaproxima
#6 2547125-6.patch1.38 KBphenaproxima
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan created an issue. See original summary.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Issue tags: +Migrate D7 critical

It's not currently possible to import private files in D7 without this, so I believe that makes this a Migrate D7 critical.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Issue tags: +Barcelona2015, +Novice

Tagging for Barcelona sprinting.

adooo’s picture

adooo CreditAttribution: adooo as a volunteer commented

I'll try this one!

adooo’s picture

adooo CreditAttribution: adooo as a volunteer commented

Couldn't figure out how to simulate migrations...
Someone else can try this one :)

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +Needs tests
FileSize
2547125-6.patch1.38 KB

Initial patch; the unit tests will need to be updated.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Active » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: -Needs tests
FileSize
2547125-8.patch3.31 KB

Added a unit test.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Issue tags: -Barcelona2015, -Novice

What's here looks good. But, I'd like to see the d7_file tests trying this out - adding at least one private:// to the dumps, and verifying that migrations passing scheme: public:// and scheme: private:// to the source plugin actually get the right rows from the source table.

Status: Needs review » Needs work

The last submitted patch, 8: 2547125-8.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review

Uh...what? No it didn't. I see green. What are you smoking, testbot? Too much green, perhaps?

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs review » Reviewed & tested by the community

Made a first pass at implementing private file migration in the drush migrate-upgrade command at https://www.drupal.org/node/2505283#comment-10366951, this patch works fine. I'm not going to insist on the additional tests, ready to go for me.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Reviewed & tested by the community » Needs review

Hrrm... I'm realizing that my patch partly worked by accident (because I was migrating my private files directly from their location on the source site). Not a bug in this patch, but I think we may need to refactor some of the existing stuff that constructs file paths a bit to fully support private files.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs review » Needs work

My problem was:

    $path = str_replace($this->migration->get('destination.source_base_path'), NULL, $path);

Can you spot it? I knew you could... get() isn't going to dig into child fields, so get('destination.source_base_path') always returns NULL (and my source_base_path doesn't get stripped, so it gets added again by the destination). We need to get the destination then extract the source_base_path.

This is a pre-existing bug, but let's fix that in this patch, since it's necessary to making private files work.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented

By the way, it's horrifying that the source plugin is extracting stuff from the destination configuration, but fixing that is more refactoring than we can deal with here...

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
FileSize
2547125-16.patch4.03 KB
interdiff-2547125-8-16.txt1.27 KB

How's this?

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented 
+++ b/core/modules/file/src/Plugin/migrate/source/d7/File.php
@@ -69,7 +89,8 @@ public function prepareRow(Row $row) {
+    // @todo Don't depend on destination configuration.
+    $path = str_replace($this->migration->get('destination')['source_base_path'], NULL, $path);

Needs an issue

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented

Rest looks good, RTBC once we have a link to that issue I mentioned in #17

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Related issues: +#2577871: d7_file plugin should receive source_base_path configuration option
FileSize
2547125-19.patch4.08 KB

Added a link to #2577871: d7_file plugin should receive source_base_path configuration option.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks good, and works with the patch at #2505283: Handle import of private files.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Maybe let's get a follow-up to explicitly test a file with both a private:// and "flickr://" (or something) scheme, but overall this looks great.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 807e568 on 
    Issue #2547125 by phenaproxima, mikeryan: D7 file migration should allow...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.