Fix for #83: misconfigured aliases can lead to infinite loops

[Ocramius]

Fixes #83

[weierophinney]

Nice, clean solution. Would be nice if we hit a cyclic structure, but just being told it cannot resolve is reasonable, I think. 👍

[stof]

you could detect it: when exiting the loop, check whether the final name appears in $this->aliases. If yes, it means there was a cycle for this alias.

[Ocramius]

I provided a much more comprehensive exception, which is thrown at ServiceManager::__construct() and has some decent error messages (debugging cycles is no fun!).

Hope that clears up all this :-)

[stof]

Should you really detect all cycles in the alias map when building the exception, or only the cycle being found at this point ? Your current logic means that each cycle is duplicated many times (the number of times is equal to the length of the cycle), making the user think there is 4 cycles in the map while there is actually only 1 (involving 4 items)

[Ocramius]

Your current logic means that each cycle is duplicated many times (the number of times is equal to the length of the cycle), making the user think there is 4 cycles in the map while there is actually only 1 (involving 4 items)

Agreed, but I wanted to show all possible entry points. De-duplicating those is another piece of work.

In general, my though-flow is following:

• quick servicemanager runtime
• slow exception building (performance doesn't matter here), just show everything and provide possibly meaningful information

[stof]

@Ocramius you could display only the cycle involving the service detected by the runtime IMO. Then, people would be able to fix cycles one after the other. Displaying the same cycle many times (letting the user determine that it is actually the same as the display is different and may not even be grouped together) makes debugging harder IMO.
An alternative would be to display each cycle only once, but removing cycle members from the list of remaining entry points when finding a cycle (so that the cycle is not found again starting at a different place in the cycle)

[Ocramius]

@stof I simply de-duplicated the cycles (see latest commits). Less confusing now, IMO :-)

[kynx]

I'd love to see the use case for aliasing deeper then 100 :)

[kynx]

How's about:

    /**
     * Resolve all aliases to their canonical service names.
     */
    private function resolveAliases(array $aliases)
    {
        foreach ($aliases as $alias => $service) {
            $name = $alias;

            $count = 0;
            while (isset($this->aliases[$name]) && $count < 100) {
                $name = $this->aliases[$name];
                $count++;
            }

            if ($count > 99) {
                throw new ExceptionalIdiotlException("This is nuts. Find another library.")
            }

            $this->resolvedAliases[$alias] = $name;
        }
    }

[Ocramius]

@kynx correct solutions over approximate ones, IMO

[kynx]

@Ocramius yeah, but please, please can't we squeak the ExceptionalIdiotExecption into this one? The world needs it ;)

[Ocramius]

Would love to, but it would violate some sort of yet to be invented CoC ;-)

[asgrim]

LGTM 👍

[weierophinney]

Really nice work, @Ocramius !