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.

Sign up for GitHub

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

Add more CRUD form tests #1552

Merged
merged 8 commits into from
Aug 12, 2022
Merged

Add more CRUD form tests #1552

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8264f9d
Add more CRUD form tests
jordisala1991 Aug 11, 2022
d40cf30
Fix tests
jordisala1991 Aug 11, 2022
9a48a2f
Update composer.json
jordisala1991 Aug 11, 2022
fcc9daa
More block tests
jordisala1991 Aug 12, 2022
28f87f0
Remove old code
jordisala1991 Aug 12, 2022
eb43569
Remove PageFormMapper
jordisala1991 Aug 12, 2022
d922ffe
Simplify logic
jordisala1991 Aug 12, 2022
bdde78e
Simplify more
jordisala1991 Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
"dama/doctrine-test-bundle": "^6.7",
"doctrine/annotations": "^1.13.3",
"friendsofphp/php-cs-fixer": "^3.4",
"masterminds/html5": "^2.7",
"matthiasnoback/symfony-dependency-injection-test": "^4.1.1",
"phpstan/extension-installer": "^1.0",
"phpstan/phpstan": "^1.0",
Expand Down
14 changes: 0 additions & 14 deletions src/Block/ChildrenPagesBlockService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,20 +170,6 @@ public function configureSettings(OptionsResolver $resolver): void
]);
}

public function prePersist(BlockInterface $block): void
{
$page = $block->getSetting('pageId');

$block->setSetting('pageId', $page instanceof PageInterface ? $page->getId() : null);
}

public function preUpdate(BlockInterface $block): void
{
$page = $block->getSetting('pageId');

$block->setSetting('pageId', $page instanceof PageInterface ? $page->getId() : null);
}

public function load(BlockInterface $block): void
{
if (!$block instanceof PageBlockInterface) {
Expand Down
20 changes: 5 additions & 15 deletions src/Block/SharedBlockBlockService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,28 +134,18 @@ public function load(BlockInterface $block): void
$block->setSetting('blockId', $sharedBlock);
}

public function prePersist(BlockInterface $block): void
{
$block = $block->getSetting('blockId');

$block->setSetting('blockId', $block instanceof BlockInterface ? $block->getId() : null);
}

public function preUpdate(BlockInterface $block): void
{
$block = $block->getSetting('blockId');

$block->setSetting('blockId', $block instanceof BlockInterface ? $block->getId() : null);
}

private function getBlockBuilder(): FormBuilderInterface
{
$fieldDescription = $this->sharedBlockAdmin->createFieldDescription('block', [
'translation_domain' => 'SonataPageBundle',
'edit' => 'list',
]);
$fieldDescription->setAssociationAdmin($this->sharedBlockAdmin);

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

Choose a reason for hiding this comment

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

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:

  1. Field description didn't had association admin, and it break on a template from sonataAdmin, fixed line 157.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is:

  1. Some of our bundles define blocks (and they do require block bundle) 👍
  2. Some of that blocks are EditableBlocks (an interface defined on block bundle) 👍
  3. That interface is only testable HERE, because those editableBlocks are for SonataPageBundle 👎
  4. 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) 👎
  5. 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

'sonata_field_description' => $fieldDescription,
'class' => $this->sharedBlockAdmin->getClass(),
'model_manager' => $this->sharedBlockAdmin->getModelManager(),
Expand Down
142 changes: 141 additions & 1 deletion tests/Functional/Admin/SharedBlockAdminTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,151 @@ public function testCrudUrls(string $url, array $parameters = []): void
public static function provideCrudUrlsCases(): iterable
{
yield 'List Block' => ['/admin/tests/app/sonatapageblock/shared/list'];
yield 'Create Block' => ['/admin/tests/app/sonatapageblock/shared/create'];
yield 'List Block types' => ['/admin/tests/app/sonatapageblock/shared/create'];

yield 'Create Block' => ['/admin/tests/app/sonatapageblock/shared/create', [
'type' => 'sonata.page.block.shared_block',
]];

yield 'Edit Block' => ['/admin/tests/app/sonatapageblock/shared/1/edit'];
yield 'Remove Block' => ['/admin/tests/app/sonatapageblock/shared/1/delete'];
}

/**
* @dataProvider provideFormUrlsCases
*
* @param array<string, mixed> $parameters
* @param array<string, mixed> $fieldValues
*/
public function testFormsUrls(string $url, array $parameters, string $button, array $fieldValues = []): void
{
$client = self::createClient();

$this->prepareData();

$client->request('GET', $url, $parameters);
$client->submitForm($button, $fieldValues);
$client->followRedirect();

self::assertResponseIsSuccessful();
}

/**
* @return iterable<array<string|array<string, mixed>>>
*
* @phpstan-return iterable<array{0: string, 1: array<string, mixed>, 2: string, 3?: array<string, mixed>}>
*/
public static function provideFormUrlsCases(): iterable
{
// Blocks From SonataBlockBundle
// yield 'Create Shared Block - Container' => ['/admin/tests/app/sonatapageblock/shared/create', [
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is broken on SonataBlockBundle.

Copy link
Member

Choose a reason for hiding this comment

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

Can it be fix ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

And this PR: sonata-project/SonataBlockBundle#1098

// 'uniqid' => 'shared_block',
// 'type' => 'sonata.block.service.container',
// ], 'btn_create_and_list', [
// 'shared_block[name]' => 'Name',
// 'shared_block[enabled]' => 1,
// 'shared_block[settings][code]' => 'code',
// 'shared_block[settings][layout]' => '{{ CONTENT }}',
// 'shared_block[settings][class]' => 'custom_class',
// 'shared_block[settings][template]' => '@SonataPage/Block/block_container.html.twig',
// 'shared_block[children]' => '',
// ]];

yield 'Create Shared Block - Text' => ['/admin/tests/app/sonatapageblock/shared/create', [
'uniqid' => 'shared_block',
'type' => 'sonata.block.service.text',
], 'btn_create_and_list', [
'shared_block[name]' => 'Name',
'shared_block[enabled]' => 1,
'shared_block[settings][content]' => 'Text',
]];

yield 'Create Shared Block - Template' => ['/admin/tests/app/sonatapageblock/shared/create', [
'uniqid' => 'shared_block',
'type' => 'sonata.block.service.template',
], 'btn_create_and_list', [
'shared_block[name]' => 'Name',
'shared_block[enabled]' => 1,
'shared_block[settings][template]' => '@SonataBlock/Block/block_template.html.twig',
]];

yield 'Create Shared Block - Menu' => ['/admin/tests/app/sonatapageblock/shared/create', [
'uniqid' => 'shared_block',
'type' => 'sonata.block.service.menu',
], 'btn_create_and_list', [
'shared_block[name]' => 'Name',
'shared_block[enabled]' => 1,
'shared_block[settings][title]' => 'Title',
'shared_block[settings][menu_name]' => 'sonata_admin_sidebar',
'shared_block[settings][safe_labels]' => 1,
'shared_block[settings][current_class]' => 'active',
'shared_block[settings][first_class]' => 'first',
'shared_block[settings][last_class]' => 'last',
'shared_block[settings][menu_class]' => 'menu_sonata',
'shared_block[settings][children_class]' => 'children',
'shared_block[settings][menu_template]' => '@SonataBlock/Block/block_core_menu.html.twig',
]];

yield 'Create Shared Block - RSS' => ['/admin/tests/app/sonatapageblock/shared/create', [
'uniqid' => 'shared_block',
'type' => 'sonata.block.service.rss',
], 'btn_create_and_list', [
'shared_block[name]' => 'Name',
'shared_block[enabled]' => 1,
'shared_block[settings][url]' => 'https://sonata-project.org',
'shared_block[settings][title]' => 'Title',
'shared_block[settings][translation_domain]' => 'SonataPageBundle',
'shared_block[settings][icon]' => 'fa fa-home',
'shared_block[settings][class]' => 'custom_class',
]];

// Blocks From SonataPageBundle
yield 'Create Shared Block - Children Pages' => ['/admin/tests/app/sonatapageblock/shared/create', [
'uniqid' => 'shared_block',
'type' => 'sonata.page.block.children_pages',
], 'btn_create_and_list', [
'shared_block[name]' => 'Name',
'shared_block[enabled]' => 1,
'shared_block[settings][title]' => 'Title',
'shared_block[settings][translation_domain]' => 'SonataPageBundle',
'shared_block[settings][icon]' => 'fa fa-home',
'shared_block[settings][current]' => 1,
// 'shared_block[settings][pageId]' => 1,
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 remove this line or uncomment it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

'shared_block[settings][class]' => 'custom_class',
]];

// yield 'Create Shared Block - Breadcrumb' => ['/admin/tests/app/sonatapageblock/shared/create', [
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is broken on SonataSeoBundle.

Copy link
Member

Choose a reason for hiding this comment

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

Can it be fix ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@eerison eerison Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

One for this: sonata-project/SonataSeoBundle#685

// 'uniqid' => 'shared_block',
// 'type' => 'sonata.page.block.breadcrumb',
// ], 'btn_create_and_list', [
// ]];

yield 'Create Shared Block - Shared Block' => ['/admin/tests/app/sonatapageblock/shared/create', [
'uniqid' => 'shared_block',
'type' => 'sonata.page.block.shared_block',
], 'btn_create_and_list', [
'shared_block[name]' => 'Name',
'shared_block[enabled]' => 1,
'shared_block[settings][blockId]' => 1,
]];

yield 'Create Shared Block - Page List' => ['/admin/tests/app/sonatapageblock/shared/create', [
'uniqid' => 'shared_block',
'type' => 'sonata.page.block.pagelist',
], 'btn_create_and_list', [
'shared_block[name]' => 'Name',
'shared_block[enabled]' => 1,
'shared_block[settings][title]' => 'Title',
'shared_block[settings][translation_domain]' => 'SonataPageBundle',
'shared_block[settings][icon]' => 'fa fa-home',
'shared_block[settings][class]' => 'custom_class',
'shared_block[settings][mode]' => 'form.choice_public',
]];

yield 'Edit Shared Block' => ['/admin/tests/app/sonatapageblock/shared/1/edit', [], 'btn_update_and_list', []];
yield 'Remove Shared Block' => ['/admin/tests/app/sonatapageblock/shared/1/delete', [], 'btn_delete'];
}

/**
* @return class-string<KernelInterface>
*/
Expand Down
37 changes: 37 additions & 0 deletions tests/Functional/Admin/SiteAdminTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,43 @@ public static function provideCrudUrlsCases(): iterable
yield 'Snaphosts Site' => ['/admin/tests/app/sonatapagesite/1/snapshots'];
}

/**
* @dataProvider provideFormUrlsCases
*
* @param array<string, mixed> $parameters
* @param array<string, mixed> $fieldValues
*/
public function testFormsUrls(string $url, array $parameters, string $button, array $fieldValues = []): void
{
$client = self::createClient();

$this->prepareData();

$client->request('GET', $url, $parameters);
$client->submitForm($button, $fieldValues);
$client->followRedirect();

self::assertResponseIsSuccessful();
}

/**
* @return iterable<array<string|array<string, mixed>>>
*
* @phpstan-return iterable<array{0: string, 1: array<string, mixed>, 2: string, 3?: array<string, mixed>}>
*/
public static function provideFormUrlsCases(): iterable
{
yield 'Create Site' => ['/admin/tests/app/sonatapagesite/create', [
'uniqid' => 'site',
], 'btn_create_and_list', [
'site[name]' => 'Name',
'site[host]' => 'localhost',
]];

yield 'Edit Site' => ['/admin/tests/app/sonatapagesite/1/edit', [], 'btn_update_and_list', []];
yield 'Remove Site' => ['/admin/tests/app/sonatapagesite/1/delete', [], 'btn_delete'];
}

/**
* @return class-string<KernelInterface>
*/
Expand Down