Symfony FileValidator constraint is not working with flysystem v2

[VincentLanglet]

Bug Report

Q	A
BC Break	no
Bundle version	1.18.0
Symfony version	4.4.26
PHP version	7.4

Summary

Hi @garak, I started to use this bundle with flysystem v2. It works well for uploading and downloading files. But it seems like none of my entity validation are passing now.

The FileValidator of Symfony is checking for is_file($path) and the file is never found. With gaufrette, the file was

The path is coming from https://github.com/dustin10/VichUploaderBundle/blob/master/src/Storage/GaufretteStorage.php#L77, and in the FileValidator is_path were returning true for this value.

With the flysystem, the file is now And now the check done by the FileValidator with is_file is returning false. It seems normal because no prefix is added to say to use flysystem and to precise the upload destination.

It doesn't seems easy to fix this, because the resolvePath is used in multiple places, and I assume for instance that the resolveStream method is working well since I can download files. https://github.com/dustin10/VichUploaderBundle/blob/94ba188f2603fad9bf9d2a88c61444f5e52a11df/src/Storage/FlysystemStorage.php#L79

So it would mean that sometimes, we need the "naked" path and sometimes we would need something more elaborate in order to have is_file returning true for the path. Maybe @frankdejonge could help us on the question "Which path should be use with flysystem in order to have is_file($path) returning true".

Maybe I'm taking this problem the wrong way, and we should focus about the place VichUploaderBundle is generating the file (which symfony is validating). If I understand correctly, it's done here: https://github.com/dustin10/VichUploaderBundle/blob/94ba188f2603fad9bf9d2a88c61444f5e52a11df/src/Injector/FileInjector.php#L26-L33 I would say, the file should be generated differently for Flysystem. But if we want to avoid downloading the file everytime we're loading the entity, just in order to have a correct path, we end with the same question, "Which path should be use with flysystem in order to have is_file($path) returning true". Don't know if a new method should be added to the flysystem API or flysystem-bundle API (cc @tgalopin)

@fullbl, you added the support for flysystem 2 in https://github.com/dustin10/VichUploaderBundle/pull/1179, don't you have the same issue ? Any idea how to fix it ?

[garak]

Unfortunately, my knowledge of Flysystem is not sufficient to give an opinion here. I hope that some of the mentioned people can chime in and help.

[Padam87]

This was broken in #1179.

https://github.com/dustin10/VichUploaderBundle/pull/1179/files#diff-2b8001f2c9bd1b9232145a0f4f8f046185f9449271c43eda05b15b03d369fb12L70

The absolute part has been removed.

This method is used by the FileInjector: https://github.com/dustin10/VichUploaderBundle/blob/master/src/Injector/FileInjector.php#L29

Unless I'm missing something this would be a hard fix, since flysystem doesn't provide a stream wrapper, unlike gaufrette. Until this is fixed, I would consider the flysystem storage as unsafe to use, since workarounds are needed to adapt to the situation.

You cannot rely on the injected files at all.

[frankdejonge]

Just for the record, Flysystem has never supplied a stream wrapper. There is a fileExists method on Flysystem's Filesystem object which tells you if a file exists or not. Also, the absence of a stream wrapper does not make Flysystem unsafe for use. Seems like a bit if a dramatic thing to say.

[VincentLanglet]

Just for the record, Flysystem has never supplied a stream wrapper.

I never try with the v1, but seems like VichUploader had a way to do as-if.

There is a fileExists method on Flysystem's Filesystem object which tells you if a file exists or not.

Sure, there are method for this, which are useful in a personal and specific usage.

The issue was to be compatible with the generic check is_file done by the FileValidator of Symfony. https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Validator/Constraints/FileValidator.php#L125

Also, the absence of a stream wrapper does not make Flysystem unsafe for use. Seems like a bit if a dramatic thing to say.

I think what he wanted to say is that it's unsafe to use with VichUploaderBundle, since they are currently not compatible. In my case, I soon as I started to move from gaufrette to flysystem, all my files were reported as not found by validation constraints.

[fullbl]

Sorry for the delay. Seems to me that an alternative could be to provide a custom validator (or improve Symfony's), I don't see how is_file could work with a file that is not present on the server's filesystem!

Edit: as I'm seeing, File gets constructed with a $checkPath parameter, which is not saved, nor taken into consideration into FileValidator. Taking this into account could solve the problem, otherwise the validator should use another way to provide an alternative to is_file. Also, I think none of the File functions would work, for example move() uses php rename(), which needs a file in the host filesystem, getContent uses file_get_contents, and so on!

Il dom 22 ago 2021, 14:15 Vincent Langlet @.***> ha scritto:

Just for the record, Flysystem has never supplied a stream wrapper.

I never try with the v1, but seems like VichUploader had a way to do as-if.

There is a fileExists method on Flysystem's Filesystem object which tells you if a file exists or not.

Sure, there are method for this, which are useful in a personal and specific usage.

The issue was to be compatible with the generic check is_file done by the FileValidator of Symfony.

https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Validator/Constraints/FileValidator.php#L125

Also, the absence of a stream wrapper does not make Flysystem unsafe for use. Seems like a bit if a dramatic thing to say.

I think what he wanted to say is that it's unsafe to use with VichUploaderBundle, since they are currently not compatible. In my case, I soon as I started to move from gaufrette to flysystem, all my files were reported as not found by validation constraints.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dustin10/VichUploaderBundle/issues/1217#issuecomment-903259978, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6BWUK6GULXGSUL3YST7DLT6DS7FANCNFSM5AV6QNLA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

[VincentLanglet]

In the V1 of Flysystem, Calling FilesystemAdapter::applyPathPrefix method was kinda a hack since it wasn't in the interface.

if (\is_callable([$adapter, 'applyPathPrefix'])) {
    return (string) $adapter->applyPathPrefix($path);
}

But it could still be used with V2.

The main issue with V2 is that the method Filesystem::getAdapter was removed.

Sorry for the delay. Seems to me that an alternative could be to provide a custom validator (or improve Symfony's), I don't see how is_file could work with a file that is not present on the server's filesystem!

Indeed

[Padam87]

The best solution would be for flysystem to provide a stream wrapper, like gaufrette.

[Padam87]

Just for the record, Flysystem has never supplied a stream wrapper.

@frankdejonge I've seen in the flysystem issues it has been brought up a few times.

I think you should consider it. It would make working with flysystem much easier for symfony devs, as we could use symfony's File object.

I've put one together quickly, https://gist.github.com/Padam87/3825951526bd10a735dedfa26a3bf4ed

(It has issues, like the times etc - requires work)

But Symfony's file is now usable.

        dump(new File('flysystem://contract_document/'.$contractDocument->getFileName())); die();

After this is done, Vich Uploader could simply use the same logic as the gaufrette storage. https://github.com/dustin10/VichUploaderBundle/blob/master/src/Storage/GaufretteStorage.php#L78

BTW, this would allow us to drop gaufrette, as that projects seems to be EOL (or at least taking a break)

[frankdejonge]

There are community provided stream-wrappers available. I'm not interested in maintaining one. Stream wrappers have to make trade-offs, more severe ones than the normal filesystem trade-offs need. Maintaining one will be a constant battle between opinions while I personally think they should not be used and I'm not looking to get that burden on my shoulders. I personally think it's a design flaw that a local filesystem needs to be faked, often with severe performance ramifications, to be able to check if something is a file. The tight coupling with symfony's internals causes this, so I think a better way to deal with it is to address the root cause, which is IMO not the absence of a stream wrapper. Maintaining Flysystem is mostly a one-man thin. I'm grateful that I'm getting more help now with a couple of adapters, but I can only spend my time once, and I don't feel much for spending it on this.

[Padam87]

@frankdejonge Time is always limited on OS projects, and I fully understand why you wouldn't want to maintain a wrapper.

Unfortunately community packages are outdated, and I would imagine @garak would be against relying on one of those in the first place.

Welp, I don't really see any other way to do this without a stream wrapper.

[tgalopin]

Perhaps an intermediate solution could be to provide the stream wrapper in flysystem-bundle? I guess it would make sense as it'll mostly used by Symfony devs and I'd be fine maintaining it.

[VincentLanglet]

Perhaps an intermediate solution could be to provide the stream wrapper in flysystem-bundle? I guess it would make sense as it'll mostly used by Symfony devs and I'd be fine maintaining it.

This would be awesome.

If I understand correctly, you know how to do it @Padam87 ? Do you mind doing a PR on the flysystem bundle ?

[frankdejonge]

@tgalopin if you want to maintain it I can create a separate repository for this so people can depend on the stream wrapper directly. Let's touch base on the slack 👍

[tgalopin]

As discussed with Frank, I'm working on a small lib implementing a stream wrapper for Flysystem. I hope to release a first version soon.

[VincentLanglet]

As discussed with Frank, I'm working on a small lib implementing a stream wrapper for Flysystem. I hope to release a first version soon.

Hi @tgalopin ; did you have time to implement the stream wrapper ? :)

[tgalopin]

Hello Vincent,

I started to do so but didn't manage to finish yet. I'll push initial code soon so that you can contribute if you wish.

[kor3k]

@VincentLanglet @tgalopin maybe take a look at this https://github.com/m2mtech/flysystem-stream-wrapper

[VincentLanglet]

As discussed with Frank, I'm working on a small lib implementing a stream wrapper for Flysystem. I hope to release a first version soon.

With gaufrette being deprecated, having an "official" stream-wrapper for flysystem seems even more useful now.

I started to do so but didn't manage to finish yet. I'll push initial code soon so that you can contribute if you wish.

What is the current state/link of the WIP library ?

[SebastienTainon]

This seems to be a blocking issue for anyone wanting to use VichUploaderBundle in conjunction with a Flysystem abstraction and Symfony Validator constraints like @Assert\Image(maxSize = "5M"), a situation I'm currently facing too...