Support for sftp - #88
Conversation
7279bc1 to
c6ec760
Compare
c6ec760 to
360cdc6
Compare
|
|
||
| public function getLocationConfig(): LocationConfig | ||
| { | ||
| return new LocationConfig( |
There was a problem hiding this comment.
Would this not be better using a factory rather than "new" ing?
There was a problem hiding this comment.
Yeah would be, followed here convention in the module, will change it
|
|
||
| public function finish(Source $source): Result | ||
| { | ||
| return new Result([]); |
There was a problem hiding this comment.
Would this not be better using a factory rather than "new" ing?
There was a problem hiding this comment.
Yeah would be, followed here convention in the module, will change it
| use JhImport\Import\Config\AppConfigProvider; | ||
| use JhImport\Import\Source\Csv; | ||
| use JhImport\Import\Source\Db; | ||
| use JhImport\Import\Source\SftpCsv; | ||
| use JhImport\Import\Source\Webapi; |
There was a problem hiding this comment.
ah yes, I worked on it in CDC project so I can test it and there is a conflict there as namespace is JH so had to change it locally, and then when commiting I must have forgotten to change it back
| use JhImport\Import\Archiver\Archiver; | ||
| use JhImport\Import\Config; | ||
| use JhImport\Import\Source\SftpCsv; |
|
|
||
| $this->localTempFile = $localTempFile; | ||
| $this->client->get($this->remoteFile, $this->localTempFile); | ||
| $this->csv = new Csv($this->localTempFile, $delimiter, $enclosure, $escape, $headerRowNum); |
There was a problem hiding this comment.
Should this use a factory rather than "new" ing?
There was a problem hiding this comment.
Yeah would be, followed here convention in the module, will change it
| } | ||
|
|
||
| return array_values(array_filter(array_map( | ||
| static fn (string $file): string => $remotePath . '/' . $file, |
There was a problem hiding this comment.
I'd recommend using DIRECTORY_SEPARATOR rather than hardcoding "/" for better cross-OS support
There was a problem hiding this comment.
Yeah you are right
| { | ||
| $basename = basename($file); | ||
|
|
||
| return $basename !== '.' && $basename !== '..' && !str_contains($basename, '..'); |
There was a problem hiding this comment.
Edge case, but technically file names could contain ".." (my-import..csv would be valid, for example)
| */ | ||
| private function assertWithinRemotePath(string $remoteFile): void | ||
| { | ||
| $remotePath = rtrim($this->locationConfig->getRemotePath(), '/') . '/'; |
There was a problem hiding this comment.
Again, would suggest DIRECTORY_SEPARATOR for better support as this is a public module
There was a problem hiding this comment.
Yeah you are right
| { | ||
| $remotePath = rtrim($this->locationConfig->getRemotePath(), '/') . '/'; | ||
|
|
||
| if (str_contains($remoteFile, '..') || !str_starts_with($remoteFile, $remotePath)) { |
There was a problem hiding this comment.
Again, files may contain ".." and be valid - the path itself shouldn't (though you can also use realpath to expand these out and then validate the end result is in an expected parent and avoid the situation entirely)
| return $this->connection; | ||
| } | ||
|
|
||
| $sftp = new SFTP($this->locationConfig->getHost(), $this->locationConfig->getPort()); |
There was a problem hiding this comment.
Might be worth a comment that this cannot use a factory because it's not a Magento module so autogeneration won't work (well, not in later versions of Magento - some older versions allowed autogen of library classes, which broke the Klevu php-sdk later because we'd relied on that)
Just in case anyone like me comes along and goes "why is this 'new'-ed?"
There was a problem hiding this comment.
Yeah factory wouldn't work, will add a comment
3 participants
This pull request adds support for a new
sftpfilesimport type, enabling file imports over SFTP with configurable connection details and archiving behavior. It introduces a new SFTP archiver, updates configuration handling to support the new type, and provides a fully working example module to demonstrate usage. Additionally, the project is now explicitly licensed under the MIT License.New SFTP Import Type and Example Module
sftpfilesas a new import type, including configuration schema, type registration, and example implementation:Converterto recognize and processsftpfilesimports, with fields for SFTP connection, file matching, archiving, and processing options (F0bed31fR1, src/Config/Converter.phpR68-R78).examples/sftpfiles/MyVendor/Import) with DI and import configuration, a sampleLocationConfigProvider, and minimalSpecificationandWriterclasses [1] [2] [3] [4] [5] [6] [7].SFTP Archiving Support
SftpArchiverclass to handle archiving or deleting imported files on the remote SFTP server after successful import, and mapped it to the new source type in the archiver factory [1] [2] [3].General Improvements and Maintenance
Converter[1] [2] [3] [4] [5] [6] [7].