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

Customization for _getUploadRoot and _getAllowedExtensions in Mage_Adminhtml_Model_System_Config_Backend_File #3933

Conversation

eneiasramos
Copy link
Contributor

@eneiasramos eneiasramos commented Apr 8, 2024

…lowedExtensions

Description (*)

Here we have the implementation of 2 functionalities:

1 - Add validation for allowed extensions when uploading files.

today:

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

new:

<upload_dir allowed_extensions="png,jpg,jpeg,gif" config="system/filesystem/media" scope_info="1">sales/store/logo</upload_dir>

2 - Add validation to the config's attribute in upload_dir's tag (not used today). Also the possibility of customizing it. Example:

today:

<upload_dir config="system/filesystem/media" scope_info="1">docs/pdf</upload_dir>

new:

<upload_dir allowed_extensions="pdf" config="system/filesystem/var" scope_info="1">docs/pdf</upload_dir>

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes OpenMage/magento-lts#<issue_number>

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label Apr 8, 2024
eneiasramos added a commit to gamuzatech/tolucastore-openmage that referenced this pull request Apr 8, 2024
@fballiano
Copy link
Contributor

@eneiasramos could you please write a real title and description? thanks!

@eneiasramos
Copy link
Contributor Author

@eneiasramos could you please write a real title and description? thanks!

Sorry I forgot that my dear 😸

@eneiasramos eneiasramos changed the title bugfix/adminhtmlSystemConfigBackendFile: fix _getUploadRootand _getAl… bugfix/adminhtmlSystemConfigBackendFile: implements functionality for methods _getUploadRoot and _getAllowedExtensions Apr 8, 2024
@eneiasramos eneiasramos changed the title bugfix/adminhtmlSystemConfigBackendFile: implements functionality for methods _getUploadRoot and _getAllowedExtensions bugfix/adminhtmlSystemConfigBackendFile: implements functionalities for methods _getUploadRoot and _getAllowedExtensions Apr 8, 2024
@eneiasramos eneiasramos changed the title bugfix/adminhtmlSystemConfigBackendFile: implements functionalities for methods _getUploadRoot and _getAllowedExtensions Customization for _getUploadRoot and _getAllowedExtensions in Mage_Adminhtml_Model_System_Config_Backend_File Apr 8, 2024
@eneiasramos
Copy link
Contributor Author

@fballiano done my dear!

@fballiano
Copy link
Contributor

@Flyingmana can you help me here? I'm not sure I understand how to test this

@Flyingmana
Copy link
Contributor

will have a look on the weekend

@pquerner
Copy link
Contributor

pquerner commented May 2, 2024

Images can use the backend_model Mage_Adminhtml_Model_System_Config_Backend_Image, this will allow uploading of 'jpg', 'jpeg', 'gif', 'png' already. No need to have that in the base class.

As for the path I don't follow the changes here. Can you explain some more of what you are trying to do?

If you only want to allow a specific file extension I would provide a class that extends Mage_Adminhtml_Model_System_Config_Backend_File and override protected function _getAllowedExtensions.
(see Mage_Adminhtml_Model_System_Config_Backend_Image as an example)

@Flyingmana
Copy link
Contributor

Ok, for the how to Test part.

have a config which overwrites the system.xml for
image

There by adding the new allowed_extensions attribute it should be able to control which ones are allowed to upload.

Although, its probably more for the logo in app/code/core/Mage/Sales/etc/system.xml not the placeholder one in my screenshot.

What Iam not sure about is, what the exact use case or benefit should be.

Also as this part is a bit complex, I would would like to require a phpunit test for this parts

$dir = str_replace('root_dir', 'base_dir', $matches[1]);
$path = str_replace('/', DS, $matches[2]);
return Mage::getConfig()->getOptions()->getData($dir) . $path;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert the changes in this method(and do it as a separate PullRequest with proper description), they are not covered by the Description and there is no reason visible for me, why this change might be needed.

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 put it in the comment below

Copy link
Contributor

@Flyingmana Flyingmana left a comment

Choose a reason for hiding this comment

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

please do separate the getUploadRoot change, and provide further explaination why the getAllowedExtensions should be extended in this way and which file formats you actually plan to add (or remove?)

@fballiano fballiano marked this pull request as draft May 5, 2024 18:11
eneiasramos added a commit to gamuzatech/tolucastore-openmage that referenced this pull request May 24, 2024
eneiasramos added a commit to gamuzatech/tolucastore-openmage that referenced this pull request May 24, 2024
eneiasramos added a commit to gamuzatech/tolucastore-openmage that referenced this pull request May 24, 2024
eneiasramos added a commit to gamuzatech/tolucastore-openmage that referenced this pull request May 24, 2024
eneiasramos added a commit to gamuzatech/tolucastore-openmage that referenced this pull request May 24, 2024
@eneiasramos
Copy link
Contributor Author

@Flyingmana here:

#4078

#4079

@fballiano
Copy link
Contributor

fballiano commented Jul 6, 2024

replaced by #4079 and #4078

@fballiano fballiano closed this Jul 6, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants