NEW Legacy thumbnail migration task #8924
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Apr 15, 2019
chillu
force-pushed
the
pulls/4/thumbnail-helper
branch
2 times, most recently
from
a55d0e6 to
a0c0e23
Compare
bergice
previously requested changes
Apr 17, 2019
bergice
reviewed
Apr 17, 2019
1 task
See silverstripe/silverstripe-assets#235 Makes a start at silverstripe/silverstripe-assets#219 as well
For the vast majority of sites, you really don't want to run your file migration as part of dev build. The step is involved enough to warrant it's own task. I don't think this is an API change, since the setting won't have affect for anyone who has already enabled it - they would've already done the one-off migration. See https://github.com/silverstripeltd/open-sourcerers/issues/91 and silverstripe/silverstripe-assets#235
chillu
force-pushed
the
pulls/4/thumbnail-helper
branch
from
188f432 to
39e72d0
Compare
Technically an API breakage, but I don't see migration helpers as part of our API surface. They're one off utilitie
chillu
force-pushed
the
pulls/4/thumbnail-helper
branch
from
39e72d0 to
bb75995
Compare
maxime-rainville
requested changes
Apr 30, 2019
There was a problem hiding this comment.
While the new logger approach is a step in the right direction, it also makes the output a lot less pleasant to look at on the web and CLI. Any chance we could sneak in some minor formatting tweaks?
I tested that each sub tasks gets call correctly. I didn't test that each task actually works together. If you feel that this is important to test before the RC release, I can have a go at it.
Also, sub-sites extends the FileMigrationTask, which I haven't tested yet. Let me know if this needs to be done before the RC release.
| If additional security or visibility rules should be applied to File dataobjects, then | ||
| make sure to correctly extend `canView` via extensions. | ||
|
|
||
| ## Automatic migration | ||
| Imports all files referenced by File dataobjects into the new Asset Persistence Layer introduced in 4.0. |
There was a problem hiding this comment.
The style of this paragraph doesn't seem to match the one use by the two previous ones and seem to partially repeat some of the points.
Maybe reformat the entire thing in the form of:
This task will:
* Import all files referenced by File dataobjects into the new Asset Persistence Layer introduced in 4.0.
* Migrate existing File objects to file versioning.
* Moves existing thumbnails, and generates new thumbnail sizes for the CMS UI.
Any pre-existing File objects will be automatically published to the live stage, to ensure
that previously visible assets remain visible to the public site.
If the task fails or times out, run it again and it will start where it left off.
There was a problem hiding this comment.
OK, I've rearranged that a bit
src/Dev/Tasks/MigrateFileTask.php
Outdated
| @@ -22,36 +23,127 @@ class MigrateFileTask extends BuildTask | |||
|
|
|||
| protected $title = 'Migrate File dataobjects from 3.x'; | |||
There was a problem hiding this comment.
That's no longer SS3 specific
According to Max's feedback
Max pointed out that it's only shown on the website, so would need HTML for the list.
chillu
commented
Apr 30, 2019
| If additional security or visibility rules should be applied to File dataobjects, then | ||
| make sure to correctly extend `canView` via extensions. | ||
|
|
||
| ## Automatic migration | ||
| Imports all files referenced by File dataobjects into the new Asset Persistence Layer introduced in 4.0. |
There was a problem hiding this comment.
OK, I've rearranged that a bit
src/Dev/Tasks/MigrateFileTask.php
Outdated
| @@ -22,36 +23,127 @@ class MigrateFileTask extends BuildTask | |||
|
|
|||
| protected $title = 'Migrate File dataobjects from 3.x'; | |||
maxime-rainville
approved these changes
May 1, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Merge after #8910Merge before silverstripe/silverstripe-assets#242
See silverstripe/silverstripe-assets#235 Makes a
start at silverstripe/silverstripe-assets#219 as well