-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fix config backend model PDF file name extension #24153
Fix config backend model PDF file name extension #24153
Conversation
Hi @Vinai. Thank you for your contribution
In case you'd like to rerun tests use the following comments:
For more details, please, review the Magento Contributor Guide documentation. |
Hi @sidolov, thank you for the review.
|
✔️ QA passed |
@magento run all tests |
Thanks for catching the namespace bug. Regarding the |
@sidolov Is there anything I can do to help move this PR forward? |
Hi @Vinai, thank you for your contribution! |
Description (*)
The PDF system config file backend model does not include the file name ending
pdf
in the list of allowed extensions.Instead, it allows the extensions
['tif', 'tiff', 'png', 'jpg', 'jpe', 'jpeg']
, which are not relevant for PDF files.This PR adds
pdf
to the list of allowed extensions without removing the existing wrong extensions. This way it's a backward compatible change, and it makes the backend model usable for PDFs.It also adds a new backend model
\Magento\Config\Model\Config\Backend\Image\Pdf
as a replacement that only returns the allowed file name extensionpdf
.I added a
@deprecated
annotation to the existing backend model with an explanation and a@see
annotation to point to the new PDF backend model.Manual testing scenarios (*)
Referencing the backend model in a
system.xml
file, e.gThen upload a file with a
.pdf
extension. It will be rejected by the server side validation in\Magento\Config\Model\Config\Backend\File::beforeSave
Questions or comments
I'm not sure what the best way is to get this PR merged as quickly as possible.
If the deprecation of the
@api
class and addition of a new@api
class prevent it from being merged, I would suggest to remove those annotations and just merge the file changes, and then add the annotations in a new PR that can be merged in the next major release.Open to other suggestions if there is a better approach.
Contribution checklist (*)