Upload fails in image uploader due to unrequired dependency

[posttechguy]

Description (*)

In lib/internal/Magento/Framework/File/Mime.php:97 fails because mime_content_type function does not exist, which php extension fileinfo adds

Thus when the temp uploaded file, i.e., '/tmp/sdfds5we23e', which has no extension, is trying to get the extension of the file via pathinfo function in getFileExtension(), it fails to verify against the validTypes array because the extension does not exist.

@magento give me 2.3-develop instance

Fixed Issues (if relevant)

1. [Magento 2.3.2] Upload fails in image uploader #24332: [Magento 2.3.2] Upload fails in image uploader

Manual testing scenarios (*)

1. Upload image via wysiwyg editor on a page without fileinfo php extension

Questions or comments

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @posttechguy. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[magento-cicd2]

All committers have signed the CLA.

[posttechguy]

@buskamuza I have signed the Contributor License Agreement though it says still pending, my icon is appearing in the PR, (i saw this in some of the documentation), not sure what the issue is

[ihor-sviziev]

Hi @posttechguy,
Commit author’s email is not associated with your github account, that’s why it showes that cla is not signed.
Could you add commiter email to your github account and check again?

If you’ll do everything correct - near commit message you’ll have your github account avatar and it will become clickable

[buskamuza]

I would suggest to fix this in 2.4 because adding an ext dependency can break deployments w/o this extension (they'll have to update the environment to perform upgrade).
This can be documented for now as a recommended extension to be installed.
The issue should not be critical as it's possible to fix it by installing corresponding extension.

[VladimirZaets]

Hi @posttechguy. Thanks for collaboration. Please take a look comments in the review.

[ihor-sviziev]

@buskamuza

I would suggest to fix this in 2.4 because adding an ext dependency can break deployments w/o this extension (they'll have to update the environment to perform upgrade).
This can be documented for now as a recommended extension to be installed.
The issue should not be critical as it's possible to fix it by installing corresponding extension.

Actually this dependency we have in 2.3, but it’s not defined anywhere. Otherwise we wouldn’t have this issue.
As for me - it’s ok to add it to 2.3

[buskamuza]

Actually this dependency we have in 2.3, but it’s not defined anywhere.

It's a soft dependency now:

        if (function_exists('mime_content_type')) {
            $result = $this->getNativeMimeType($file);
        }

Yes, files w/o extensions won't be processed correctly, but files w/ extension will be fine.
With the added ext dependency people w/o it won't be able to install Magento at all, though they can now.

[ihor-sviziev]

Actually this dependency we have in 2.3, but it’s not defined anywhere.

It's a soft dependency now:

        if (function_exists('mime_content_type')) {
            $result = $this->getNativeMimeType($file);
        }

Yes, files w/o extensions won't be processed correctly, but files w/ extension will be fine.
With the added ext dependency people w/o it won't be able to install Magento at all, though they can now.

@buskamuza, ah yes, you’re right, so let’s add it to release line 2.4 due to backward compatibility issues

[hostep]

@buskamuza: no it's not a soft dependency. Most of the time in the code it is checked if the function exists, but at one occurrence it's not being checked:

• https://github.com/magento/magento2/blob/2.3.2/app/code/Magento/Cms/Model/Wysiwyg/Images/Storage.php#L333

So either we need to add the dependency to the cms module (or framework?) in 2.3.x, or either that call should also be wrapped in a function_exists and an alternative should be used if it doesn't exist.

[buskamuza]

@hostep , good catch.
This then should be refactored to use the \Magento\Framework\File\Mime::getMimeType(), so we don't depend on PHP native functions directly.
Or Magento_Cms module should have the dependency on ext-fileinfo because it's that module that uses it. This approach is not desirable. That's incorrect usage of PHP functions.

[hostep]

Tnx for feedback @buskamuza!

@posttechguy: are you willing to update the PR with the suggested change?

[ihor-sviziev]

Please review comment from @buskamuza and adjust PR accordingly

[ihor-sviziev]

Hi @posttechguy,
Could you review following comment #24455 (comment) and adjust your PR accordingly?

[posttechguy]

Please review

[ihor-sviziev]

Hi @posttechguy,
I can't see changes related to Magento_Cms module that @buskamuza requested:

This then should be refactored to use the \Magento\Framework\File\Mime::getMimeType(), so we don't depend on PHP native functions directly.
Or Magento_Cms module should have the dependency on ext-fileinfo because it's that module that uses it. This approach is not desirable. That's incorrect usage of PHP functions.

Maybe you missed to push them?

[ihor-sviziev]

Hi @posttechguy,
Sorry, still can't see requested changes.

[ihor-sviziev]

@posttechguy I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

[m2-assistant]

Hi @posttechguy, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.