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

Export and import widget templates #49

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

eluhr
Copy link
Contributor

@eluhr eluhr commented Apr 25, 2022

Export and import widget templates

This PR provides an export and import functionality for the existing widget template CRUD

Export

Exporting a widget can be done via the newly added export buttons either on the widget template overview page or the widget templates detail page

When exporting a widget template a .tar file will be created to summarize the following three files:

  1. meta.json - Optional for the import. If not present a fallback for the name and php_class column will be used
  2. schema.json - This contains the json schema for the widget
  3. template.twig - This contains the twig template for the widget

Import

To import a generated .tar file to the db you can do this either from the widget overview (/widget/default/index) page or the widget template overview page. You can either upload one or up to 20 .tar files (we can still discuss this value. But more makes no sense in my opinion)

For each functionality a new permission check was added:

  • import: widgets_crud_widget-template_import
  • export: widgets_crud_widget-template_export

Only if these permissions are granted to the user, the user will be able to see the links and access the corresponding pages.

widget-template-general-overview

widget-template-view

widget-template-overview

// Create the tar archive
$phar = new \PharData($this->getTarFilePath());
// Add generated files
$phar->addFile($this->getTemplateFilePath(), $this->templateFilename);
Copy link
Member

Choose a reason for hiding this comment

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

why not add strings to the tar directly instead of creating/removing tmp-files?

$phar->addFromString($this->templateFilename, $this->widgetTemplate->twig_template);

This would significantly reduce the complexity because we would have much less file handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have modified the code to meet your requirements. In this move I also removed the methods that create the temporary files.

*
* @var string
*/
public $exportDirectory = '@runtime/tmp/dmstr/widgets/templates';
Copy link
Member

Choose a reason for hiding this comment

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

directories (and/or filenames) used for such exports should always have some randomness to prevent race conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now a protected property and is set to a random directory in /tmp (via sys_get_temp_dir)

}

// Make sure that if an alias is set, it is resolved correctly
$this->importDirectory = \Yii::getAlias($this->importDirectory);
Copy link
Member

Choose a reason for hiding this comment

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

directories (and/or filenames) used for imports should always have some randomness to prevent race conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now a protected property and is set to a random directory in /tmp (via sys_get_temp_dir)

]);

if ($model->save() === false) {
\Yii::error($model->getErrors());
Copy link
Member

Choose a reason for hiding this comment

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

These errors should be passed to the user so that he gets information why the import did not work. (Exceptions?)
see: Line 136

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a custom exceptions. This bundles the error messages that occur when saving. The exception is then caught in the controller and passed on to the user.

try {
FileHelper::removeDirectory($this->importDirectory);
return true;
} catch (\ErrorException $e) {
Copy link
Member

Choose a reason for hiding this comment

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

yii\base\ErrorException is imported. Is it intended that another ErrorException is used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line has been removed because it is no longer needed

@handcode
Copy link
Member

thanks for your work, but we should change file and error handling to:

  • reduce complexity (create tar from strings not from tmp files)
  • prevent race conditions (randomness in dir and/or file names)
  • give user more information if something went wrong (Exceptions?)

see comments above.

@eluhr
Copy link
Contributor Author

eluhr commented Apr 27, 2022

@handcode : I have updated the code and left explanations to your comments respectively

@schmunk42
Copy link
Member

@eluhr @handcode
Anything left to do here?

]);

if ($model->save() === false) {
$this->addError('tarFiles', print_r($model->getErrors(), true));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe would c|should format error messages a little bit ;-)
Screenshot 2022-05-10 at 10 14 53

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.

3 participants