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

[FEATURE] Extend the task "install:fixfolderstructure" to create configured extension directories #290

Merged
merged 17 commits into from Dec 22, 2016

Conversation

websi
Copy link
Contributor

@websi websi commented Sep 14, 2016

No description provided.

@fsaris
Copy link

fsaris commented Sep 14, 2016

Isn't this covered by extension:setupactive?

@helhum
Copy link
Member

helhum commented Sep 14, 2016

Isn't this covered by extension:setupactive?

Yes, it is.

@websi can you describe the use case of having a command that creates directories only?

@websi
Copy link
Contributor Author

websi commented Sep 15, 2016

This is not a part of extension:setupactiv. This is only a part of extension:activate.

Our use case is an automatic deployment of TYPO3.
So we don't want to setup every extension at every deployment run.

Also we don't want to write the LocalConfiguration.php at an deployment run.
We configure the extensions in a git repository and release that.

@bauschan
Copy link

Any news here?

@helhum
Copy link
Member

helhum commented Nov 9, 2016

Will be fixed with https://review.typo3.org/#/c/50561

@helhum
Copy link
Member

helhum commented Nov 9, 2016

Also we don't want to write the LocalConfiguration.php at an deployment run.

That is indeed unfortunate, but does not harm, except that the file (system) needs to be writable.
That is a conceptual issue which should also be handled on TYPO3 side.

But to put everything aside, I think it would be great to let the install:fixfolderstructure command handle the folder creation also for extensions.

@websi websi changed the title [FEATURE] Add task to create configured directories for installed extensions [FEATURE] Extend the task "install:fixfolderstructure" to create configured extension directories Dec 16, 2016
Conflicts:
Classes/Command/InstallCommandController.php
Configuration/Console/Commands.php
Copy link
Member

@helhum helhum left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this.

Two things here.

First, please remove all re-formatting from the change. Mixing code style changes with other changes, makes the actual changes much harder to read and the PR harder to review. Besides that, it adds unnecessary additional potential discussions about a topic that needs to discussed separately.

Second and most importantly, we need to follow a different approach for creating extension folders. Since TYPO3 has no API for that, we need to create it. This fix foldersturcture command needs to stay low level.

Thanks again for your efforts! I'd be happy to support you in changing the code in the direction I mentioned. If you have questions, feel free to ask.

@@ -41,24 +42,24 @@ class InstallCommandController extends CommandController
protected $packageStatesGenerator;

/**
* TYPO3 Setup
* TYPO3 Setup.
Copy link
Member

@helhum helhum Dec 20, 2016

Choose a reason for hiding this comment

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

Full stops in the headline have been removed in #256 and should stay removed.

The docheader comments are used for documentation and to show the help texts. Having them consistent matters

* @param bool $nonInteractive If specified, optional arguments are not requested, but default values are assumed.
* @param bool $force Force installation of TYPO3, even if <code>LocalConfiguration.php</code> file already exists.
* @param string $databaseUserName User name for database server
* @param bool $nonInteractive If specified, optional arguments are not requested, but default values are assumed.
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this might affect the documentation and help texts as well. Besides that, all other classes are formatted with a single space to separate the argument types, names and description.

I have no plans to change that

@@ -115,22 +117,23 @@ public function generatePackageStatesCommand($removeInactive = false, $activateD
if (empty($activePackages[$package->getPackageKey()])) {
$this->packageManager->unregisterPackage($package);
GeneralUtility::flushDirectory($package->getPackagePath());
$this->outputLine('Removed Package: ' . $package->getPackageKey());
$this->outputLine('Removed Package: '.$package->getPackageKey());
Copy link
Member

Choose a reason for hiding this comment

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

This violates the code style. There should even be a fixer defined in .php_cs that adds the spaces

'typo3_console:configuration:*' => \Helhum\Typo3Console\Core\Booting\RunLevel::LEVEL_MINIMAL ,
'typo3_console:install:databasedata' => \Helhum\Typo3Console\Core\Booting\RunLevel::LEVEL_MINIMAL,
'typo3_console:install:defaultconfiguration' => \Helhum\Typo3Console\Core\Booting\RunLevel::LEVEL_MINIMAL,
'typo3_console:install:fixfolderstructure' => \Helhum\Typo3Console\Core\Booting\RunLevel::LEVEL_FULL,
Copy link
Member

@helhum helhum Dec 20, 2016

Choose a reason for hiding this comment

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

That is a breaking change and also a rather severe one. It limits this command to be only executable with a full db connection, which isn't the case on a build system, which creates the package to be deployed. The command must stay very low level.

@@ -149,10 +152,20 @@ public function fixFolderStructureCommand()
$this->outputLine($fixedStatusObject->getTitle());
}
}

/** @var $installUtility \TYPO3\CMS\Extensionmanager\Utility\InstallUtility */
$installUtility = $this->objectManager->get(\TYPO3\CMS\Extensionmanager\Utility\InstallUtility::class);
Copy link
Member

Choose a reason for hiding this comment

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

as pointed out below, we can't simply do it like this unfortunately.

My idea is to gather the information (folders to be created) from all active extensions and put this info into the array of to be created folders and let the folder structure facade create the folders

*
* This command is great e.g. for creating the typo3temp folder structure during deployment
*
* @throws \TYPO3\CMS\Install\FolderStructure\Exception
* @throws \TYPO3\CMS\Install\Status\Exception
* @throws \TYPO3\CMS\Extensionmanager\Exception\ExtensionManagerException
Copy link
Member

Choose a reason for hiding this comment

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

is this still the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, can be removed.

@helhum helhum merged commit 6984793 into TYPO3-Console:master Dec 22, 2016
@helhum
Copy link
Member

helhum commented Dec 22, 2016

Awesome! Thanks a lot for your work @websi !

@helhum
Copy link
Member

helhum commented Dec 22, 2016

… and your patience to apply all the requested changes.

@websi websi deleted the extension_directories branch April 6, 2017 12:39
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

4 participants