Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Video (local) is not displayed if it is in a symlink folder #7313

Closed
ufeldmann opened this issue Jun 28, 2024 · 17 comments · Fixed by #7319
Closed

Video (local) is not displayed if it is in a symlink folder #7313

ufeldmann opened this issue Jun 28, 2024 · 17 comments · Fixed by #7319
Labels
bug up for discussion Issues and PRs which will be discussed in our monthly Mumble calls.
Milestone

Comments

@ufeldmann
Copy link

Affected version(s)

5.3.10

Description

I have a problem with the integration of local videos in our deployment structure.
We are symlinking a content folder in the files folder so that it persists across releases.

Our structure is the following:
current (symlink to the current release in the releases folder, the document root is current/public)
releases (folder with the individual releases)
shared/files/content (folder with the files. In the release folder a symlink is created from files/content to this folder)

If I upload a video in the files/content folder it is available in Contao.
Then I use the “Video/Audio” content element and embed the video there, it will also be displayed to me when editing the content element, but in the backend list, where the preview is already rendered, it will not displayed.
The entire element is not rendered in the frontend on the website.

If I create a new folder in files directly in the release that is not a symlink, then it works perfectly.
So it must be due to the symlink logic (maybe symlink to symlink).

In 5.3.9 we also had problems with images in the symlinks, but that is fixed in 5.3.10.
Maybe this also has something to do with.

Thanks for all your hard work!

@Toflar
Copy link
Member

Toflar commented Jun 28, 2024

@ausi this sounds related to #7294 maybe?

@fritzmg
Copy link
Contributor

fritzmg commented Jun 28, 2024

@ufeldmann so the file shared/files/content/.public exists? (i.e. the files/content folder is set to "Public"?)

@ufeldmann
Copy link
Author

@fritzmg yes it is set to public, all other types of files, such as images or downloads, work.

@fritzmg
Copy link
Contributor

fritzmg commented Jun 28, 2024

Not sure why it wouldn't work just for videos. Either all or no files should be accessible. Can you post a link to an example?

@fritzmg
Copy link
Contributor

fritzmg commented Jun 28, 2024

To clarify: the video element does not even render, even though a valid video from a public folder was selected.

@ausi
Copy link
Member

ausi commented Jun 28, 2024

this sounds related to #7294 maybe?

Probably related, but the video element does not use the FigureBuilder AFAIK, so the source of the issue must be different I guess.

Probably needs to be debugged here:

if (!$publicUri = $this->filesStorage->generatePublicUri($item->getPath())) {
continue;
}

@fritzmg fritzmg added this to the 5.3 milestone Jun 29, 2024
@fritzmg
Copy link
Contributor

fritzmg commented Jun 29, 2024

I can confirm the issue.

And yes, $this->filesStorage->generatePublicUri($item->getPath()) only returns null. Our SymlinkedLocalFilesProvider returns null here:

if ($options || $adapter !== $this->localFilesAdapter) {
return null;
}

Because $adapter is a LocalFileSystemAdapter with

rootLocation: "C:/Users/fmg/www/c5x/_test"

(the symlink target in my case) and $this->localFilesAdapter is a LocalFileSystemAdapter with

rootLocation: "C:\Users\fmg\www\c5x/files"

I do not know how to fix this though as I am unsure how this is intended to work.

@ausi
Copy link
Member

ausi commented Jun 29, 2024

I do not know how to fix this though as I am unsure how this is intended to work.

Maybe as described in #5869 (comment) ?

@fritzmg
Copy link
Contributor

fritzmg commented Jun 29, 2024

Hm, I still don't quite understand why we need all this complexity. If I remove ConfigureFilesystemPass::mountAdaptersForSymlinks altogether it works (and so do images for example).

@m-vo
Copy link
Member

m-vo commented Jun 30, 2024

Maybe I can clarify how things are intended to work. There are two pieces, that are relevant.

  1. A path in the VFS storage describes a location in that storage. There is no concept of absolute paths, because this does not apply to any adapter other than the local filesystem adapter. That's why streaming is the primary way to get data out of the VFS. In cases where streaming is not possible or feasible, you likely want a public URI - a resource, that a client browser could request (could for instance be an URI to a 3rd party server hosting the asset). Because we know that the application's webserver serves files from public/files which points to the filesStorage, we ship a public path provider that answers all requests for public paths in that storage.

  2. Flysystem intentionally does not support symlinks (e.g. see 1, 2, 3). I think the reasoning is fair. For our system (assuming the VFS would be integrated everywhere already), there would also be no reason you would need symlinks. Everything can also be achieved by mounting an adapter. But because the VFS is not integrated everywhere, yet, we have a workaround in place that does just that for you: mounting adapters for every symlink.

IMHO the correct solution to getting generatePublicUri() to work for those automatically mounted adapters would be to enhance our SymlinkedLocalFilesProvider to also support these adapters. Currently we only support the localFilesAdapater:

if ($options || $adapter !== $this->localFilesAdapter) {
return null;
}

We could do something like this:

  1. Add a public method registerAdditionalAdapter(FilesystemAdapter $adapter, string $root): void to the SymlinkedLocalFilesProvider.
  2. Adjust the getUri() method to also handle these additional adapters.
  3. Add a call to this method in ConfigureFilesystemPass#mountAdaptersForSymlinks() for each mounted adapter.

@fritzmg
Copy link
Contributor

fritzmg commented Jul 1, 2024

But in what way does Flysystem not support symlinks? The whole point of symlinks is to map directories or files to different physical locations seamlessly - and that seems to work with Flysystem too. After all why would Flysystem care whether something in its relative path is actually a Symlink to somewhere else?

Furthermore if you use the shared_files feature of Deployer to keep files between deployments, you will always have symlinks.

@leofeyer leofeyer added the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Jul 1, 2024
@m-vo
Copy link
Member

m-vo commented Jul 1, 2024

I don't know what you mean? It does not work with filesystem (at least for me). The local adapter does not follow links which is a design decision according to the issues I linked to from the flysystem repo (see above). You could create your own adapter, that worked differently but you would probably run into the outlined issues.

IMHO we already have a workaround, that we just need to tweak a little bit.

@ausi
Copy link
Member

ausi commented Jul 1, 2024

But in what way does Flysystem not support symlinks?

There is an issue that showed that they actually do not work with Contao and flysystem: #4277

@m-vo
Copy link
Member

m-vo commented Jul 2, 2024

See #7319.

@m-vo m-vo closed this as completed Jul 2, 2024
@fritzmg
Copy link
Contributor

fritzmg commented Jul 3, 2024

I don't know what you mean? It does not work with filesystem (at least for me).

How exactly did you test? May be it is OS dependent somehow?

@m-vo
Copy link
Member

m-vo commented Jul 3, 2024

https://github.com/thephpleague/flysystem/blob/3.x/src/Local/LocalFilesystemAdapter.php#L217-L222 - and our configuration of local adapter is already to ignore/skip, rather than throw.

leofeyer pushed a commit that referenced this issue Jul 3, 2024
…links (see #7319)

Description
-----------

Fixes #7313.

This implements the changes as outlined in #7313 (comment).

@ufeldmann Can you check if this solves your problem?

Commits
-------

af2d30d support multiple adapters in the SymlinkedLocalFilesProvider
4156aa2 also configure SymlinkedLocalFilesProvider for automatically added ad…
3a0283b adjust tests
3825cde move local filesystem adapter from dev to regular dependencies
ee49e75 use a weak map instead of manually calling spl_object_Hash
leofeyer pushed a commit to contao/core-bundle that referenced this issue Jul 3, 2024
…links (see #7319)

Description
-----------

Fixes #7313.

This implements the changes as outlined in contao/contao#7313 (comment).

@ufeldmann Can you check if this solves your problem?

Commits
-------

af2d30da support multiple adapters in the SymlinkedLocalFilesProvider
4156aa2f also configure SymlinkedLocalFilesProvider for automatically added ad…
3a0283b1 adjust tests
3825cde5 move local filesystem adapter from dev to regular dependencies
ee49e750 use a weak map instead of manually calling spl_object_Hash
@fritzmg
Copy link
Contributor

fritzmg commented Jul 4, 2024

https://github.com/thephpleague/flysystem/blob/3.x/src/Local/LocalFilesystemAdapter.php#L217-L222

I see, so it only affects listing folder content. That's even more arbitrary 🙄.

That's why singular images or video files are not affected by this and work out of symlinked directories normally.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug up for discussion Issues and PRs which will be discussed in our monthly Mumble calls.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants