Fixed deprecation warning if downloadable sample is a url

[ma4nn]

Description (*)

This PR fixes the error Deprecated functionality: str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated in /app/htdocs/app/code/core/Mage/Downloadable/Helper/File.php on line 149.
It occurs when a downloadable product is edited in backend that uses urls for the samples (as opposed to files).

Manual testing scenarios (*)

1. Create a new downloadable product in backend
2. In tab "Downloadable Information" add a sample row with a URL

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

[kiatng]

The method getFilePath() is called from many places, so I think it's better to fix at the source:

magento-lts/app/code/core/Mage/Downloadable/Helper/File.php

Line 130 in c7d6d55

public function getFilePath($path, $file)

    public function getFilePath($path, $file)
    {
        if (!$file) {
            return $path . DS;
        }
// ...

[ma4nn]

@kiatng thanks for your feedback. I didn't change that method intentionally for several reasons:

1. Lots of more tests would be required which makes that PR not that tiny any more (and in most of the other cases the parameter cannot be null). It's a similar discussion as in Fix deprecated functionality str_contains() in Design/Package.php #3473. I am more on the conservative side of changes, especially if there are no automated testings.

2. I don't like those conditional arguments, imho it would be better to somehow later use Typehints and simply use string $file as parameter for that method (what kind of sense does a function getFilePath() make if no file is given?)

But I'm happy to change it if that's the way to go.

[sreichel]

I think @kiatng is right. Before deprecation null was casted to empty string, ending up with return $path . DS;.

[sreichel]

2. I don't like those conditional arguments, imho it would be better to somehow later use Typehints and simply use string $file as parameter for that method

I agree in somehow later having typehints, but at this time we maybe have rector or phpstan who let you know about issues you mention.

In case that checks looks pretty safe. Even with typehints it avoids running str-functions on empty string.

[fballiano]

@ma4nn can you please check my last comment? I can't modify this PR because you didn't allow edits by admins. other than that I'd approve it

[ma4nn]

@fballiano thanks I have rebased to main to resolve all conflicts.
I did activate modifications of the PR, but it seems like Github only allows this for personal forks. I will do future PRs from a personal fork then 👍

[addison74]

@ma4nn - Could you please check this PR https://github.com/sreichel/openmage-future/pull/8 which seems to be related to yours?

[ma4nn]

@addison74 I must admit the contribution process here in the OpenMage repo is really weired and not very encouraging 🙈 Why check another repo after I have updated this PR according to @fballiano? What do you mean with "check"? etc.

[addison74]

@ma4nn - Thank you for your question and I assure you that you have no reason to worry. I personally appreciate any contribution. I noticed that your PR has a reference to another repository attached today. I looked over the other proposal, which is made by an important OpenMage contributor, who unfortunately no longer has the rights to post here and I can say that he has analyzed the issues discussed here. If it is a better PR then it should be in OpenMage, if the author agrees for his work to be brought here. If not, then I approve this PR after @kiatng expresses his opinion and we move on.