[Loader] Abstract filesystem resource locator and legacy insecure locator implementation

[robfrawley]

Q	A
Branch?	1.0
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#846
License	MIT
Doc PR	n/a

Abstract file locator, specifically FileSystemLocator and FileSystemInsecureLocator, outside of their respective "loader" classes to allow for multiple implementations of resource resolution. This includes an insecure resource locator version that allows symbolic links to be passed and blindly followed with no regard as to where they might point. This fixes environments that rely on deploy tools using symbolic links.

The question is, do we want to offer this functionality? @antoligy @cedricziel @davidromani

[robfrawley]

Note that the FileSystemInsecureLocator is missing a comprehensive test suite; this will be added later if the consensus is to add this functionality. Additionally, this is a first-pass at tearing out file resource resolution from loaders, and will likely be refactored and cleaned up; right now we just need to figure out if this is something we should allow out-of-the-box.

[robfrawley]

Thoughts as to whether we want to offer a file locator (optional) implementation that is less secure, but allows symbolic links? @antoligy @cedricziel

[alexwilson]

Might be worth keeping this to multiple lines for readability?

[robfrawley]

Yeah, fair enough. I hate multi-line constructors (unless it's well past 120 characters). That said, I know many are big on it (and an 80 character line limit). I'll revert. ;-) 👍

[alexwilson]

I wouldn't mind them so much if we could typehint on primitives... Grr... 😛

[alexwilson]

Seems like a good answer to this issue, without introducing too much complexity for future maintenance, so I'm happy with this!

[alexwilson]

My opinion is that it's worth adding the InsecureFileLoader, that way we can document that this practice is not recommended, without breaking existing deployments.

Happy for a test suite to appear in a future PR just so we can verify that this works, but I see it should function the same way as before we improved resolution.

[alexwilson]

Went ahead and resolved the conflict (loving that you can do this now!)- Janky commit but it should be resolved by a squash!

[robfrawley]

@antoligy Thanks for the conflict resolution! Since you're on board with this approach, I'm gonna go ahead and add the tests to this PR tonight before merging (I don't like the idea of adding any new code without comprehensive tests ;-). Should we release 1.7.2 once this is merged? We have a decent amount of additions for a patch release, no? Or should we wait a bit?

[alexwilson]

Yeah that sounds like a good idea and I think so, as it addresses probably the only repeat complaint since 1.7.0 was released!

[robfrawley]

Right, sounds good. 👍 I'll get the tests implemented. Of note, I was just granted collaborator access to the avalanche123/Imagine repository today, so that should help us push items that effects this bundle upstream in the future.

[davidromani]

I think that it is a very good idea. FileSystemInsecureLocator can help me a lot, I prefer to release ASAP.

Thank you so much guys.

[robfrawley]

@antoligy Can you give this an additional once-over. I moved some stuff around, created a deprecation layer for FileSystemLoader::__construct() so we can change the method signature in 2.0, moved to services for FileSystemLocator implementations, refactored the locator classes a bit to remove as much duplication as possible, and changed the configuration to an enum value. Comprehensive tests have been implemented, as well.

[robfrawley]

@davidromani Before merging this, I'd like to confirm it does resolve your symbolic link issue. Can you patch your vendor release of this bundle with the PR and write back confirming proper functionality?

To apply this PR as a patch, you must perform the following:

# install the `master` from source
cd vendor/liip; rm -fr imagine-bundle
git clone https://github.com/liip/LiipImagineBundle.git imagine-bundle
cd imagine-bundle

# apply this PR as a patch
curl -L https://github.com/liip/LiipImagineBundle/pull/866.diff | patch -p1

# clear your cached assets
rm -fr web/path/to/cached/images/

Then enable the new filesystem_insecure locator by adding the following to your configuration:

liip_imagine :
  loaders:
    default:
      filesystem:
        locator: filesystem_insecure

[davidromani]

@robfrawley Yes, it works! Thank you so much!!

[alexwilson]

Nice work all! 🙂

[robfrawley]

@davidromani The 1.7.2 release has just been published and you can immediately upgrade by calling composer via composer require liip/imagine-bundle:~1.7.2.