10058: Tablerate->getCsvFile() fails with non-default PHP upload_tmp_dir

[RomaKis]

Description

Now path for upload_tmp_dir can be any.
2.3 pr: #12376

Fixed Issues (if relevant)

1. Tablerate->getCsvFile() fails with non-default PHP upload_tmp_dir #10058: Tablerate->getCsvFile() fails with non-default PHP upload_tmp_dir

Manual testing scenarios

1. Make a new directory for uploads, eg mkdir -p /var/lib/php/uploads && chmod 1777 /var/lib/php/uploads
2. Set PHP upload_tmp_dir to this directory. eg, add upload_tmp_dir = /var/lib/php/uploads to /etc/php.ini
3. Login to Magento Admin
4. Go to Stores > Configuration > Sales > Shipping Methods
5. Select Table Rates, and enable
6. Click "Import"
7. Choose a file, click save.

Expected result
Table rates are updated

Actual result
Receive an error, eg The file “/tmp/var/lib/php/uploads/phphEpJe5” doesn’t exist

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 on Travis CI are green)

[dmanners]

Unfortunately we cannot add new public methods to this class as it has been marked with @api would it work if we called getDirectoryRead here instead of making the new call?

[RomaKis]

@dmanners, no it wouldn't. It checks it suggested path is one of the /Magento/Framework/App/Filesystem/DirectoryList::getDefaultConfig().
And what if I'll create the pr with a same code to 2.3?

[dmanners]

Hmm ok, If that is not possible then I would suggest that 2.3 would be fine for this PR.

[RomaKis]

@dmanners,link for the pr to 2.3: #12376.

[korostii]

How about creating a new class (something like \Magento\Framework\FilesystemV2) and using that one instead (by injecting it as an extra optional parameter at Magento\OfflineShipping\Model\ResourceModel\Carrier\Tablerate::__construct()) and deprecating the old one? Would that be deemed acceptable in such a case?

[dmanners]

@RomaKis is is possible to still use getDirectoryRead instead of adding the new method? For version 2.2 we cannot add a new method to that class as it has been marked with the @api notation. Thanks

[dmanners]

@RomaKis I will close this PR as we now have a pr to 2.3 as the changes required a new function that could not be back ported. If you can think of a may to make it work for 2.2 then please feel free to open a new PR for that branch.

[RomaKis]

@dmanners, but why can't we backport it to 2.2? Seems like it doesn't break any backward compatibility http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/.

[dmanners]

@RomaKis adding a public method to a class marked with the @api notation is not allowed in patch release such as the next 2.2. This is fine going to 2.3 as it is but will need to be re-worked if we want to produce this fix for the 2.1 or 2.2 versions. http://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/index.html#introduction-of-a-method-to-a-class-or-interface

[RomaKis]

@dmanners, ok, thanks for clarification :)

[dmanners]

@RomaKis no problem. If you can see a way of getting this change into 2.2 and 2.1 without breaking BC then I would be happy to see a PR for it :)