Skip to content

Conversation

acrobat
Copy link
Contributor

@acrobat acrobat commented Dec 16, 2021

Q A
Branch? "master" for new features / the branch of the current release for fixes
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Fixed tickets
License MIT
Doc PR

This PR adds some required typehints for symfony 6 and to make this change easier I suggest to release this PR as a new major release. It's easy for project to upgrade/support the upcoming major, as only a previous deprecation is removed and all older symfony versions are still supported (+ sf6 support)

@acrobat acrobat force-pushed the allow-sf6 branch 11 times, most recently from 8046396 to 1820167 Compare December 19, 2021 12:27
@acrobat
Copy link
Contributor Author

acrobat commented Dec 19, 2021

All tests pass again, the PR is ready for review!

Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks for that work!

you are removing the deprecation about passing objects - i think if we don't have string parameter type declarations, we should check with is_string to have clean error reporting rather than end up in unexpected behaviour.

public function generate($name, $parameters = [], $absolute = UrlGeneratorInterface::ABSOLUTE_PATH): string
{
if (is_object($name)) {
@trigger_error('Passing an object as route name is deprecated since version 2.3. Pass the `RouteObjectInterface::OBJECT_BASED_ROUTE_NAME` as route name and the object in the parameters with key `RouteObjectInterface::ROUTE_OBJECT`.', E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

should we check $name with is_string and error if it is not a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I've applied the check!

@acrobat
Copy link
Contributor Author

acrobat commented Dec 20, 2021

I've added the necessary type checks and tests for this case

Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks for the work!

@dbu dbu merged commit 864d560 into symfony-cmf:master Dec 21, 2021
@dbu
Copy link
Member

dbu commented Dec 21, 2021

i adjusted the branch alias in composer.json.

can you add a build for php 8.1 as well? that might lead to further changes because of deprecations.

@acrobat acrobat deleted the allow-sf6 branch December 21, 2021 08:29
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