[#4249] Override 'ServiceManager::has' to do not use peering service managers

[danizord]

Maybe this breaks BC, tell me if I need to reopen this PR against develop

[danizord]

I wonder why Travis build failed 😟

[weierophinney]

@danizord Transient cache timing issue -- your tests passed.

[weierophinney]

I tried the test on master, without the code change, and it passed.

I then altered the test case to remove the @expectedException annotation, and instead put a $this->setExpectedException(/* ... */) call between the has() and get() calls - and the test continued to pass.

So, my conclusions are:

• The code change is unnecessary OR
• The test is not testing what you're intending.

Let's figure out which it is. :)

[danizord]

@weierophinney You're (or I'm) doing something wrong, because I tried the test without the code change and it failed:

[weierophinney]

Figured it out -- I'd missed adding the addPeeringServiceManager() call; with that in place, I'm indeed getting a failure.

[danizord]

Yes! addPeeringServiceManager is necessary to the test, since ControllerLoaderFactory::createService is calling this.

In hindsight, another solution would be to remove the call addPeeringServiceManager in ControllerLoaderFactory.php line 39

What do you think?

[weierophinney]

@danizord The peering manager functionality was originally built with controllers in mind, but then we discovered the potential security implementations of using it (basically, if you specify a :controller segment in your route, if we allow pulling from the peering manager, you could pull a service that has side effects).

I think removing the call to addPeeringServiceManager is the more correct way, but would be a potential BC break; as such, your fix makes sense, as it makes the behavior between get() and has() match.