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

Added support to specify file upload dir in system configuration #4079

Conversation

eneiasramos
Copy link
Contributor

@eneiasramos eneiasramos commented Jul 3, 2024

Added support to specify filesystem's directory in file upload ...

@addison74
Copy link
Contributor

@eneiasramos - I need more details about this PR. I have some time to test it.

@eneiasramos
Copy link
Contributor Author

eneiasramos commented Jul 7, 2024

@addison74 my dear

The routine here allows a file to be uploaded to a folder different from media.

The idea is to allow the upload of a file, but not to expose the file to the internet (like the media folder does).

A use of case: I use the var folder to store PFX certificates used to communicate with a third party system:

<upload_dir allowed_extensions="pfx" config="system/filesystem/var" scope_info="1">brazil/cert</upload_dir>

https://github.com/gamuzatech/tolucastore-openmage/blob/f559a0e819a3124599b392fc3c4defe5c3803b68/app/code/local/Gamuza/Brazil/etc/system.xml#L400

@kiatng
Copy link
Contributor

kiatng commented Jul 7, 2024

Review Notes:
This PR is to allow setting the root dir for file upload in the backend, in which the upload field is configured in the files system.xml. An example:

                        <filename translate="label" module="brazil">
                            <label>Filename</label>
                            <frontend_type>file</frontend_type>
                            <backend_model>adminhtml/system_config_backend_file</backend_model>
                            <upload_dir allowed_extensions="pfx" config="system/filesystem/var" scope_info="1">brazil/cert</upload_dir>
                            <sort_order>20</sort_order>
                            <show_in_default>1</show_in_default>
                            <show_in_website>1</show_in_website>
                            <show_in_store>1</show_in_store>
                            <depends><type_id>1</type_id></depends>
                        </filename>

In the above example, the config attribute of the upload_dir node is referenced to this:

<system>
<filesystem>
<base>{{root_dir}}</base>
<app>{{root_dir}}/app</app>
<code>{{app_dir}}/code</code>
<design>{{app_dir}}/design</design>
<locale>{{app_dir}}/locale</locale>
<etc>{{app_dir}}/etc</etc>
<media>{{root_dir}}/media</media>
<upload>{{root_dir}}/media/upload</upload>
<skin>{{root_dir}}/skin</skin>
<var>{{var_dir}}</var>
<cache>{{var_dir}}/cache</cache>
<session>{{var_dir}}/session</session>
<tmp>{{var_dir}}/tmp</tmp>
<pear>{{var_dir}}/pear</pear>
<export>{{var_dir}}/export</export>
</filesystem>
</system>

Based on the above, the config path is {{var_dir}}, which points to the var dir at OM root.

Before this PR, regardless of the attribute config value, the upload file always go to the media dir. With this PR, it will respect the config, in the example, it will allow upload to the var dir.

For the actual setting of the path, see

$this->_data['app_dir'] = $appRoot;
$this->_data['base_dir'] = $root;
$this->_data['code_dir'] = $appRoot . DS . 'code';
$this->_data['design_dir'] = $appRoot . DS . 'design';
$this->_data['etc_dir'] = $appRoot . DS . 'etc';
$this->_data['lib_dir'] = $root . DS . 'lib';
$this->_data['locale_dir'] = $appRoot . DS . 'locale';
$this->_data['media_dir'] = $root . DS . 'media';
$this->_data['skin_dir'] = $root . DS . 'skin';
$this->_data['var_dir'] = $this->getVarDir();
$this->_data['tmp_dir'] = $this->_data['var_dir'] . DS . 'tmp';
$this->_data['cache_dir'] = $this->_data['var_dir'] . DS . 'cache';
$this->_data['log_dir'] = $this->_data['var_dir'] . DS . 'log';
$this->_data['session_dir'] = $this->_data['var_dir'] . DS . 'session';
$this->_data['upload_dir'] = $this->_data['media_dir'] . DS . 'upload';
$this->_data['export_dir'] = $this->_data['var_dir'] . DS . 'export';

kiatng
kiatng previously approved these changes Jul 7, 2024
Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

Approved based on testing protected function _getUploadRoot($token).

@eneiasramos
Copy link
Contributor Author

@kiatng thank you my dear!

@kiatng kiatng changed the title Added support to specify filesystem's directory in file upload Added support to specify file upload dir in system configuration Jul 8, 2024
@fballiano
Copy link
Contributor

I've modified the PR slightly because if you'd use

<upload_dir config="system/filesystem/UNEXSTING" scope_info="1">sales/store/logo</upload_dir>

it would generate a null parameter warning.

tested quite some cases and it seems to work correctly now

@eneiasramos
Copy link
Contributor Author

@fballiano perfect!

@fballiano
Copy link
Contributor

I'll merge it in a couple of days just to see if we get some more feedback, thank you @eneiasramos

@eneiasramos
Copy link
Contributor Author

@fballiano thank you!

@kiatng kiatng merged commit 5d0569c into OpenMage:main Aug 7, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants