Add more CRUD form tests #1552

jordisala1991 · 2022-08-11T20:07:43Z

Subject

There are a ton of errors to fix :)

jordisala1991 · 2022-08-11T20:13:07Z

src/Block/SharedBlockBlockService.php

+        return $this->sharedBlockAdmin->getFormContractor()->getFormBuilder(
+            $this->sharedBlockAdmin->getUniqId(),
+            ['data_class' => $this->sharedBlockAdmin->getClass()]
+        )->create('blockId', ModelListType::class, [


This kind of fix should be applied to other bundles. There is still a lot of code to write to do such fields.

There were 2 problems:

Field description didn't had association admin, and it break on a template from sonataAdmin, fixed line 157.

getFormBuilder does a lot of things, it instantiate al the form for that admin, and it needs a subject, so it breaks. I picked the first lines where it creates de form builder. There are other options to fix this too. One of them is use the original $form from above, but we are wrapping it on a PageFormMapper (For no reason).

IMO we might refactor this further, and remove PageFormMapper too, it is not needed.

What's the issue ? And in which bundle should we apply the same fix ?

It was never reported before

I would say that PageFormMapper was maybe needed prior to sonata-project/SonataBlockBundle@cd2f8f3. But now the AdminFormMapper implements the BlockBundle FormMapper it's not needed.

Does removing this class (in this PR) would simplify your bugfix ?

The problem is:

Some of our bundles define blocks (and they do require block bundle) 👍

Some of that blocks are EditableBlocks (an interface defined on block bundle) 👍

That interface is only testable HERE, because those editableBlocks are for SonataPageBundle 👎

When we move for example SonataMediaBundle from 3.x to 4.x we migrated those blocks to newer SonataBlock version, but we didn't test those because of point 3. (and we didn't try to unit test AFAIK, will need to check) 👎

Now that we are migrating this bundle, some errors appear.

Those are other examples of the same problem:
https://github.com/sonata-project/SonataMediaBundle/blob/4.x/src/Block/MediaBlockService.php#L210-L226

I don't know if there are a lot of them (I think not), but this kind of dependencies are hard to test.

Take a look at my last commit, not using PageFormMapper, allows to \assert for AdminFormMapper and we have getFormBuilder there.

jordisala1991 · 2022-08-11T20:13:45Z

It should work with second commit, but the solution I don't like it a lot, AND I need to add more test because other kind of blocks are broken too.

Edit: nice it works on my computer, but not on github actions, for some reason, will need to take a deeper look.

VincentLanglet · 2022-08-11T22:09:33Z

Edit: nice it works on my computer, but not on github actions, for some reason, will need to take a deeper look.

You have a great computer :p

jordisala1991 · 2022-08-12T07:17:41Z

tests/Functional/Admin/SharedBlockAdminTest.php

+    public static function provideFormUrlsCases(): iterable
+    {
+        // Blocks From SonataBlockBundle
+        // yield 'Create Shared Block - Container' => ['/admin/tests/app/sonatapageblock/shared/create', [


This one is broken on SonataBlockBundle.

Can it be fix ?

Yes, we need to remove this field: https://github.com/sonata-project/SonataBlockBundle/blob/97cc8c77d4c87203e2cfd052c0853ef932239955/src/Block/Service/ContainerBlockService.php#L37

And this PR: sonata-project/SonataBlockBundle#1098

jordisala1991 · 2022-08-12T07:17:54Z

tests/Functional/Admin/SharedBlockAdminTest.php

+            'shared_block[settings][class]' => 'custom_class',
+        ]];
+
+        // yield 'Create Shared Block - Breadcrumb' => ['/admin/tests/app/sonatapageblock/shared/create', [


This one is broken on SonataSeoBundle.

Can it be fix ?

This one is a bit problematic... Take a look at the class:

2.x: https://github.com/sonata-project/SonataSeoBundle/blob/2.x/src/Block/Breadcrumb/BaseBreadcrumbMenuBlockService.php#L29

Here the class extends MenuBlockService which implements EditableBlockService 👍

3.x: https://github.com/sonata-project/SonataSeoBundle/blob/3.x/src/Block/Breadcrumb/BaseBreadcrumbMenuBlockService.php#L29

Here the class does not extend MenuBlockService, and does not implement EditableBlockService 👎

To fix this we will need to BC break, but it should be accepted IMO, because that block is completely unusable.

Maybe @eerison or other users of this bundle could help us with it. IMHO we should not block this PR for it, because it will need some thinking, we can create an issue or something.

To fix this we will need to BC break, but it should be accepted IMO, because that block is completely unusable.

Yeah, I agree

we can create an issue or something.

Can you create an issue with the list of things to fix then ? Then we'll merge this one.

Maybe @eerison or other users of this bundle could help us with it. IMHO we should not block this PR for it, because it will need some thinking, we can create an issue or something.

Create an issue to check it's fine!

But this breadcrumb menu is a bit trick! I'm still struggling with this in #1497 😢

One for this: sonata-project/SonataSeoBundle#685

VincentLanglet · 2022-08-12T07:19:50Z

tests/Functional/Admin/SharedBlockAdminTest.php

+            'shared_block[settings][translation_domain]' => 'SonataPageBundle',
+            'shared_block[settings][icon]' => 'fa fa-home',
+            'shared_block[settings][current]' => 1,
+            // 'shared_block[settings][pageId]' => 1,


Should we remove this line or uncomment it ?

There is a pageId on the settings, but not sure how to make it populate with pages, because it needs a page on the block. I leave it commented to not forget to think about it...

VincentLanglet · 2022-08-12T07:20:00Z

tests/Functional/Admin/SharedBlockAdminTest.php

+    public static function provideFormUrlsCases(): iterable
+    {
+        // Blocks From SonataBlockBundle
+        // yield 'Create Shared Block - Container' => ['/admin/tests/app/sonatapageblock/shared/create', [


Can it be fix ?

VincentLanglet · 2022-08-12T07:20:05Z

tests/Functional/Admin/SharedBlockAdminTest.php

+            'shared_block[settings][class]' => 'custom_class',
+        ]];
+
+        // yield 'Create Shared Block - Breadcrumb' => ['/admin/tests/app/sonatapageblock/shared/create', [


Can it be fix ?

VincentLanglet · 2022-08-12T07:21:07Z

src/Block/SharedBlockBlockService.php

+        return $this->sharedBlockAdmin->getFormContractor()->getFormBuilder(
+            $this->sharedBlockAdmin->getUniqId(),
+            ['data_class' => $this->sharedBlockAdmin->getClass()]
+        )->create('blockId', ModelListType::class, [


What's the issue ? And in which bundle should we apply the same fix ?

It was never reported before

I would say that PageFormMapper was maybe needed prior to sonata-project/SonataBlockBundle@cd2f8f3. But now the AdminFormMapper implements the BlockBundle FormMapper it's not needed.

Does removing this class (in this PR) would simplify your bugfix ?

VincentLanglet · 2022-08-12T07:51:26Z

src/Block/SharedBlockBlockService.php

-            $this->sharedBlockAdmin->getUniqId(),
-            ['data_class' => $this->sharedBlockAdmin->getClass()]
-        )->create('blockId', ModelListType::class, [
+        \assert($form instanceof AdminFormMapper);


I don't like the assert here.

Nothing is preventing

configureEditForm

to be called with another FormMapper.

I would change this too

private function getBlockBuilder(AdminFormMapper $form): FormBuilderInterface

and

public function configureEditForm(FormMapper $form, BlockInterface $block): void { if (!$form instanceof AdminFormMapper) { throw new \InvalidArgumentException(); }

(And we can consider later to make the EditableBlockService generics)

What do you think now?

jordisala1991 added 2 commits August 11, 2022 20:22

Add more CRUD form tests

8264f9d

Fix tests

d40cf30

jordisala1991 added the major label Aug 11, 2022

jordisala1991 force-pushed the hotfix/more-crud-form-tests branch from cf40f54 to 8264f9d Compare August 11, 2022 20:09

jordisala1991 commented Aug 11, 2022

View reviewed changes

Update composer.json

9a48a2f

jordisala1991 added 2 commits August 12, 2022 09:12

More block tests

fcc9daa

Remove old code

28f87f0

jordisala1991 commented Aug 12, 2022

View reviewed changes

jordisala1991 marked this pull request as ready for review August 12, 2022 07:18

jordisala1991 requested a review from VincentLanglet August 12, 2022 07:18

Remove PageFormMapper

eb43569

jordisala1991 force-pushed the hotfix/more-crud-form-tests branch from f2e516e to eb43569 Compare August 12, 2022 07:26

VincentLanglet reviewed Aug 12, 2022

View reviewed changes

Simplify logic

d922ffe

jordisala1991 mentioned this pull request Aug 12, 2022

BaseBreadcrumbMenuBlockService changed from 2.x to 3.x sonata-project/SonataSeoBundle#685

Closed

VincentLanglet requested changes Aug 12, 2022

View reviewed changes

jordisala1991 mentioned this pull request Aug 12, 2022

Remove editable from ContainerBlockService sonata-project/SonataBlockBundle#1098

Merged

Simplify more

bdde78e

jordisala1991 force-pushed the hotfix/more-crud-form-tests branch from a02dce0 to bdde78e Compare August 12, 2022 08:15

VincentLanglet approved these changes Aug 12, 2022

View reviewed changes

jordisala1991 merged commit 6304bc8 into sonata-project:4.x Aug 12, 2022

jordisala1991 deleted the hotfix/more-crud-form-tests branch August 12, 2022 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more CRUD form tests #1552

Add more CRUD form tests #1552

jordisala1991 commented Aug 11, 2022 •

edited

Loading

jordisala1991 Aug 11, 2022

VincentLanglet Aug 12, 2022

jordisala1991 Aug 12, 2022

jordisala1991 Aug 12, 2022

jordisala1991 commented Aug 11, 2022 •

edited

Loading

VincentLanglet commented Aug 11, 2022

jordisala1991 Aug 12, 2022

VincentLanglet Aug 12, 2022

jordisala1991 Aug 12, 2022

jordisala1991 Aug 12, 2022

jordisala1991 Aug 12, 2022

VincentLanglet Aug 12, 2022

jordisala1991 Aug 12, 2022

jordisala1991 Aug 12, 2022

VincentLanglet Aug 12, 2022

eerison Aug 12, 2022 •

edited

Loading

jordisala1991 Aug 12, 2022

VincentLanglet Aug 12, 2022

jordisala1991 Aug 12, 2022

VincentLanglet Aug 12, 2022

VincentLanglet Aug 12, 2022

VincentLanglet Aug 12, 2022

VincentLanglet Aug 12, 2022

jordisala1991 Aug 12, 2022

Add more CRUD form tests #1552

Add more CRUD form tests #1552

Conversation

jordisala1991 commented Aug 11, 2022 • edited Loading

Subject

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

jordisala1991 commented Aug 11, 2022 • edited Loading

VincentLanglet commented Aug 11, 2022

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

eerison Aug 12, 2022 • edited Loading

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

jordisala1991 commented Aug 11, 2022 •

edited

Loading

jordisala1991 commented Aug 11, 2022 •

edited

Loading

eerison Aug 12, 2022 •

edited

Loading