Skip to content

Change ProxyCacheWarmer::warmUp signature #53128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 18, 2023
Merged

Change ProxyCacheWarmer::warmUp signature #53128

merged 1 commit into from
Dec 18, 2023

Conversation

llupa
Copy link
Contributor

@llupa llupa commented Dec 18, 2023 •
edited
Loading

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 carsonbot added this to the 7.0 milestone Dec 18, 2023
@carsonbot
Copy link

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
Copy link
Member

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
Copy link
Contributor Author

llupa commented Dec 18, 2023

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
Copy link
Member

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
Copy link
Member

xabbuh commented Dec 18, 2023

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 llupa changed the base branch from 7.0 to 6.4 December 18, 2023 17:01
@llupa llupa changed the title fix: HttpKernel conflict version with DoctrineBridge [DoctrineBridge][BC BREAK] Change ProxyCacheWarmer::warmUp signature Dec 18, 2023
@llupa llupa marked this pull request as draft December 18, 2023 17:02
@llupa
Copy link
Contributor Author

llupa commented Dec 18, 2023

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

@GromNaN
Copy link
Member

GromNaN commented Dec 18, 2023 •
edited
Loading

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

nicolas-grekas
nicolas-grekas approved these changes Dec 18, 2023
View reviewed changes
@carsonbot carsonbot changed the title [DoctrineBridge][BC BREAK] Change ProxyCacheWarmer::warmUp signature Change ProxyCacheWarmer::warmUp signature Dec 18, 2023
@llupa llupa marked this pull request as ready for review December 18, 2023 19:39
@carsonbot carsonbot modified the milestones: 7.0, 6.4 Dec 18, 2023
@carsonbot
Copy link

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
Copy link
Member

fabpot commented Dec 18, 2023

Thank you @llupa.

@fabpot fabpot merged commit de3b253 into symfony:6.4 Dec 18, 2023
@llupa llupa deleted the http-kernel-conflicts branch December 19, 2023 08:42
This was referenced Dec 30, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
6 participants
@llupa @carsonbot @nicolas-grekas @xabbuh @GromNaN @fabpot