Allow multiple root paths for FileSystemLoader

[robfrawley]

Allow multiple data roots for file system loader as a possible approach to resolve #846. Includes beforeNormalization step in configuration tree-builder to allow prior string values for data_root bundle configuration to be seamlessly converted into an array. For example, both of the following are now valid configurations:

liip_imagine:
  loaders:
    default:
      filesystem:
        data_root: "/your/data/root"

liip_imagine:
  loaders:
    default:
      filesystem:
        data_root: 
          - "/your/data/root/a"
          - "/your/data/root/b"

This also adds a "feature" in that we can look in multiple locations using the same file system loader now, which I suspect could be beneficial for some use-cases. Most importantly, though, this implementation doesn't break any prior unit tests (but it does add a few unit tests to cover the new cases introduced).

[robfrawley]

Amended PR to support PHP 5.3 ($this closure calling context isn't properly implemented in 5.3).

[robfrawley]

@antoligy @cedricziel How do you guys feel about this feature, as both a ligament new feature (regardless of symbolic links) and a possible fix for those attempting to use multiple symbolic links that point outside a single path (see #846).

[robfrawley]

Amended PR to add a test case that ensures configuration data_root string is correctly normalized to an array by the configuration tree processor.

[alexwilson]

Nice, with that latest addition this should be fully backwards compatible with the latest release! 🙂

I'm very happy with this approach, not least of all as it addresses the "real" issue, but also as it adds more thorough tests.
I still think that it is worth amending the documentation to better detail this behaviour, however that's something which we can address later. For the meantime, no objections from me, but I will take a more in-depth look first before hitting approve.

[alexwilson]

Looks good!

Wondering if it's worth standardising how we format the display of multiple paths in the future (I rather like /whatever/:/foo/bar/, but I'm not sure if this is defined).

[robfrawley]

@antoligy I too like the format /foo/bar:/bar/foo, but no, I do not believe such is well-defined elsewhere. I do agree we should standardize on something. Perhaps we should investigate and see where similar usage might be appropriate. Also, docs PR will be incoming shortly that details this behavior, as well as provides some concrete information for commonly encountered errors.

[robfrawley]

So, I'm pretty positive this doesn't result in any BC breaks: the configuration value for data_root continues to support a scalar alongside the newly supported type of scalar[] (an array of scalars); the loader class FileSystemLoader continues to support a scalar alongside the newly supported type of scalar[] for the $dataRoots parameter; etc. In short, the API hasn't changed and continues to operate as expected using either the old or new data types. But, with that said...

A possible BC break exists for someone extending the FileSystemLoader class: anyone relying on the protected rootPath member, which has been renamed to rootPaths (plural), may be affected. IMHO, this is better than leaving it as a singularly named property and changing the type, which would cause other (less easily identified) issues. Do we have a BC policy as it applies to extending our classes and protected properties/methods?

Which brings us to one of the most pressing issues for v2.0 (in my opinion): we need to make everything private that doesn't serve as an API entry point, a move many projects have been making as of late, including Symfony itself. In my opinion, most properties and methods should be either private or public, unless we have an internal need to enable inheritance and set something as protected.

Thoughts?

[robfrawley]

So, @antoligy: we want to merge this?

[alexwilson]

Sorry for the delay, and yes I think we should be good to merge this! Can we can warn about the potential BC breakage in the changelog? (I doubt that this bit of the API is actually used though, and I agree that in 2.0 we should reduce the surface area exposed by the API - As we're looking to decouple business logic from controllers the API definition would be greatly improved anyway!)

[robfrawley]

Agreed (re decoupling and reduction of surface area). Ok; I'll merge this and release 1.7.1 later today, which will include an updated changelog. Are there any other pressing issues I'm not aware of that need to be part of this patch release?

[alexwilson]

Nothing as far as I'm aware

[alexwilson]

🎉

[robfrawley]

Still need to cherry pick for master and 2.0 branches, but 1.7.1 release is now live! ;-)

[robfrawley]

Comment about the BC break for those extending FileSystemLoader is in the UPGRADE.md file in the respective line about the changes to that class. I'll be pushed updated docs shortly which will cause the online documentation to be re-rendered, as well, with information about the ability to configure multiple data roots.