Skip to content

Conversation

@webignition
Copy link

@webignition webignition commented Dec 1, 2025

Method signatures in MigrationsCollector require updating to be compatible with base classes and interfaces in Symfony 8.

  • add void return type to MigrationsCollector::collect()
  • add void return type to MigrationsCollector::reset()
  • add string return type to MigrationsCollector::getName()

The string and void return types were added in PHP 7.0 and 7.1, respectively. As the latest version of this package supports a minimum PHP version of 7.2 I see no backwards compatibility issues or breaking changes.

I'm looking to merge this into 3.7.x purely to allow for the soonest possible release. Please adjust if this is not what I should be doing.

Update `MigrationsCollector::reset()` to `MigrationsCollector::reset():void`.

For compatibiity with `Symfony\Component\HttpKernel\DataCollector\reset()` 8.x,
@webignition webignition changed the title Add void return type to MigrationsCollector::reset() Add require return types to MigrationsCollector for Symfony 8 compatibility Dec 2, 2025
@webignition webignition changed the title Add require return types to MigrationsCollector for Symfony 8 compatibility Add required return types to MigrationsCollector for Symfony 8 compatibility Dec 2, 2025
@greg0ire greg0ire requested a review from stof December 2, 2025 20:24
@greg0ire
Copy link
Member

greg0ire commented Dec 2, 2025

I'm requiring @stof's review because he was not OK with this kind of thing for the bundle. Maybe the right thing here would be to release 4.0.0, but maybe things are different here since we (wrongly) published 3.x versions of this bundle that allow Symfony 8.

I'll attempt to address that last point in #644

@stof, with the above in mind, are you OK with this PR? If not, would you be OK with a PR that would use a trait to add the return types iff symfony/http-kernel 8 is in use? If not, I guess we'll just have to release 4.0.0.

use function count;
use function get_class;

/** @final */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class is annotated @final, but that annotation was added in 3.5.0 (see 25b672a)

@webignition
Copy link
Author

@greg0ire regarding your suggestion of possibly using a trait to add the return types iff symfony/http-kernel >= 8, I don't think such conditional application of the return types is strictly necessary.

In projects of mine being upgraded to symfony 8, I've been preemptively applying a patch to add the return types. This is for projects already on symfony 7.4. Adding return types where none are present in an interface seems to be fine from a syntactic correctness perspective.

@greg0ire
Copy link
Member

greg0ire commented Dec 3, 2025

Adding return types where none are present in an interface seems to be fine from a syntactic correctness perspective.

It is a breaking change for any class extending the class where you add types (but then again the class is annotated with @final).

@webignition
Copy link
Author

Gotcha, thanks for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants