Timestamp returned for trait last modification times in the cached reader should be an integer, is an array

[trsteel88]

This PR fixes an issue where the cache is constantly being written - wasting IO.

After looking into this further, I found that the problem I'm having with excessive IO is because getLastModification is merging the timestamps incorrectly.

The result of the following can't be merged:

dump(array_merge(
    [$filename ? filemtime($filename) : 0],
    array_map([$this, 'getTraitLastModificationTimes'], $class->getTraits()),
    array_map([$this, 'getLastModification'], $class->getInterfaces()),
    $parent ? [$this->getLastModification($parent)] : []
));

The result of this is that it can't work out the max() of the traits.

To resolve this, I updated getTraitLastModificationTimes to always return the max timestamp.

Using this PR in favour of another change I made: #104

[mikeSimonson]

@trsteel88 Thanks.
Can you add a test case for this ?

[stof]

the name should be changed, as it does not return several times anymore

[Ocramius]

This method returns an int[], which is then passed to max() in private function getLastModification(). If I get it correctly, the fix is different, and it is to change that function's body instead:

         return max(array_merge(
             [$filename ? filemtime($filename) : 0],
            // notice the three dots:
             ...array_map([$this, 'getTraitLastModificationTimes'], $class->getTraits()),
             array_map([$this, 'getLastModification'], $class->getInterfaces()),
             $parent ? [$this->getLastModification($parent)] : []
         ));

[trsteel88]

The three dots don't work.

Compile Error: Cannot use positional argument after argument unpacking

[Ocramius]

A test case demonstrating the current issue is also needed.

[Ocramius]

This method returns an int[], which is then passed to max() in private function getLastModification(). If I get it correctly, the fix is different, and it is to change that function's body instead:

         return max(array_merge(
             [$filename ? filemtime($filename) : 0],
            // notice the three dots:
             ...array_map([$this, 'getTraitLastModificationTimes'], $class->getTraits()),
             array_map([$this, 'getLastModification'], $class->getInterfaces()),
             $parent ? [$this->getLastModification($parent)] : []
         ));

[trsteel88]

@Ocramius @mikeSimonson where should this test go? I can't see any existing tests for the CachedReacher class.

I'm assuming we just need a single test to ensure that getLastModification returns an int (and never an array)?

[mikeSimonson]

@trsteel88 The test should go there with the other tests of cachedReader.

[trsteel88]

Thanks @mikeSimonson - I'll add one in.

Also, I noticed that within fetchFromCache() it has the following:

     if (($data = $this->cache->fetch($cacheKey)) !== false) {
            if (!$this->debug || $this->isCacheFresh($cacheKey, $class)) {
                return $data;
            }
        }

In dev, debug is always going to be false. This is rendering the cache completely useless. Is this meant to be there?

[Ocramius]

In dev mode, caches should really not be hit at all, so that's intentional.

[Ocramius]

@trsteel88 merged, I added a test to it :-)

master: 8ac9494
1.3.x: bc1963d

[chosroes]

This caused the error in #117 =/

[Ocramius]

@chosroes I still need a test case for that - can't do anything without it, and I'm not bootstrapping a symfony application for that.