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

Refactored Downloadable Files Import #393

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

sprankhub
Copy link
Collaborator

  • imported downloaded files are now created via the standard uploader Mage_ImportExport_Model_Import_Uploader
  • this ensures that these files are created in sub-folders like /media/downloadable/files/links/f/i/file.pdf
  • this also ensures that these folders will be created if they do not exist yet

Copy link
Collaborator

@nhp nhp left a comment

Choose a reason for hiding this comment

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

Adding a bit of test data, especially for new functionality would be great. Makes it a whole lot easier to reproduce and check.
Nevertheless great effort and work. Thanks Simon


$tmpDir = Mage::getConfig()->getOptions()->getMediaDir() . '/import';
$destDir = Mage::getModel('downloadable/link')->getBasePath();
if ( ! is_writable($destDir)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really what you want to do? Check if not writeable and then create it? I really have strong opinions against the usage of @ for error suppression like in this case. Either a correct check or a correct handling of the result of the check you already do would be the cleaner alternative.

$this->_downloadableUploader = Mage::getModel('fastsimpleimport/import_uploader_downloadable', null);
$this->_downloadableUploader->init();

$tmpDir = Mage::getConfig()->getOptions()->getMediaDir() . '/import';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Usage of DS instead of / for compatibility reasons.

if ( ! file_exists($tmpDir)) {
@mkdir($tmpDir, 0777, true);
}
if ( ! $this->_downloadableUploader->setTmpDir($tmpDir)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you change the check for existence and writability you could supply better Exception messages.

@sprankhub
Copy link
Collaborator Author

Thanks for your review! Please see my last commits. I agree that the usage of @ should really be avoided. To be honest I basically simply copied the method _getUploader, which does exactly the same. Hence, it is a more general problem. I created a new issue #394 in order to work on this.

Test data can be taken form here. I will try to write test scripts and/or update the wiki and/or the website as soon as my work on downloadable products is finished.

@sprankhub
Copy link
Collaborator Author

@nhp Would you approve the changes after my updates? Thanks!

@sprankhub
Copy link
Collaborator Author

@nhp would you have a short look again? Thanks!

@sprankhub sprankhub mentioned this pull request Oct 12, 2018
pixelhed added a commit to pixelhed/AvS_FastSimpleImport that referenced this pull request Oct 17, 2018
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.

2 participants