Bump doctrine/doctrine-bundle and sonata-project/datagrid-bundle #986

wbloszyk · 2021-02-07T17:18:38Z

Subject

I am targeting this branch, because it should be done in master.

Changelog

### Changed
- Bump "doctrine/doctrine-bundle" to ^2.0
- Bump "sonata-project/datagrid-bundle" to ^3.0
### Removed
- Removed support for "swiftmailer/swiftmailer-bundle"

VincentLanglet · 2021-02-07T17:22:11Z

.env

@@ -3,3 +3,7 @@ APP_SECRET=13345f5bb5c82aa1cc0033b64cde8989

 MAILER_URL=smtp://mailhog:1025
 DATABASE_URL=mysql://sonata:sonata@mysql:3306/sonata?serverVersion=5.7
+
+###> symfony/mailer ###
+# MAILER_DSN=smtp://localhost


You have both a MAILER_URL and a MAILER_DSN

It was added by recipies. MAILER_URL is in swiftmailer config. We do not need both?

Then you can remove the section, can't you?

I will keep this section becouse we are changing swiftmailer to mailer:

wbloszyk@wbloszyk-Z170-D3H:~/PhpstormProjects/sandbox_3$ composer why symfony/mailer sonata-project/user-bundle 4.10.2 requires symfony/mailer (^4.4 || ^5.1)

wbloszyk@wbloszyk-Z170-D3H:~/PhpstormProjects/sandbox_3$ composer why symfony/swiftmailer-bundle sonata-project/sandbox dev-master requires symfony/swiftmailer-bundle (^3.4) sonata-project/news-bundle 3.16.0 requires symfony/swiftmailer-bundle (^3.4)

Proof: sonata-project/SonataUserBundle@ecc961b

@phansys Can you do the same for SonataNewsBundle?

BTW we should also change MAILER_URL to the MAILER_DNS like it is using in mailer.

@phansys Can you do the same for SonataNewsBundle?

I'll try to find time this week.

@phansys sonata-project/SonataNewsBundle#711

OskarStark · 2021-02-07T17:22:11Z

config/bundles.php

-declare(strict_types=1);
-
-/*
- * This file is part of the Sonata Project package.
- *
- * (c) Thomas Rabaix <thomas.rabaix@sonata-project.org>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-


wbloszyk · 2021-02-07T17:28:12Z

After do it I got some error like:

 Argument 1 passed to Sonata\Component\Product\SeoProductIterator::__construct() must be an instance of Symfony\Bridge\Doctrine\RegistryInterface, instance of Doctrine\Bundle\DoctrineBundle\Registry given, called in /home/zbiru/Phps  
  tormProjects/sandbox_3/var/cache/dev/ContainerN2fiw4U/srcApp_KernelDevDebugContainer.php on line 17529

and

Argument 3 passed to Sonata\ProductBundle\Block\RecentProductsBlockService::__construct() must be an instance of Symfony\Bridge\Doctrine\RegistryInterface, instance of Doctrine\Bundle\DoctrineBundle\Registry given, called in /home/zbiru/PhpstormProjects/sandbox_3/var/cache/dev/ContainerFehTqnf/srcApp_KernelDevDebugContainer.php on line 9953

Both are in ecommerce and are made by inject <argument type="service" id="doctrine"/>. WDYT We should drop support for doctrine/doctrine-bundle v1 and present it as potential BC-break or we should change RegistyInterface to ManagerRegistry? @greg0ire

Also thanks for @OskarStark and @VincentLanglet for very quick review. :)

VincentLanglet · 2021-02-07T17:40:52Z

.php_cs.cache

Shouldn't we add this to .gitignore ?

greg0ire · 2021-02-07T17:51:25Z

Dropping support for doctrine bundle v1 is not a BC break if that is the question.

wbloszyk · 2021-02-07T18:07:51Z

Dropping support for doctrine bundle v1 is not a BC break if that is the question.

I do not ask about it. Anyway all of sonata bundles suport doctrine/doctrine-bundle v2 so It is even nice idea to drop support for 1.x. Are you agree? @greg0ire

VincentLanglet · 2021-02-07T18:32:49Z

config/packages/mailer.yaml

@@ -0,0 +1,3 @@
+framework:
+    mailer:
+        dsn: '%env(MAILER_DSN)%'


You're using MAILER_DSN, but you removed it from the .env in favor of MAILER_URL.

There is something not clear for me.

OskarStark · 2021-02-10T05:59:25Z

Can you finish this one @wbloszyk ?

And please rebase, thanks

SonataCI · 2021-02-10T06:04:59Z

Could you please rebase your PR and fix merge conflicts?

SonataCI · 2021-02-13T16:35:00Z

Could you please rebase your PR and fix merge conflicts?

wbloszyk · 2021-02-13T16:41:43Z

RTM

SonataCI · 2021-02-13T16:44:59Z

Could you please rebase your PR and fix merge conflicts?

SonataCI · 2021-02-16T10:05:09Z

Could you please rebase your PR and fix merge conflicts?

wbloszyk · 2021-02-16T17:57:19Z

@OskarStark Can you check it?

VincentLanglet · 2021-02-16T18:10:21Z

All the test are failing @wbloszyk

wbloszyk · 2021-02-16T18:16:19Z

@VincentLanglet The test will be taking becouse we do not rewrite it to new system. They are from symfony 2.

VincentLanglet · 2021-02-16T18:30:09Z

I don't understand. First things to do would be to fix the tests.
Linter is failing
Cache:clear is returning an error:

You have requested a non-existent parameter "sonata.comment.class.comment.e  
!!    ntity". Did you mean this: "sonata.news.admin.comment.entity"?

I cant make a proper review with a failing ci which throw a lot of errors.
Maybe someone else thinks differently.

wbloszyk · 2021-02-16T19:43:06Z

@VincentLanglet The tests are always failing from time when I update symfony from 2 to 4. Look at tests in other PRs, like:
#1004

You have requested a non-existent parameter "sonata.comment.class.comment.e  
!!    ntity". Did you mean this: "sonata.news.admin.comment.entity"?

This error was introduce in SonataCommentBundle 3.3. I check it with 3.2 and it was disapere.

BTW
Can you also look at: #634

VincentLanglet · 2021-02-16T19:52:49Z

@VincentLanglet The tests are always failing from time when I update symfony from 2 to 4. Look at tests in other PRs, like:
#1004

Yes, so fixing the tests should be the priority. How can we bump dependencies without tests ?
If it break something we cant know it.

You have requested a non-existent parameter "sonata.comment.class.comment.e  
!!    ntity". Did you mean this: "sonata.news.admin.comment.entity"?      
This error was introduce in SonataCommentBundle 3.3. I check it with 3.2 and it was disapere.

This should come from https://github.com/sonata-project/SonataCommentBundle/blob/55d0784bc7325d1a644fa5f2a1cfe39c0fdb3d5f/src/DependencyInjection/SonataCommentExtension.php#L315

It was introduced in sonata-project/SonataCommentBundle#190, maybe @jordisala1991 could help

wbloszyk · 2021-02-16T19:58:31Z

I cant make a proper review with a failing ci which throw a lot of errors.
Maybe someone else thinks differently.

We agree to accept errors in test to allow upgrade to symfony 4 directly from:
https://github.com/sonata-project/sandbox/blob/2.3/composer.json

We are make it sandbox:master working but from this time I do not have time to finish it.

This should come from https://github.com/sonata-project/SonataCommentBundle/blob/55d0784bc7325d1a644fa5f2a1cfe39c0fdb3d5f/src/DependencyInjection/SonataCommentExtension.php#L315

It was introduced in sonata-project/SonataCommentBundle#190, maybe @jordisala1991 could help

It is getter. Anyway I working on it.

jordisala1991 · 2021-02-16T20:01:39Z

https://github.com/sonata-project/SonataCommentBundle/blob/55d0784bc7325d1a644fa5f2a1cfe39c0fdb3d5f/src/DependencyInjection/SonataCommentExtension.php#L131

The parameter should be defined here. Even thought it could be workinh with 3.2 , the logic there didnt change a lot.

jordisala1991 · 2021-02-16T20:03:12Z

Oh, it is used previous its definition. Probably the bug is there. Can you check @wbloszyk ?

wbloszyk · 2021-02-16T20:04:39Z

Oh, it is used previous its definition. Probably the bug is there. Can you check @wbloszyk ?

Correct, now it is working. Do you have time for make PR or should I do it?

jordisala1991 · 2021-02-16T20:06:12Z

Oh, it is used previous its definition. Probably the bug is there. Can you check @wbloszyk ?

Correct, now it is working. Do you have time for make PR or should I do it?

Better if you do it, I can review it.

wbloszyk · 2021-02-18T11:24:24Z

I merge this PR becouse tests are working on my local machine afer some changes in vendor. I describe require changes in #1008

VincentLanglet reviewed Feb 7, 2021

View reviewed changes

OskarStark reviewed Feb 7, 2021

View reviewed changes

VincentLanglet reviewed Feb 7, 2021

View reviewed changes

wbloszyk requested a review from VincentLanglet February 7, 2021 18:05

VincentLanglet reviewed Feb 7, 2021

View reviewed changes

SonataCI added the pending author label Feb 10, 2021

wbloszyk force-pushed the bump_datagrid branch from f61ec00 to 9eb7e2d Compare February 11, 2021 10:31

SonataCI removed the pending author label Feb 11, 2021

SonataCI added pending author and removed pending author labels Feb 13, 2021

wbloszyk requested review from OskarStark, phansys, VincentLanglet and a team February 13, 2021 16:41

SonataCI added the pending author label Feb 13, 2021

wbloszyk force-pushed the bump_datagrid branch from 90d7695 to 4e28416 Compare February 13, 2021 16:46

SonataCI removed the pending author label Feb 13, 2021

SonataCI added the pending author label Feb 16, 2021

wbloszyk added 3 commits February 16, 2021 18:45

Bump doctrine/doctrine-bundle and sonata-project/datagrid-bundle

996c959

Revert header in config/bundles.php

b02606b

Add .php_cs.cache to .gitignore

97b16ab

wbloszyk added 3 commits February 16, 2021 18:45

Drop symfony/swiftmailer-bundle

d2d0b81

Uncomment MAILER_DNS

3ebccf0

Update composer.lock

997dcb8

wbloszyk force-pushed the bump_datagrid branch from 4e28416 to 997dcb8 Compare February 16, 2021 17:52

SonataCI removed the pending author label Feb 16, 2021

Fix cs

1511e25

VincentLanglet removed their request for review February 16, 2021 18:30

wbloszyk added 2 commits February 16, 2021 21:47

Update composer.lock

c67d952

Update composer.lock

3d7f93b

wbloszyk mentioned this pull request Feb 18, 2021

How to make tests green - TODO list #1008

Closed

9 tasks

wbloszyk added this to the 3.0 milestone Feb 18, 2021

wbloszyk merged commit 0d0ea90 into sonata-project:master Feb 18, 2021

wbloszyk deleted the bump_datagrid branch February 18, 2021 11:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump doctrine/doctrine-bundle and sonata-project/datagrid-bundle #986

Bump doctrine/doctrine-bundle and sonata-project/datagrid-bundle #986

wbloszyk commented Feb 7, 2021 •

edited

Loading

VincentLanglet Feb 7, 2021

wbloszyk Feb 7, 2021

OskarStark Feb 8, 2021

wbloszyk Feb 11, 2021

wbloszyk Feb 11, 2021

phansys Feb 11, 2021

wbloszyk Feb 12, 2021

OskarStark Feb 7, 2021

wbloszyk Feb 7, 2021

wbloszyk commented Feb 7, 2021

VincentLanglet Feb 7, 2021

greg0ire commented Feb 7, 2021

wbloszyk commented Feb 7, 2021

VincentLanglet Feb 7, 2021

OskarStark commented Feb 10, 2021

SonataCI commented Feb 10, 2021

SonataCI commented Feb 13, 2021

wbloszyk commented Feb 13, 2021

SonataCI commented Feb 13, 2021

SonataCI commented Feb 16, 2021

wbloszyk commented Feb 16, 2021

VincentLanglet commented Feb 16, 2021

wbloszyk commented Feb 16, 2021

VincentLanglet commented Feb 16, 2021

wbloszyk commented Feb 16, 2021

VincentLanglet commented Feb 16, 2021

wbloszyk commented Feb 16, 2021

jordisala1991 commented Feb 16, 2021

jordisala1991 commented Feb 16, 2021 •

edited

Loading

wbloszyk commented Feb 16, 2021

jordisala1991 commented Feb 16, 2021

wbloszyk commented Feb 18, 2021

Bump doctrine/doctrine-bundle and sonata-project/datagrid-bundle #986

Bump doctrine/doctrine-bundle and sonata-project/datagrid-bundle #986

Conversation

wbloszyk commented Feb 7, 2021 • edited Loading

Subject

Changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbloszyk commented Feb 7, 2021

Choose a reason for hiding this comment

greg0ire commented Feb 7, 2021

wbloszyk commented Feb 7, 2021

Choose a reason for hiding this comment

OskarStark commented Feb 10, 2021

SonataCI commented Feb 10, 2021

SonataCI commented Feb 13, 2021

wbloszyk commented Feb 13, 2021

SonataCI commented Feb 13, 2021

SonataCI commented Feb 16, 2021

wbloszyk commented Feb 16, 2021

VincentLanglet commented Feb 16, 2021

wbloszyk commented Feb 16, 2021

VincentLanglet commented Feb 16, 2021

wbloszyk commented Feb 16, 2021

VincentLanglet commented Feb 16, 2021

wbloszyk commented Feb 16, 2021

jordisala1991 commented Feb 16, 2021

jordisala1991 commented Feb 16, 2021 • edited Loading

wbloszyk commented Feb 16, 2021

jordisala1991 commented Feb 16, 2021

wbloszyk commented Feb 18, 2021

wbloszyk commented Feb 7, 2021 •

edited

Loading

jordisala1991 commented Feb 16, 2021 •

edited

Loading