-
Notifications
You must be signed in to change notification settings - Fork 31
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
[Feature] Add command to setup legacy settings/designs symlinks #107
Conversation
…esigns symlinks Without this feature, each and every user would have to find out the hard way that Composer sometimes wipes out the ezpublish_legacy folder, and also that versioning this folder is not really recommended. The feature in this PR is primary aiming at upgrade/migration use cases, where people are coming from 5.4 or 2014.x and similar and need to move over settings and designs. Review wanted from partners that have their own way of doing this, as you will know better what you need. *Why no extension folder handling?* Extensions are handled by composer and also by legacy bundles, so if you need an extension in src folder you can make app bundle an legacy bundle. Hence it felt wrong to give another option for this which could be confusing. *Why no storage and storage/packages folder handling?* As site storage folder is not typically versioned, I guess this should be handled by something else. Input on how people tend to deal with this more then welcome.
96faf7a
to
ef7a3c2
Compare
I will go through the code in couple of days since I'm on a short break right now, but this looks okay to me. At Netgen, we do something similar, with the difference that |
Cool, looking forward to it 😃 Btw reason why I opted for not putting legacy files in bundle was among other things because symfony 4 has single bundle layout with no bundle dir afaik, and legacy bundles feature already allow you to place extensions and legacy stuff inside it even if it takes a bit more config + extension structure. |
+1 |
@emodric had a chance to have a look yet? :) |
@andrerom Nope, but will do it ASAP and let you know! |
*/ | ||
$filesystem = $this->getContainer()->get('filesystem'); | ||
|
||
if (!$filesystem->exists($srcArg)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use creating the source folder structure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migration/Upgrade use case:
- install legacy bridge (will execute this step)
- manually move files over from old to new structure
So no manual steps needed for creating the folders, and it can be explicit referred to in doc where you move them as a default convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How hard is it to type mkdir -p src/legacy_files/settings
? :) Doesn't seem that much useful to me if it's documented right, but okay, if you think it can help, sure, but I still don't like that script is running with -c
automatically every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm with @emodric here on having -c
as default. Leaving off -c
will throw the exception and help the users who have not read the docs be aware they can and should migrate their existing config to the new location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR updated, also added much more output to try to explain the concepts better for user.
better? (changes done in new commit)
INSTALL.md
Outdated
"eZ\\Bundle\\EzPublishLegacyBundle\\Composer\\ScriptHandler::installAssets", | ||
"eZ\\Bundle\\EzPublishLegacyBundle\\Composer\\ScriptHandler::installLegacyBundlesExtensions", | ||
"eZ\\Bundle\\EzPublishLegacyBundle\\Composer\\ScriptHandler::generateAutoloads" | ||
"eZ\\Bundle\\EzPublishLegacyBundle\\Composer\\ScriptHandler::generateAutoloads", | ||
"eZ\\Bundle\\EzPublishLegacyBundle\\Composer\\ScriptHandler::symlinkLegacySrcFiles" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just symlinkLegacyFiles
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we rename folder name, yes
->setName('ezpublish:legacy:symlink_src') | ||
->setDefinition( | ||
array( | ||
new InputArgument('src', InputArgument::OPTIONAL, 'The src directory for legacy files', 'src/legacy_files'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about src/legacy
as the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to not imply it should contain legacy itself, but if this is preferred or something else I'm open for it.
protected function configure() | ||
{ | ||
$this | ||
->setName('ezpublish:legacy:symlink_src') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ezpublish:legacy:symlink
?
(I'm not a fan of acronyms :P)
new InputArgument('src', InputArgument::OPTIONAL, 'The src directory for legacy files', 'src/legacy_files'), | ||
) | ||
) | ||
->addOption('create', 'c', InputOption::VALUE_NONE, 'Create src folder structure if it does not exists') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does not exist
$legacyRootDir = rtrim($this->getContainer()->getParameter('ezpublish_legacy.root_dir'), '/'); | ||
|
||
if ($force || !$filesystem->exists("$legacyRootDir/settings/override")) { | ||
$filesystem->symlink( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably check if "$srcArg/settings/override"
exists before symlinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true
$symlinks[] = "$legacyRootDir/settings/override"; | ||
} | ||
|
||
$directories = ['design', 'settings/override']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
settings/siteaccess
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, wrong string here, override is handled above :)
@andrerom Left a couple of remarks, but in general, this looks okay. What I would like to definitely see is symlinking based on Symfony environment used when running the script ( |
|
Symlinking design folder is simpler than creating and activating the extension together with |
From slack
Not experienced it? I guess if you don't use composer in prod (which is a good thing, I guess you then have deployment pipeline for instance involving composer when rolling out new code, anyway) it won't be an issue for you.
I assumed some people use it given it was never deprecated in favour of extensions, as for extensions they are as said already handled by other features of this bundle. Maybe I should get the folder creation step to place a readme in legacy_folder directory to explain what it is for and not. |
updated. Regarding folder creation, maybe create on install? the point is to have a meaningful output to users and also to avoid additional steps. Also for the sake of trying to keep the documentation on this straight forward without to much if's and buts :) |
@andrerom If you think it can help, you can leave it as is. Since you don't have to use the command in your stack if empty folder structure bothers you, this should be okay :) |
Ok, then who else should we get to review this then? |
@wizhippo maybe ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides my thought on the default creation of of the src directory structure above I'm fine with this as is.
I too am curious how others are handling storage and should it be addressed, perhaps in another PR. I'm glad I have had backups a couple of times.
yes, at the very least would need to alert the user when we detect that folder being removed (overwritten by symlink) is not empty, like now reported in #110. |
95e2b72
to
ffa53c9
Compare
any design/settings project files, and symlinks these into <info>ezpublish_legacy/</info> which is installed by composer. | ||
|
||
The benefit of this is: | ||
1. Being able to version your design/config files in git withouth versing legacy itself |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
versing -> versioning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(more nitpicks)
Aborting: The src directory "$srcArg" does not exist. | ||
|
||
You can create the directory by running <info>ezpublish:legacy:symlink -c</info>, OR by creating the folders you need | ||
manually among the once supported by this command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once -> ones
if ($symlinkFolderStr) { | ||
$output->writeln("The following folders where symlinked: '$symlinkFolderStr'."); | ||
} else { | ||
$output->writeln('No folders where symlinked, use force option if they need to be re-created.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be overkill, but: --force
, to clarify?
Without this feature, each and every user would have to find out the hard way that
Composer sometimes wipes out the
ezpublish_legacy
folder, and also that versioningthis folder is not really recommended so you somehow need to deal with settings and designs.
So this command setups and symlinks the following into legacy on compser install and update:
The feature in this PR is primary aiming at upgrade/migration use cases, where
people are coming from 5.4 or 2014.x and similar and need to move over settings and designs.
Review wanted from partners that have their own way of doing this, as you will know
better what you need. /cc @emodric @joekepley @peterkeung ..
Why no extension folder handling?
Extensions are handled by composer, and also by legacy bundles command by this bundle,
so if you need an extension in src folder you can make app bundle a legacy bundle.
Hence it felt easily confusing to give another option for this.
Why no storage and storage/packages folder handling?
As site storage folder is not typically versioned in git, I guess this should be handled by something else then
src/
. Input on how people tend to deal with this is more then welcome!NOTE: But merged version of this PR contains hints to user during setup on manually handling storage folder, and with similar pointers in documentation this is somewhat covered for now, even if it is not automated (yet).
Why place them directly in
src/legacy_files
?Opted for this mainly because placing it in bundle is more of a legacy bundles feature, and also makes sure we are forward compatible with Symfony 4/Flex's single bundle src layout.