Rewrite FTP ext storage impl to use an FTP connection/session

[PVince81]

Currently, the FTP ext storage is using a URL-based approach.
It calls stat('ftp://...'), fopen('ftp://'...') etc without keeping an explicit connection.

It seems that this approach has many issues:

1. PHP can't get mtime for directories using the URL approach, neither with stat() not with mtime(): Mounted FTP shows wrong change date ("Years ago" - 01.01.1970) #6395 which makes it impossible to detect folder changes

2. Some PHP environments have stat() failing for some unknown reasons Problem accessing FTP server using external storage #5655 OC5.0.6 - External Storage - FTP - unable to mount share #3408

We should maybe rewrite it to use ftp_connect() instead which might provide a more stable and compatible connection.

Let's discuss @icewind1991 @karlitschek @DeepDiver1975 @schiesbn

Before going with a full fledged implementation, we must try out whether the ftp_connect() approach can solve the above issues.

[karlitschek]

makes sense!

[DeepDiver1975]

Do we have any clue who often ftp is used as external storage system?
The current implementation using streamwrappers was actually a low-brainer and implemented with little effort.

I'd suggest to invest time and effort only if there is a significant usage/request - because honestly speaking:
I don't care about ftp as long as we have more urgent topics - e.g. get rid of 'Shared' folder, failing unit tests on oracle, broken MSSQL support, too many sql queries on each http request ....

[PVince81]

I just raised this to make sure the idea / possible solution doesn't get lost...

[DeepDiver1975]

I just raised this to make sure the idea / possible solution doesn't get lost...

Sure - thanks a lot! Please don't get me wrong - I just want to make sure that we focus on certain areas and topics.

[PVince81]

@j-ed would you like to have a go at this ?
In #3408 you already did some nice testing related to ftp_connect() 😄

[j-ed]

@PVince81 Let me have a look on the source in detail before I commit to take over the job ;-)

[PVince81]

@j-ed sure. Let us know if you need any pointers. 😄

[j-ed]

@PVince81 Are you available on #owncloud-dev in the evening? I need some hints how OC is creating the mount point and how/where the decision is felt if it should be shown as a file or directory. I'm currently testing a custom FTP stat function and the mount status shows is already "green" but the mount point is still shown as a file and not a sub directory.

[PVince81]

@j-ed sorry I was already off and didn't see your message.
Is your nickname the same on IRC ? (mine is)

First the class hierarchy: FTP storage extends \OC\Files\Storage\StreamWrapper defined in apps/files_external/lib/streamwrapper.php which itself extends \OC\Files\Storage\Common defined in lib/private/files/storage/common.php.

To distinguish between files and folders, the call goes as follows:

1. Calls is_dir() on the FTP external storage class
2. is_dir() calls $this->filetype()
3. filetype() calls the PHP filetype using the FTP URL, which I believe might internally call stat().

Does your stat() implementation returns the correct file type ?

If you like you can submit your experiment as a PR and put "WIP" in the title so I can check it.

[cdamken]

@PVince81 this solution could help for #11797 and #10820 too

[j-ed]

I've already spent some time on this issue and tried to overwork this function but the code never found its way into a pull request because I had stuck at some point and @PVince81 and I weren't able to find the root cause of the problem. If someone is interested in that code please let me know.

[RobinMcCorkell]

@j-ed did you push your code to a branch here on GitHub? I'd be interested in looking at it

[j-ed]

Here you will find my attempt to create a custom FTP stat() function:
https://github.com/j-ed/core/blob/master/apps/files_external/lib/ftp.php

Unfortunately @PVince81 and I weren't able to identified what else
need to be modified to get this peace of code running.

[cdamken]

@PVince81 Any news?

[PVince81]

This hasn't been scheduled.
If needed please raise priority.

[cdamken]

@craigpg can be this scheduled?

[PVince81]

@Xenopathic did you get a chance to have a look ?

@cdamken it is not confirmed yet that this solution would fix the issues, so first thing would be to research whether it does.

[craigpg]

@cdamdken, please work with @markviens et. al. to get this properly scheduled.

[RobinMcCorkell]

Not yet unfortunately, but it's definitely something I will look at, hopefully for oC 8.1

[PVince81]

In that case I'll set the milestone to 8.1 and assign to you if you don't mind. 😄
Let us know (with a WIP PR) if you start working on this.

[RobinMcCorkell]

Will be redundant with #14551

[oparoz]

One more reason to do it #16906
Hopefully flysystem will be the solution...

[RobinMcCorkell]

Closing as we want to move to Flysystem

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.