fix doWarmUp $buildDir deprecation

[dmaicher]

Remaining self deprecation notices (1)

  1x: The "Doctrine\Bundle\DoctrineBundle\CacheWarmer\DoctrineMetadataCacheWarmer::doWarmUp()" method will require a new "string|null $buildDir" argument in the next major version of its parent class "Symfony\Bundle\FrameworkBundle\CacheWarmer\AbstractPhpFileCacheWarmer", not defining it is deprecated.
    1x in CacheCompatibilityPassTest::testCacheConfigUsingServiceDefinedByApplication from Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection\Compiler

Also see https://github.com/doctrine/DoctrineBundle/actions/runs/6600459221/job/17930490923#step:7:44

[derrabus]

Note that this change is a breaking one if we don't consider DoctrineMetadataCacheWarmer as final. Not sure if targeting a bugfix release for that was the best move.

[dmaicher]

Note that this change is a breaking one if we don't consider DoctrineMetadataCacheWarmer as final. Not sure if targeting a bugfix release for that was the best move.

Yeah I don't see DoctrineMetadataCacheWarmer as an extension point really but technically its a BC break.

How about adding @final annotation and moving the change to 2.11.x?

[ostrolucky]

I don't see how is this a BC breaking change, care to clarify?

[dmaicher]

I don't see how is this a BC breaking change, care to clarify?

Well if someone extended the class like this

class MyCustomCacheWarmer extends DoctrineMetadataCacheWarmer
{
    protected function doWarmUp($cacheDir, ArrayAdapter $arrayAdapter): bool
    {
        return parent::doWarmUp($cacheDir, $arrayAdapter);
    }
}

Then it would fail with my change

PHP Fatal error:  Declaration of MyCustomCacheWarmer::doWarmUp($cacheDir, Symfony\Component\Cache\Adapter\ArrayAdapter $arrayAdapter): bool must be compatible with Doctrine\Bundle\DoctrineBundle\CacheWarmer\DoctrineMetadataCacheWarmer::doWarmUp(string $cacheDir, Symfony\Component\Cache\Adapter\ArrayAdapter $arrayAdapter, ?string $buildDir = null): bool in /var/www/DoctrineBundle/CacheWarmer/MyCustomCacheWarmer.php on line 9

[ostrolucky]

That's because $buildDir has not been specified, not related to this PR. This would work https://3v4l.org/2fLp1

[dmaicher]

That's because $buildDir has not been specified, not related to this PR. This would work https://3v4l.org/2fLp1

hm... but my PR added the $buildDir argument? 😄 so its related to this PR?

[ostrolucky]

Man, somehow I missed that... Proceed according your preference. In my opinion it doesn't matter much if we do it in 2.10.x or 2.11.x, only major is allowed by semver, so we break the rule anyways, but it needs to ship.

[derrabus]

Yeah I don't see DoctrineMetadataCacheWarmer as an extension point really but technically its a BC break.

Neither do I. Classes like this one should be considered final, but we haven't documented it as such.

How about adding @final annotation and moving the change to 2.11.x?

Yes, let's do that.