Adaptors for SFTP/FTP #58

GwendolenLynch · 2017-08-01T07:44:34Z

No description provided.

CarsonF

Looking again path prefix does not double apply the prefix as comments state, but it should not be used anyways. get/setRoot should be used instead.

CarsonF · 2017-08-01T16:52:59Z

src/Adapter/Sftp.php

+class Sftp extends SftpAdapter
+{
+    /** @var int */
+    protected $filePerm = 0644;


This is not needed. Parent has permPublic and permPrivate for this.

CarsonF · 2017-08-01T16:54:15Z

src/Adapter/Sftp.php

+     */
+    public function __construct(array $config)
+    {
+        $this->configurable[] = 'filePerm';


configurable just calls setters for config array. There is no setFilePerm so this does nothing, also not needed as mentioned above.

CarsonF · 2017-08-01T16:54:40Z

src/Adapter/Sftp.php

+        $location = $this->applyPathPrefix($path);
+
+        return (bool) $this->getMetadata($location);
+    }


~~Path prefix is already applied to connection object. This double applies it.~~
get/setRoot should be used instead.

CarsonF · 2017-08-01T16:56:03Z

src/Adapter/Sftp.php

+        }
+
+        return $metadata;
+    }


Where is path needed?

Ah I see that our Filesystem::getTypeFromMetadata needs it. That's a bug that should be fixed there instead.

// Right before getTypeFromMetadata call in doGetType method. $metadata += ['path' => $path];

Done. 3222738

Thanks mate, well appreciated!

CarsonF · 2017-08-01T16:57:23Z

src/Adapter/Sftp.php

+        }
+
+        return parent::createDir($dirname, $config);
+    }


~~Double applying path prefix.~~
get/setRoot should be used instead.

CarsonF · 2017-08-01T16:59:13Z

src/Adapter/Ftp.php

+    /** @var int */
+    protected $filePerm = 0664;
+    /** @var int */
+    protected $directoryPerm = 0775;


This should be split into two values - one for public and one for private.

OK, starting to get a picture of 6 months ago … A lot of running around in circles. 🤣

Upstream uses $directoryPerm

CarsonF · 2017-08-01T16:59:27Z

src/Adapter/Ftp.php

+     */
+    public function __construct(array $config)
+    {
+        $this->configurable[] ='directoryPerm';


No setter, does nothing.

[Bolt\Filesystem\Exception\IOException] Notice: Undefined property: Bolt\Filesystem\Adapter\Ftp::$directoryPerm

Correct reply in the correct place always helps 🤣

It's hard to tell without a stack trace, but that seems unrelated. You sure you didn't remove the class property and then still call it later in the class when getting that error?

Literally all $configurable is for is to call setters for config options in setConfig.

Removing the constructor.

CarsonF · 2017-08-01T17:01:21Z

src/Adapter/Ftp.php

+        $location = $this->applyPathPrefix($path);
+
+        return (bool) $this->getMetadata($location);
+    }


~~This double applies path prefix.~~
get/setRoot should be used instead.

CarsonF · 2017-08-01T17:01:34Z

src/Adapter/Ftp.php

+        }
+
+        return parent::createDir($dirname, $config);
+    }


~~This double applies path prefix.~~
get/setRoot should be used instead.

CarsonF · 2017-08-01T17:02:18Z

src/Adapter/Ftp.php

+    {
+        try {
+            $result = parent::createActualDirectory($directory, $connection);
+        } catch (ContextErrorException $e) {


This is only thrown if Symfony's Error handler is hooked up. If you are trying to silence errors set a temporary error handler or use @.

GwendolenLynch · 2017-08-07T13:28:26Z

OK, Monday … so let's get back to this 😄

Everything address, but the sole point for discussion (based on last review) was the use of protected $directoryPerm.

The reasoning I went for a single class value is that matches the exact name & use in the upstream SFTP adaptor. Splitting that into two parameters would mean re-implementing several more methods in our SFTP adaptor, and I went with keeping the FTP one consistent. So impasse?

CarsonF

Looks good, other than the "for mirror() to work" comments. Filesystem just needs a truthy value return or it throws an exception. mirror() doesn't have anything to do with this.

GwendolenLynch · 2017-08-09T07:55:24Z

OK … I got back to the bottom of this!

So, the AdapterInterface::createDir() PHPDoc says that it returns array|false, where-as FilesystemInterface::createDir() PHPDoc says that it returns bool with the attached comment of "True on success, false on failure."

Now given the clueless ~~<censored>~~ that did those docblocks, it is entirely feasible that the Clueless Level was over 9,000. That, and our mirror() doesn't care what is returned by createDir(), so for once reading the upstream code was (originally) a disadvantage to me 🤣

I'll drop those bits and we can move on 👍

CarsonF · 2017-08-09T14:10:51Z

Well for one we don't use Flysystem's FilesystemInterface. And two that interface and the adapters usually do return different values in an attempt to reduce code in the adapter. But yeah in this case since we know the adapter is only going to be used with our filesystem it's ok just to return a bool.

GwendolenLynch · 2017-08-09T14:18:00Z

Yeah, just pointing to the origin of what we were debating, not defending it.

CarsonF · 2017-08-09T15:34:41Z

src/Adapter/Ftp.php

+class Ftp extends FtpAdapter
+{
+    /** @var int */
+    protected $directoryPerm = 0750;


Sftp's default is 0744. Should we update this one or that one to match?

Yes, I had copy/pasta what I was using (over & over & over again) without thinking. It is a small but important distinction and you are 100% correct! Done.

Getting CarsonF's attention for merge 😜

GwendolenLynch added 3 commits June 2, 2017 12:52

Add league/flysystem-sftp ^1.0 to require-dev

f72be76

SFTP adapter

c27ec80

FTP adapter

958ba61

GwendolenLynch requested a review from CarsonF August 1, 2017 07:44

GwendolenLynch mentioned this pull request Aug 1, 2017

[Tracker] Bolt 3.4 Release Blocking Issues bolt/bolt#6412

Closed

CarsonF previously requested changes Aug 1, 2017

View reviewed changes

GwendolenLynch force-pushed the feature/s-ftp branch 2 times, most recently from b86ee78 to 9bbe81e Compare August 4, 2017 14:14

CarsonF reviewed Aug 8, 2017

View reviewed changes

GwendolenLynch force-pushed the feature/s-ftp branch from 9bbe81e to b5e713b Compare August 9, 2017 08:43

CarsonF reviewed Aug 9, 2017

View reviewed changes

GwendolenLynch force-pushed the feature/s-ftp branch from b5e713b to 958ba61 Compare August 10, 2017 04:42

CarsonF approved these changes Aug 10, 2017

View reviewed changes

CarsonF merged commit fe6b5bb into master Aug 10, 2017

CarsonF deleted the feature/s-ftp branch August 10, 2017 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adaptors for SFTP/FTP #58

Adaptors for SFTP/FTP #58

GwendolenLynch commented Aug 1, 2017

CarsonF left a comment

CarsonF Aug 1, 2017

CarsonF Aug 1, 2017

CarsonF Aug 1, 2017 •

edited by GwendolenLynch

Loading

CarsonF Aug 1, 2017

CarsonF Aug 1, 2017

CarsonF Aug 1, 2017

GwendolenLynch Aug 1, 2017

CarsonF Aug 1, 2017 •

edited by GwendolenLynch

Loading

CarsonF Aug 1, 2017

GwendolenLynch Aug 4, 2017

CarsonF Aug 1, 2017

GwendolenLynch Aug 2, 2017

GwendolenLynch Aug 2, 2017

CarsonF Aug 2, 2017

GwendolenLynch Aug 3, 2017

CarsonF Aug 1, 2017 •

edited by GwendolenLynch

Loading

CarsonF Aug 1, 2017 •

edited by GwendolenLynch

Loading

GwendolenLynch Aug 4, 2017

CarsonF Aug 1, 2017

GwendolenLynch commented Aug 7, 2017

CarsonF left a comment

GwendolenLynch commented Aug 9, 2017

CarsonF commented Aug 9, 2017

GwendolenLynch commented Aug 9, 2017

CarsonF Aug 9, 2017

GwendolenLynch Aug 10, 2017

Adaptors for SFTP/FTP #58

Adaptors for SFTP/FTP #58

Conversation

GwendolenLynch commented Aug 1, 2017

CarsonF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarsonF Aug 1, 2017 • edited by GwendolenLynch Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarsonF Aug 1, 2017 • edited by GwendolenLynch Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarsonF Aug 1, 2017 • edited by GwendolenLynch Loading

Choose a reason for hiding this comment

CarsonF Aug 1, 2017 • edited by GwendolenLynch Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GwendolenLynch commented Aug 7, 2017

CarsonF left a comment

Choose a reason for hiding this comment

GwendolenLynch commented Aug 9, 2017

CarsonF commented Aug 9, 2017

GwendolenLynch commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarsonF Aug 1, 2017 •

edited by GwendolenLynch

Loading

CarsonF Aug 1, 2017 •

edited by GwendolenLynch

Loading

CarsonF Aug 1, 2017 •

edited by GwendolenLynch

Loading

CarsonF Aug 1, 2017 •

edited by GwendolenLynch

Loading