magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.52k stars 9.31k forks source link

Improve Error Message "Invalid file name" in setValueAfterValidation() Method #39222

Open obsergiu opened 3 weeks ago

obsergiu commented 3 weeks ago

Currently, the setValueAfterValidation method in Magento\Config\Model\Config\Backend\File.php throws a generic error message, Invalid file name, when a file name doesn't pass validation(like: default/logo.png). I think it would be better for improving this by including the actual invalid file name in the message to make it more informative.

This validation and function were introduced in Magento 2.4.6-p5, but I don't see it in the current beta version. As such it should be treated more like a reference for others who might encounter similar issues.

The code is located at: https://github.com/magento/magento2/blob/1819fe7377ab3010fc41e80f5634e9053707de65/app/code/Magento/Config/Model/Config/Backend/File.php#L283

The existing code:

private function setValueAfterValidation(string $value): void
{
    if (preg_match('/[^a-z0-9_\/\\-\\.]+/i', $value)
        // phpcs:ignore Magento2.Functions.DiscouragedFunction
        || !$this->_mediaDirectory->isFile($this->_getUploadDir() . DIRECTORY_SEPARATOR . basename($value))
    ) {
        throw new LocalizedException(__('Invalid file name'));
    }

    $this->setValue($value);
}

Suggested change:

private function setValueAfterValidation(string $value): void
{
    if (preg_match('/[^a-z0-9_\/\\-\\.]+/i', $value)
        // phpcs:ignore Magento2.Functions.DiscouragedFunction
        || !$this->_mediaDirectory->isFile($this->_getUploadDir() . DIRECTORY_SEPARATOR . basename($value))
    ) {
        throw new LocalizedException(__('Invalid file name: "%1".', $value));
    }

    $this->setValue($value);
}

Including the actual file name in the error message will make it easier to understand what went wrong and help debug the issue faster.

Steps to Reproduce:

  1. Try to save a configuration with an invalid file name.
  2. You’ll see a generic error message: Invalid file name.

Expected Result: The error message should show the exact file name, like: Invalid file name: "default/logo.png".

Actual Result: Currently, it just says: Invalid file name.

m2-assistant[bot] commented 3 weeks ago

Hi @obsergiu. Thank you for your report. To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce.

m2-assistant[bot] commented 3 weeks ago

Hi @engcom-Bravo. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

engcom-Bravo commented 3 weeks ago

Hi @obsergiu,

Thanks for your reporting and collaboration.

We are Considering this as a improvement to proceed further marking this as a feature request.

Thanks.