Problem/Motivation

This issue is about the text on the credential form, /upgrade/credentials.

This is a follow up to #2925899: MigrateUpgradeImportBatch does not use source_private_file_path & source_base_path correctly, making it impossible to have public & private files in separate locations.
In that issue benjifisher stated,

The current form suggests (at least to me) that the files will be directly under bar/, when they are actually under bar/sites/default/files/.

Let's review the help text and make sure it is clear to everyone where the files need to be. Use the description in the makeFiles method from the patch in 2925899-Comment#39 for inspiration.

For reference here are the current descriptions.

When the source site is Drupal 6:
Files directory
To import files from your current Drupal site, enter a local file directory containing your site (e.g. /var/www/docroot), or your site address (for example http://example.com).

When the source site is Drupal 7:
Public files directory
To import public files from your current Drupal site, enter a local file directory containing your site (e.g. /var/www/docroot), or your site address (for example http://example.com).
Private files directory
To import private files from your current Drupal site, enter a local file directory containing your site (e.g. /var/www/docroot).

Proposed resolution

Leave the help text unchanged, but use the following text for the field labels:

  • Drupal 6 source: "Document root for files"
  • Drupal 7 source: "Document root for public files" and "Document root for private files"

Remaining tasks

User interface changes

On the credential form (/upgrade/credentials):

Before

Drupal 6
before -Drupal 6
Drupal 7

After

Drupal 6

Drupal 7

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Issue tags: +Migrate UI

Add tag

tonytheferg’s picture

Yeah I ran into this. The difficulty is people will default to thinking they should provide sites/default/files portion of the path. Maybe a note to make sure they don't include that would be helpful.

Here is a Q&A I posted on Drupal Answers

https://drupal.stackexchange.com/a/284012/93997

benjifisher’s picture

Issue summary: View changes

I think the current help text (description) is clear enough. What I find confusing are the labels: "Public files directory" and "Private files directory". (I am adding the missing word "directory" to both in the issue summary.)

One suggestion: change those to "Document root for public files" and "Document root for private files".

tonytheferg’s picture

quietone’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.88 KB
19.76 KB
10.65 KB
20.1 KB

Adding patch and screenshots.

Status: Needs review » Needs work

The last submitted patch, 6: 3151360-6.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
3 KB

Fixed the D6 string to not use 'public'. Added fix for the tests. No interdiff because this is small and it would be noise.

tonytheferg’s picture

Looks better!

benjifisher’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
15.48 KB
27.99 KB
16.01 KB
28.66 KB

@ToneLoc:

Thanks for the feedback!

I agree, the wording is an improvement. The code change is simple, just text changes to the form and the related tests. RTBC.

I am replacing the screenshots with new ones using the Seven theme, and adding a "before" screenshot for a Drupal 6 source.

benjifisher’s picture

People who run migrations with drush can use drush mmsg ... and figure out from the logged messages where the files are supposed to be. Until #3063856: Add ability to view migrate_message table data is fixed, there is no way to get that information for people who run migrations through the UI.

That makes it especially important for this UI to be clear.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Here's a question - how and when is the document root for private and public files going to be different? I don't understand how providing the document root to the private files is going to work?

+1 though to moving away from the existing terminology because it definitely makes you think you need to put your public and private directories from the file system admin page in here.

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

That is a good question. Another good question: why should we stage files for migration in a directory structure that looks like the source?

When I prepare a migration, I typically download all the files from the old site and put them ... somewhere on the site I am building. It would be convenient if I could use (relative to the docroot) ../files/public/ and ../files/private/ or something similar. Instead, I use something like ../files/sites/default/files/ (for public files).

I was 90% serious in #2925899-33: MigrateUpgradeImportBatch does not use source_private_file_path & source_base_path correctly, making it impossible to have public & private files in separate locations:

I have been doing migrations for years. Some involved private files. I read the docs, and maybe I decided that the code was too complicated to figure out what was going on. By trial and error, with a little help from drush mmsg, I figured out that I had to put the private files in a particular place and I could skip that field on the credential form.

In the long run, I would like to change things to make it more convenient for myself: specify the path to public:// and the path to private:// and go back to the original labels. In other words, I should not have to create sites/default/files/ (for the public files). I have not thought about how that will look for D6 sources, and I have not thought about what has to change in the code to make that happen.

I have thought about how much of a change this will be, and that gives me a headache. Change record, release notes, automated tests. Maybe it is so disruptive that we should wait for Drupal 10.0.

For the current issue, I hope to keep the scope small: update the labels so that they better describe the way the system currently works.

benjifisher’s picture

alexpott’s picture

Title: Improve description for file paths on the CredentialFrom » [backport] Improve description for file paths on the CredentialFrom
Version: 9.1.x-dev » 9.0.x-dev
Category: Task » Bug report
Issue tags: +String change in 9.1.0

Okay I'm going to commit this to 9.1.x as it is a string change. My original reaction of "why are we deviating from the language used on the system files form?" shows the confusion is real.

I'm going to ping this to the other committers to discuss backport because I think the current labels are wrong enough to be considered a bug.

Committed 9805714 and pushed to 9.1.x. Thanks!

  • alexpott committed 9805714 on 9.1.x
    Issue #3151360 by quietone, benjifisher, ToneLoc: Improve description...
catch’s picture

This seems wrong enough to be considered a bug for backport, and it seems likely that some sites will still be trying to migrate to 8.9.x, at least until 9.1.x is released.

benjifisher’s picture

some sites will still be trying to migrate to 8.9.x, at least until 9.1.x is released

During one of my DrupalCon sessions today, the speaker asked who had migrated to D9. At least one response was "I am still waiting for a few modules that are not yet D9 compatible.".

alexpott’s picture

Title: [backport] Improve description for file paths on the CredentialFrom » Improve description for file paths on the CredentialFrom
Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Okay backported this to 8.9.x as per #17.

  • alexpott committed 8b04775 on 9.0.x
    Issue #3151360 by quietone, benjifisher, ToneLoc: Improve description...

  • alexpott committed dc10f8e on 8.9.x
    Issue #3151360 by quietone, benjifisher, ToneLoc: Improve description...

Status: Fixed » Closed (fixed)

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