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.

Sign up for GitHub

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

Jump to bottom

legacy:assets_install should not delete non empty folders unless asked to #112

Merged
merged 5 commits into from
Oct 4, 2017
Merged

legacy:assets_install should not delete non empty folders unless asked to #112

merged 5 commits into from
Oct 4, 2017

Conversation

andrerom
Copy link
Contributor

@andrerom andrerom commented Oct 3, 2017

Fixes #110

@andrerom andrerom requested review from glye and emodric October 3, 2017 18:08
emodric
emodric approved these changes Oct 4, 2017
View reviewed changes
Copy link
Collaborator

@emodric emodric left a comment

Choose a reason for hiding this comment

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

Apart from some minor nitpicks, 👍

bundle/Command/LegacyWrapperInstallCommand.php Outdated
$output->writeln(<<<EOT
Skipping: The folder "$targetDir" already exists and seems to contain content!

Make sure to backup this content and move it into corresponding legacy folder which is will be setup to symlink / copy
Copy link
Collaborator

Choose a reason for hiding this comment

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

which will be setup

bundle/Command/LegacySrcSymlinkCommand.php Outdated
@@ -28,7 +28,7 @@ protected function configure()
)
)
->addOption('create', 'c', InputOption::VALUE_NONE, 'Create "src" directory structure if it does not exist')
->addOption('force', 'f', InputOption::VALUE_NONE, 'Symlink folders even if target already exist')
->addOption('force', null, InputOption::VALUE_NONE, 'Force symlinking folders even if target already exist')
Copy link
Collaborator

Choose a reason for hiding this comment

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

if target already exists

glye
glye approved these changes Oct 4, 2017
View reviewed changes
Copy link
Member

@glye glye left a comment

Choose a reason for hiding this comment

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

Assuming you have checked that it doesn't count . and .. as folder content :)

bundle/Command/LegacyWrapperInstallCommand.php Outdated

foreach (array('design', 'extension', 'share', 'var') as $folder) {
$targetDir = "$targetArg/$folder";
$originDir = "$legacyRootDir/$folder";

// Check if directory exists (not link) and is not empty to avoid removing things that should be backend up
Copy link
Member

Choose a reason for hiding this comment

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

backed up

bundle/Command/LegacyWrapperInstallCommand.php Outdated
Make sure to backup this content and move it into corresponding legacy folder which is will be setup to symlink / copy
to this folder before you remove it, then re-run this command.

If this folder and the other "$targetArg" directories can be safely overwritten, run this command with <info>--force</info> option.
Copy link
Member

Choose a reason for hiding this comment

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

command with the

@andrerom
Copy link
Contributor Author

andrerom commented Oct 4, 2017 •
edited
Loading

Assuming you have checked that it doesn't count . and .. as folder content :)

from comments and doc it does not include those, it also does not include hidden folders (.*), which is fine in this case.

@andrerom andrerom merged commit 133a1c9 into master Oct 4, 2017
@andrerom andrerom deleted the safe_var_symlinks branch October 4, 2017 09:04
@andrerom
Copy link
Contributor Author

andrerom commented Oct 4, 2017

Thanks for the reviews, sorry if the only things to comment on is grammar as always :P

@blankse
Copy link
Contributor

blankse commented Dec 11, 2017

I have a problem with this:
The flysystem Local adapter creates a web/var/storage folder.
The composer ScriptHandler creates no symlink for web/var anymore because the storage folder counts as content.

@andrerom
Copy link
Contributor Author

@blankse So you are referring to setup that has already been used running platform, and then you add legacy bridge? or during install of both?

@blankse
Copy link
Contributor

blankse commented Dec 11, 2017

During install of both. If the symlink already exists everything works fine.
But if you build it new from stratch (e. g. platform.sh) the symlink doesn't exist and the Local Adapter (Dependency from IOBinarydataHandler/IOMetadatahandler) create a web/var/storage folder. This folder blocks the creating of the symlink to ezpublish_legacy/var folder.

@blankse blankse mentioned this pull request Dec 11, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glye glye glye approved these changes

@emodric emodric emodric approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andrerom @blankse @glye @emodric