Skip to content

Feature/dev 978 volume fs subpaths#12173

Closed
i-just wants to merge 1074 commits into5.0from
feature/dev-978-volume-fs-subpaths
Closed

Feature/dev 978 volume fs subpaths#12173
i-just wants to merge 1074 commits into5.0from
feature/dev-978-volume-fs-subpaths

Conversation

@i-just
Copy link
Contributor

@i-just i-just commented Oct 21, 2022

Description

  • ability to use the same filesystem for multiple volumes
  • ability to specify filesystem subpath (fsSubpath) on per-volume basis

Additional info:

  • changing fsSubpath for volume doesn't automatically change the folder's name and it doesn't update child folder paths (just like changing FS base path doesn't do those things)
  • volume fsSubpath is factored into creating/renaming subfolders in Assets and creating/editing Asset Fields
  • ensures unique paths across all volumes that use the same FS

Related issues

DEV-978
#11044

@i-just i-just requested a review from a team as a code owner October 21, 2022 10:51
@linear
Copy link

linear bot commented Oct 21, 2022

DEV-978 Volume FS subpaths

Volumes should be able to choose a subpath within their selected filesystem, so filesystems can be reused across multiple volumes.

This would also mean that multiple volumes should be able to select the same filesystem.

@brandonkelly brandonkelly changed the base branch from develop to 4.4 January 4, 2023 23:10
Copy link
Member

@brandonkelly brandonkelly left a comment

Choose a reason for hiding this comment

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

@i-just The current implementation is dropping any inner slashes in the Filesystem Subpath. For example if you set it to foo/bar, it will end up getting normalized to foobar/.

I don’t think that the subpath should be stored within the volumefolders table; instead we should add a new fsSubpath column to the volumes table and store it there. (This will require adjusting the Install migration, as well as creating a new migration for existing installs + increasing the schemaVersion to 4.4.0.2 in config/app.php.)

Folder paths stored in volumefolders should not be affected by the selected subpath. They should continue to be relative to the volume root, not the filesystem root. So if a volume’s Filesystem Subpath is set to foo/bar, any folders (and assets) within the volume should have paths relative to foo/bar/.

@brandonkelly
Copy link
Member

Also, it would be good if volume validation made sure you don’t have a subpath that will conflict with another volume.

For example, if Volume A uses $MY_FS and its subpath is set to foo/bar, and Volume B wishes to also use $MY_FS and its subpath is either empty, or set to foo, foo/bar, or foo/bar/baz, it should result in a validation error due to the conflict with Volume A. The only acceptable subpath for Volume B would be a value that starts with something besides foo/.

@i-just i-just marked this pull request as draft January 5, 2023 09:48
@i-just
Copy link
Contributor Author

i-just commented Jan 6, 2023

Thanks @brandonkelly!

  • dropping inner slashes in the Filesystem Subpath => I got too eager with cleaning the path; it’s fixed now
  • subpath should be stored in the volumes table in a new column called fsSubpath (not in the volumefolders table) => agreed; I started with that and then backed out of it; it’s now implemented
  • there was already a per-volume subpath validation in place, but it was different to what you’re after, so I’ve adjusted to:
    • always require a fsSubpath if there’s already a volume using the same FS and that volume has a fsSubpath defined (otherwise, FS won’t show on the dropdown list)
    • make sure volumes using the same FS don’t have the fsSubpath that starts with the same directory

Another thing to point out is that the transform location doesn’t take the fsSubpath into consideration (intentionally). For example, if your FS base is uploads and the volume using that FS has a fsSubpath of images and transformSubpath is empty, then assets will be stored under uploads/images, but transforms will be under just uploads. It felt like it has more sense this way as you can specify the same dir for fsSubpath and transformSubpath rather than wonder how to get out of fsSubpath if it was automatically included in the transformSubpath.

@i-just i-just marked this pull request as ready for review January 6, 2023 15:38
@i-just i-just requested a review from brandonkelly January 6, 2023 15:39
brandonkelly and others added 20 commits January 31, 2023 10:58
….com:craftcms/cms into feature/dev-239-fill-image-transform-setting
…ftcms/cms into feature/volume-folder-nav-v4

# Conflicts:
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/cp/dist/css/cp.css
#	src/web/assets/cp/dist/css/cp.css.map
…ftcms/cms into feature/volume-folder-nav-v4

# Conflicts:
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
# Conflicts:
#	src/web/assets/admintable/dist/css/app.css.map
#	src/web/assets/admintable/dist/js/app.js
#	src/web/assets/admintable/dist/js/app.js.map
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/garnish/dist/garnish.js.map
#	src/web/assets/graphiql/dist/css/graphiql.css.map
#	src/web/assets/installer/dist/css/install.css.map
#	src/web/assets/login/dist/css/login.css.map
#	src/web/assets/login/dist/login.js.map
#	src/web/assets/pluginstore/dist/css/app.css.map
#	src/web/assets/pluginstore/dist/js/app.js
#	src/web/assets/pluginstore/dist/js/app.js.map
#	src/web/assets/routes/dist/css/routes.css.map
#	src/web/assets/systemmessages/dist/css/system_messages.css.map
#	src/web/assets/updates/dist/UpdatesUtility.js.map
#	src/web/assets/upgrade/dist/css/UpgradeUtility.css.map
…itch-columns

# Conflicts:
#	src/web/assets/cp/dist/css/cp.css
#	src/web/assets/cp/dist/css/cp.css.map
#	src/web/assets/craftsupport/dist/css/CraftSupportWidget.css.map
#	src/web/assets/dashboard/dist/css/Dashboard.css.map
#	src/web/assets/edittransform/dist/css/transforms.css.map
#	src/web/assets/edituser/dist/css/AccountSettingsForm.css.map
#	src/web/assets/edituser/dist/css/profile.css.map
#	src/web/assets/generalsettings/dist/css/rebrand.css.map
#	src/web/assets/graphiql/dist/css/graphiql.css.map
#	src/web/assets/installer/dist/css/install.css.map
#	src/web/assets/login/dist/css/login.css.map
#	src/web/assets/money/dist/css/Money.css.map
#	src/web/assets/plugins/dist/css/PluginManager.css.map
#	src/web/assets/pluginstore/dist/css/app.css.map
#	src/web/assets/pluginstore/dist/js/app.js
#	src/web/assets/pluginstore/dist/js/app.js.map
#	src/web/assets/pluginstoreoauth/dist/css/PluginStoreOauthCallback.css.map
#	src/web/assets/routes/dist/css/routes.css.map
#	src/web/assets/systemmessages/dist/css/system_messages.css.map
#	src/web/assets/updater/dist/css/Updater.css.map
#	src/web/assets/updates/dist/css/UpdatesUtility.css.map
#	src/web/assets/upgrade/dist/css/UpgradeUtility.css.map
#	src/web/assets/userpermissions/dist/css/UserPermissions.css.map
#	src/web/assets/utilities/dist/css/utilities.css.map
…lected

# Conflicts:
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/pluginstore/dist/js/app.js
#	src/web/assets/pluginstore/dist/js/app.js.map
…ns-button-for-selected

Convert element index actions menu into disclosure menu
# Conflicts:
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/cp/src/js/BaseElementIndex.js
…by-size

Add `width` and `height` to default asset sort
# Conflicts:
#	composer.lock
#	src/config/app.php
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
# Conflicts:
#	CHANGELOG-WIP.md
#	src/translations/ar/app.php
#	src/translations/cs/app.php
#	src/translations/da/app.php
#	src/translations/de-CH/app.php
#	src/translations/de/app.php
#	src/translations/en-GB/app.php
#	src/translations/es/app.php
#	src/translations/fa/app.php
#	src/translations/fr-CA/app.php
#	src/translations/fr/app.php
#	src/translations/he/app.php
#	src/translations/hu/app.php
#	src/translations/it/app.php
#	src/translations/ja/app.php
#	src/translations/ko/app.php
#	src/translations/nb/app.php
#	src/translations/nl/app.php
#	src/translations/pl/app.php
#	src/translations/pt/app.php
#	src/translations/ru/app.php
#	src/translations/sk/app.php
#	src/translations/sv/app.php
#	src/translations/th/app.php
#	src/translations/tr/app.php
#	src/translations/uk/app.php
#	src/translations/zh/app.php
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
[ci skip]
[ci skip]
…sion-notes-graphql

Surface `revisionNotes` to graphql
[ci skip]
# Conflicts:
#	src/config/app.php
#	src/models/VolumeFolder.php
#	src/services/AssetIndexer.php
Comment on lines +1058 to +1065
// make sure asset folderPath accounts for volume's fsSubpath
if (isset($config['volumeId'])) {
$volume = Craft::$app->getVolumes()->getVolumeById($config['volumeId']);
$fsSubpath = $volume?->getFsSubpath();
if (!empty($fsSubpath) && !str_starts_with($config['folderPath'], $fsSubpath)) {
$config['folderPath'] = $fsSubpath . $config['folderPath'];
}
}
Copy link
Member

@brandonkelly brandonkelly Feb 10, 2023

Choose a reason for hiding this comment

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

Similar to my previous comment – asset paths should be relative to the root of the volume (like folder paths), not to the root of the filesystem.

Copy link
Member

Choose a reason for hiding this comment

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

Basically, nothing about working with assets should reveal anything about the fact that a filesystem subpath was specified on the volume settings. All of the filesystem subpath logic should be completely contained to the volume itself.

@brandonkelly brandonkelly changed the base branch from 4.4 to 5.0 February 20, 2023 20:56
@i-just
Copy link
Contributor Author

i-just commented Feb 21, 2023

Closing this PR as I’ll be opening 2 new ones for this work. One for C4 (Volume to use FsInterface) and one for C5 where we actually add support for fsSubpath.

@i-just i-just closed this Feb 21, 2023
@brandonkelly brandonkelly deleted the feature/dev-978-volume-fs-subpaths branch September 27, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants