[Loader] Add bundle resources to safe path when requested

[bobvandevijver]

Q	A
Branch?	1.0
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?
Fixed tickets	#881
License	MIT
Doc PR	Not yet, will be made after approval

This change adds the functionality to add bundle public resource paths automatically when the bundle_resources option is set. See #881 for the discussion on this.

[robfrawley]

This looks good. I'm going to clean up a few things and add some additional functionality (allow the user to whitelist and/or blacklist bundles), but otherwise I'm not against adding the option to enable this. We definetly need tests and docs, though I assume you were waiting on the "okay" before providing them.

Also, I thought you said you were going to implement a means to differentiate between bundles using a bundle prefix or something? With this setup, all paths are loadable, so given the following files

./src/FooBundle/Resources/public/foobar.jpg
./src/BarBundle/Resources/public/foobar.jpg

The user could request the file foobar.jpg outright, and since both Resources/public paths are registered, the first will be loaded. Now, I know generally people will load them using the symlinked path of bundles/[foo|bar]/foobar.jpg but it would be nice to implement some logic to allow the user to simply request @FooBundle:foobar.jpg or @BarBundle:foobar.jpg, no? What do you think about adding something like that to the mix?

Doing so shouldn't be especially difficult, as we can register the data paths array as an associative array of bundle-name => bundle-public-path, and then using the index to find the correct root when the path matches this format. What do you think?

[robfrawley]

FYI, the 1.7.3 release is scheduled for this coming Monday, February 27 2017, so be sure to get the tests and docs implemented prior if you want this included in that release. Otherwise, the next release will likely be anywhere between 3 and 6 weeks from then.

Also, you have introduced test failures ATM, though it looks like it is simply related to the addition of your configuration boolean node: https://travis-ci.org/liip/LiipImagineBundle/builds/204678829.

[bobvandevijver]

I've added some doc and updated the test to complete correctly. I also duplicated the failing test with the bundle_resources parameter set to true, but I can not check anything on the output besides having the original data root, as there are no Resources/public folders available during the test. Maybe you have a good suggestion for the extra test?

Concerning the differentiation on bundle prefix: I believe I did not say that I would implement it, although it should indeed not be to hard. However, I believe it should be a feature for another PR, and I even doubt that kind of logic should be incorporated in this bundle.

If I need to do anything for this to be merged, please let me know 👍

[robfrawley]

@bobvandevijver Can you test the updated code in your project and report back? The following changes have been implemented following your initial commits:

• Squashed your commits into one
• Rebased on top of latest 1.0 branch from liip remote
• Fixed incompatibilities on edge Symfony versions currently support by 1.0 branch
• Updated to use non-reflection method of bundle name/path resolution, if available
• Implemented blacklisting/whitelisting support for bundle auto-registration
• Implemented specific root path placeholders support via syntax @root-name:the/path/to/file.ext
• Updated to have comprehensive functional tests and unit tests with full coverage
• Updated docs to describe newer configuration syntax and expanded on included functionality in detail

[bobvandevijver]

@robfrawley Just checked it in my project, works like a charm 👍 I like the whitelist/blacklist feature by the way! Also, didn't know about the kernel.bundles_metadata, don't know why I did not find that one when I was looking for the information (and yes, I have it available)...

Looks ready to merge to me 👍

[robfrawley]

@bobvandevijver The kernel.bundles_metadata parameter seems hardly documented at all, short of one issue in the Symfony repo and the source code for the HttpKernel\Kernel class, I do not believe there is any official documentation whatsoever ... don't feel bad about not finding it: I stumbled upon it out of sheer luck. ;-) Glad to hear this PR still works as expected for you! It will be part of the 1.7.3 release, scheduled for tagging later today.

[robfrawley]

@bobvandevijver By the way, thanks for pushing this and taking the time to explain your idea in detail during my stubborn conversation with you in #881. This turned out to be a solid feature, despite my initial pessimism; I'm happy to include it in this bundle.

[robfrawley]

Feature merged and available immediately via our dev-master branch alias 1.0.x-dev - will be included in the upcoming 1.7.3 release, which should be tagged shortly.

[bobvandevijver]

@robfrawley No problem, I'm also known to be stubborn :-) And it all worked out for the best 👍

[tobias-93]

@robfrawley Could you tag the release please?

[robfrawley]

@tobias-93 Yeah, it'll be tagged today. I was trying to get a few additional PRs merged, but due to the Amazon S3 failures that directly affected Travis CI (and many other websites), jobs were queued forever and failing for no reason. Things have cleared up now and all PRs are merged for this release.

[robfrawley]

@tobias-93 It's out.