[Loader] [Locator] FileSystemLocator service must not be shared

[robfrawley]

Q	A
Branch?	1.0
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#874
License	MIT

This PR adds a compiler pass that gathers the Locator services and makes them not shared, causing the Symfony container to instantiate new objects each time they are requested (instead of the default behavior of re-using the first instance created).

This solves a bug introduced in 1.7.2 that causes those who use multiple FileSystemLoader definitions to only have their last path registered. Functional tests have been added to ensure a similar regression does not occur in the future.

Additionally, when the *Locator classes were implemented, exceptions originating from Symfony's OptionsResolver were ignored; this PR adds a try/catch block to the FileSystemLocator::setOptions() method and throws an exception originating from the Liip namespace so users don't have to worry about catching external dependencies' exceptions.

---

Note: This PR should not be merged into the 2.0 branch. The special handling seen in this PR is only required because the 1.0 branch supports both Symfony <2.8 and >=2.8. These versions require different mechanisms for achieving non-shared services (with <2.8 requiring "prototype" scoping and >=2.8 allowing the service to be simply defined as "not shared"). The 2.0 branch can achieve the same intent of this PR by simpy adding shared="false" to the locator service definitions.

<service shared="false" id="liip_imagine.binary.locator.filesystem" class="%liip_imagine.binary.locator.filesystem.class%" public="false"></service>
<service shared="false" id="liip_imagine.binary.locator.filesystem_insecure" class="%liip_imagine.binary.locator.filesystem_insecure.class%" public="false"></service>

[sebastianblum]

The third argument (strict) was removed in symfony 3.0.
Should we use an switch like

if (symfony 2.x) {
    return new Reference(
        sprintf('liip_imagine.binary.locator.%s', $reference),
            ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE,
            false
    );

    return new Reference(sprintf('liip_imagine.binary.locator.%s', $config['locator']);
}

[robfrawley]

The reason I left it like this is because that will always be true for Symfony >=2.8, and therefore it doesn't matter that it's ignored. We only care about it when it is false on Symfony <2.8. Otherwise, the extra parameter can be, and is, safely ignored (the PHP interpreter allows and ignores any extra parameters). Do you really think it's necessary to add a switch?

[sebastianblum]

Good morning @robfrawley, not an easy question. You are right, the code is fine.
I would prefer a feature switch at the moment and remove it in future (maybe 2.0) when at least symfony 2.8 will be supported.

[robfrawley]

Ok; ammended the code to the following:

$name = sprintf('liip_imagine.binary.locator.%s', $reference);

if (SymfonyFramework::hasDefinitionSharedToggle()) {
    return new Reference($name);
}

return new Reference($name, ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, false);

I didn't use a version toggle, as I want to use the exact mechanism that requires a non-strict reference, which can be found here. Also, I want to avoid a direct dependency on the Symfony kernel in-code for now.

[noniagriconomie]

Hi @robfrawley thank you for this PR that actually fix our problem too

Do you think it is going to be merged soon?

Many thanks :)

[alexwilson]

Completely fine with this PR, and loving the feature toggling. Will make it much easier to reduce breakage of BC!

[robfrawley]

@noniagriconomie It's excellent to hear that this PR resolves any issues you were encountering with this bundle! With regard to the rollout of this bugfix, I'll merge this PR into master by late tomorrow (UTC-5). I want to give the code a final review and perform some additional cleanup before I merge it. Afterward, you may immediately require the mainline release by using the 1.0.x-dev version string in your project's composer.json file.

As for the next stable release, I am shooting for this coming weekend. There a two additional PRs (which I will be posting shortly) that I would like to include in the upcoming release; those are the main reason for the short delay. At the latest, by Monday 2017-02-27 you will be able to require a stable version constraint and benefit from this fix via the 1.7.3 release.

@antoligy Thanks for allotting the time to provide feedback; it always feels wrong to merge PRs without some form of peer-review. :-) As always, much appreciated.

[noniagriconomie]

fine, waiting the publication of v1.7.3 then :)