Maybe this breaks BC, tell me if I need to reopen this PR against develop
Override 'ServiceManager::has' to do not use peering service managers
I wonder why Travis build failed 😟
@Danizord Transient cache timing issue -- your tests passed.
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.
$this->setExpectedException(/* ... */)
So, my conclusions are:
Let's figure out which it is. :)
Move ExpectedException from annotation to ->setExpectedException()
@weierophinney You're (or I'm) doing something wrong, because I tried the test without the code change and it failed:
Figured it out -- I'd missed adding the addPeeringServiceManager() call; with that in place, I'm indeed getting a failure.
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?
@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.
Merge branch 'hotfix/4250' into develop
Forward port #4250
Merge branch 'hotfix/4250'