Change ProxyCacheWarmer::warmUp signature

[llupa]

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	none exists
License	MIT

I ended up with a composer.lock that has "symfony/doctrine-bridge:v6.4.0" and "symfony/http-kernel:v7.0.1", this combination results in a fatal syntax error:

Compile Error:
 Declaration of Symfony\Bridge\Doctrine\CacheWarmer\ProxyCacheWarmer::warmUp(string $cacheDir): array 
 must be compatible with 
 Symfony\Component\HttpKernel\CacheWarmer\WarmableInterface::warmUp(string $cacheDir, ?string $buildDir = null): array

I opened this PR as a starting point to "fix" this, I already wondered if the conflict should also be put over doctrine-bridge. Happy to do changes as needed.

[carsonbot]

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

[nicolas-grekas]

This won't fix the issue since the faulty combination is still allowed, so that composer can still select it, isn't it?

What about making the bridge compatible with 7.0 instead? This requires a BC break, but it shouldn't affect anybody IMHO: that class/method isn't extended by anyone 🤞
WDYT @symfony/mergers ?

[llupa]

I thought about updating Symfony\Bridge\Doctrine\CacheWarmer\ProxyCacheWarmer::warmUp too and this would be best from my POV, but wouldn't that mean you will have anyway a conflict between doctrine-bridge 6.4.0 and htt-kernel 7.x.x ? The fix would go to doctrine-bridge 6.4.1 as I understand it.

[nicolas-grekas]

have anyway a conflict between doctrine-bridge 6.4.0 and htt-kernel 7.x.x ?

no need to since .0 will be replaced by .1

[xabbuh]

What about making the bridge compatible with 7.0 instead? This requires a BC break, but it shouldn't affect anybody IMHO: that class/method isn't extended by anyone

works for me, we already did that in the Serializer and Translation components in #50391

[llupa]

I am not sure if I completely broke the process, but, I tried my best with what Nicolas and Christian said 😅

[GromNaN]

it shouldn't affect anybody IMHO: that class/method isn't extended by anyone 🤞 WDYT

Agreed, we should make this classes final in 7.1

[carsonbot]

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

[fabpot]

Thank you @llupa.