Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow multiple upload dirs, fixes #4190, fixes #4796, fixes #5047 #5005

Merged
merged 15 commits into from
Jul 3, 2023

Conversation

gilbertsoft
Copy link
Member

@gilbertsoft gilbertsoft commented Jun 21, 2023

The Issue

How This PR Solves The Issue

A new config property upload_dirs is introduced to hold more than one upload dir. Existing config upload_dir is migrated to the new config and removed from the projects afterwards. For project types which do not predefine a default upload dir like php the warning about the missing upload dir can be disabled by setting upload_dirs: false which is also mentioned in the warning message.

Manual Testing Instructions

  • upload_dir is migrated to upload_dirs
  • multiple dirs can be set
  • import-files works correctly when importing in a upload dir using the --target flag
  • configuration with ddev config works properly, using --upload-dirs and --uplod-dir flags

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

@github-actions
Copy link

github-actions bot commented Jun 21, 2023

@gilbertsoft gilbertsoft force-pushed the feature/multiple-upload-dirs branch 6 times, most recently from a30ae5f to 7d0a064 Compare June 21, 2023 11:58
@rfay rfay changed the title Feature allow multiple upload dirs Feature allow multiple upload dirs, fixes #4190, fixes #4796 Jun 21, 2023
@gilbertsoft gilbertsoft force-pushed the feature/multiple-upload-dirs branch 16 times, most recently from 0cc4086 to 096edd2 Compare June 27, 2023 19:18
@gilbertsoft gilbertsoft marked this pull request as ready for review June 28, 2023 07:35
@gilbertsoft gilbertsoft requested review from a team as code owners June 28, 2023 07:35
@gilbertsoft gilbertsoft changed the title Feature allow multiple upload dirs, fixes #4190, fixes #4796 feat: allow multiple upload dirs, fixes #4190, fixes #4796 Jun 28, 2023
@gilbertsoft gilbertsoft changed the title feat: allow multiple upload dirs, fixes #4190, fixes #4796 feat: allow multiple upload dirs, fixes #4190, fixes #4796, fixes #5047 Jun 30, 2023
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the permutations I've tried now seem to work just fine, looking great, thanks!

@rfay
Copy link
Member

rfay commented Jun 30, 2023

We can ask the Craft CMS folks to test the prerelease, doesn't have to be done before this is merged.

@gilbertsoft
Copy link
Member Author

@rfay
Copy link
Member

rfay commented Jun 30, 2023

From @mattstein about how to test Craft CMS upload_dir:

I never used the upload_dir feature because it seemed kind of old school and I wasn’t sure it’d get me anything. Craft’s editor-managed files are called Assets, and Assets live in Volumes that sit on top of filesystems that can be local folders, object storage, or anything with a Flysystem adapter.

I’m not sure what the feature should do, but to test it with Craft—assuming local Asset Volumes with locally-stored files—you’d want to create a handful of Volumes + filesystems and visit Assets in the control panel to be sure you can upload files to each one.

https://craftcms.com/docs/4.x/assets.html#volumes

If you sync new files into the local folders, or delete any, you’ll need to have Craft update its indexes:

https://craftcms.com/docs/4.x/assets.html#updating-asset-indexes

@gilbertsoft gilbertsoft force-pushed the feature/multiple-upload-dirs branch 3 times, most recently from 1515cc0 to 7cf92c1 Compare July 1, 2023 17:41
@gilbertsoft
Copy link
Member Author

Investigated the failure and the reason was that some of the tests use an empty docroot which made the new ../private upload dir for Drupal invalid. Therefor added a validation of the upload dirs to be located in the project root or an error is shown. Last commit disabled the private dir again like discussed.

@rfay rfay force-pushed the feature/multiple-upload-dirs branch from 55b6d63 to 3e6a45a Compare July 3, 2023 02:29
@rfay
Copy link
Member

rfay commented Jul 3, 2023

Rebased to re-run tests and fixed linting error in drupal.go.

Recommend using pre-push hook, https://github.com/ddev/ddev/tree/master/.githooks - maybe we don't have that in the docs?

@rfay rfay merged commit 9b3b0d9 into ddev:master Jul 3, 2023
20 checks passed
@rfay rfay deleted the feature/multiple-upload-dirs branch July 3, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants