Skip to content

Conversation

@birchestx
Copy link

In TableRates Shipping was getting an error saying it cant find the file var/tmp/private/var/tmp/

Looking at the code the file open in the import is looking for a relative path. Its not possible to change the upload path for a file so it's relative. Its adding on var/tmp to front of the directory it's reading here:

$tmpDirectory = $this->_filesystem->getDirectoryRead(DirectoryList::SYS_TMP)

It seems from initial look that getDirectoryRead has to have a path in the list passed in, so is always relative.

I've updated it to use the Io\File instead which resolves the issue.

I looked at adding unit tests but you dont have anything similar so I figured the pull request could get refused if I add too much test code in there. When I get to adding tests for own extn will submit them then as is very similar logic.

@danslo
Copy link
Contributor

danslo commented Jan 11, 2015

Just going to throw a couple of things out there:

  • Works fine for me on my (linux) system, I suspect this will have something to do with relative paths in Windows.
  • You'll definitely want a test here. I very much doubt they will reject your PR because it contains too much testing.
  • You're instantiating a new object yourself instead of using dependency injection. (and even if that was ok, there is now a \Magento\Framework\Filesystem dependency that is never used).
  • The formatting is off.

@birchestx
Copy link
Author

Appreciate the comments.

  • This is on OSX Mac FYI
  • I didn't want to do DI as they hadn't was trying to keep it simple
  • Agreed on the FileSystem - I didn't want to screw around with this in their code as it starts becoming much bigger
  • Agreed on the test but there is nothing in there similar and there is no way they will approve me making whole scale changes to tests, so I'd rather focus on just getting this raised.

I'm guessing they will reject it, but its not right as it is. Better to speak up and be wrong than not to speak up at all.

@vpelipenko
Copy link
Contributor

@wsakaren could you provide more background of how to reproduce this issue? We've tried to do that but without success.

@birchestx
Copy link
Author

I'll raise this as an issue instead, seems like not happening to all, will put steps to reproduce

@birchestx birchestx closed this Jan 18, 2015
magento-team pushed a commit that referenced this pull request Apr 11, 2017
[EAST] [Performance] Anomaly increase in redis traffic
magento-engcom-team added a commit that referenced this pull request Sep 25, 2019
 - Merge Pull Request magento/graphql-ce#952 from pmclain/graphql-ce:issue/926
 - Merged commits:
   1. 5b32e8e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants