Skip to content
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.

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

Update to Symfony 6.2 #1362

Merged
merged 1 commit into from
Dec 1, 2022
Merged

Update to Symfony 6.2 #1362

merged 1 commit into from
Dec 1, 2022

Conversation

javiereguiluz
Copy link
Member

Comments:

  • I've removed the SensioFrameworkExtraBundle because Symfony 6.2 now includes everything we need
  • The dev dependency of DoctrineBundle is needed because we need the unreleased 2.7.1 to have the equivalent of the old ParamConverter
  • The following indirect deprecations remain:
Remaining indirect deprecation notices (3)

1x: The "Symfony\Bridge\Doctrine\Logger\DbalLogger" class implements
"Doctrine\DBAL\Logging\SQLLogger" that is deprecated
Use {@see \Doctrine\DBAL\Logging\Middleware}
or implement {@see \Doctrine\DBAL\Driver\Middleware} instead.
  1x in AddUserCommandTest::testCreateUserNonInteractive from App\Tests\Command

1x: The "Monolog\Logger" class is considered final. It may change without
further notice as of its next major version. You should not extend it
from "Symfony\Bridge\Monolog\Logger".
  1x in AddUserCommandTest::testCreateUserNonInteractive from App\Tests\Command

1x: Since symfony/translation 6.2: "Symfony\Component\Translation\Extractor\PhpExtractor"
is deprecated, use "Symfony\Component\Translation\Extractor\PhpAstExtractor" instead.
  1x in AddUserCommandTest::testCreateUserNonInteractive from App\Tests\Command

@stof
Copy link
Member

stof commented Oct 28, 2022

For the first one, we need to debug why the DbalLogger still gets autoloaded while DoctrineBundle properly supports using middlewares when using DBAL 3.2+ and Symfony versions containing the middleware for the profiler integration.

for the Symfony\Bridge\Monolog\Logger, this should be reported to Symfony to find a way to stop extending that class (or to Monolog to revert that decision if we cannot find another way).

For the extractor, either a config setting needs to be added to switch to the new extractor, or something needs to be fixed in Symfony to avoid triggering the deprecation in all projects.

Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

As the DataTransformerInterface is now generic in Symfony 6.2+, you should add the @template-implements DataTransformerInterface<Tag[], string> on the TagArrayToStringTransformer to make phpstan happy about it (as it wants to know the type for that class)

config/packages/doctrine.yaml Outdated Show resolved Hide resolved
@seb-jean
Copy link
Contributor

Instead of using $this->getUser(), we could use #[CurrentUser]:
https://symfony.com/blog/new-in-symfony-5-2-controller-argument-attributes

@javiereguiluz
Copy link
Member Author

I've just updated it until Symfony 6.2 RC1 and also updated all recipes. We still have these indirect deprecations:

Remaining indirect deprecation notices (3)

  1x: The "Symfony\Bridge\Doctrine\Logger\DbalLogger" class implements
      "Doctrine\DBAL\Logging\SQLLogger" that is deprecated
      Use {@see \Doctrine\DBAL\Logging\Middleware}
      or implement {@see \Doctrine\DBAL\Driver\Middleware} instead.

  1x: The "Monolog\Logger" class is considered final. It may change without
      further notice as of its next major version. You should not extend
      it from "Symfony\Bridge\Monolog\Logger".

  1x: Since symfony/translation 6.2: "Symfony\Component\Translation\Extractor\PhpExtractor"
      is deprecated, use "Symfony\Component\Translation\Extractor\PhpAstExtractor" instead.

@seb-jean
Copy link
Contributor

seb-jean commented Dec 1, 2022

@javiereguiluz, it is good practice to put each controller argument on its own line?

@javiereguiluz
Copy link
Member Author

@seb-jean the traditional Symfony practice was to put all arguments on the same line, even if it's too long.

However, now that we're applying PHP attributes to some arguments, we think it's better in that case to put each argument on its own line, to not miss those PHP attributes.

@seb-jean
Copy link
Contributor

seb-jean commented Dec 1, 2022

We no longer need to call createView()
https://symfony.com/blog/new-in-symfony-6-2-dx-improvements#simpler-form-rendering

@seb-jean
Copy link
Contributor

seb-jean commented Dec 1, 2022

Should we use the hash_property_path option for https://github.com/symfony/demo/blob/main/src/Form/Type/ChangePasswordType.php?

// https://github.com/symfony/demo/blob/main/src/Form/Type/ChangePasswordType.php
->add('newPassword', RepeatedType::class, [
   'type' => PasswordType::class,
   'constraints' => [
       new NotBlank(),
       new Length(
           min: 5,
           max: 128,
       ),
   ],
   'first_options' => [
       'label' => 'label.new_password',
       'hash_property_path' => 'password'

   ],
   'second_options' => [
       'label' => 'label.new_password_confirm'
   ],
])

And remove setPassword(): https://github.com/symfony/demo/blob/main/src/Controller/UserController.php#L63

@stof
Copy link
Member

stof commented Dec 1, 2022

For the remaining deprecations, here is the status:

  1. work is in progress in DoctrineBundle to get rid of it (we probably need to mark the SQL logger service as deprecated, unless we mis-identified the root cause)
  2. this is discussed in MonologBridge shows deprecation error with Monolog 3.2 symfony#47096 but it requires adding a new feature in FrameworkBundle to change the way the LoggerDataCollector is wired which might require waiting for Symfony 6.3 to solve the Monolog 3.2 deprecation (or doing weird things in a compiler pass in MonologBundle)
  3. Does this deprecation go away if you install nikic/php-parser to enable the new AST-based extractor ? If yes, we might need to find a solution to avoid reporting that deprecation in a different way for projects that don't want to use the translation extractor.

.github/workflows/tests.yaml Outdated Show resolved Hide resolved
@javiereguiluz
Copy link
Member Author

@stof thanks for the update about the deprecations! After the latest updates, the deprecation related to translation is gone. I can only see these:

  1x: The "Symfony\Bridge\Doctrine\Logger\DbalLogger" class implements "Doctrine\DBAL\Logging\SQLLogger" that is deprecated Use {@see \Doctrine\DBAL\Logging\Middleware} or implement {@see \Doctrine\DBAL\Driver\Middleware} instead.
    1x in AddUserCommandTest::testCreateUserNonInteractive from App\Tests\Command

  1x: The "Monolog\Logger" class is considered final. It may change without further notice as of its next major version. You should not extend it from "Symfony\Bridge\Monolog\Logger".
    1x in AddUserCommandTest::testCreateUserNonInteractive from App\Tests\Command

@javiereguiluz
Copy link
Member Author

@seb-jean I've merged this PR without doing other changes because it was taking too long already ... but I'd love to review a PR that updates the code to use some of the nice features introduced in Symfony 6.2, like the ones you mentioned here. Thanks!

@javiereguiluz javiereguiluz mentioned this pull request Dec 1, 2022
@lxregistry
Copy link

there are also other deprecation:
User Deprecated: Method "Countable::count()" might add "int" as a native return type declaration in the future. Do the same in implementation "Doctrine\Common\Collections\AbstractLazyCollection" now to avoid errors or add an explicit @return annotation to suppress this message.

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.

5 participants