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 test for weird cases #1532

Merged
merged 3 commits into from
Aug 9, 2022
Merged

Add test for weird cases #1532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54f4103
Add test for weird cases
jordisala1991 Aug 9, 2022
96bb869
Fix test suite
jordisala1991 Aug 9, 2022
82d8126
Update src/Entity/Transformer.php
jordisala1991 Aug 9, 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
4 changes: 2 additions & 2 deletions src/Entity/Transformer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,8 @@ public function loadBlock(array $content, PageInterface $page): PageBlockInterfa

$block->setEnabled($content['enabled']);

if (isset($content['position'])) {
$block->setPosition($content['position']);
if (isset($content['position']) && is_numeric($content['position'])) {
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 throw an exception when it's set but not a numeric ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure TBH, old snapshot data, if it is not easily fixable, might contains some weird data.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I would go for the , let's try to see what data can get on the snapshot content and fix all the errors. Later we might want to provide a command to fix snapshots, or fix snapshots on the fly while being loaded. Not sure right now, but at a first step, I would prefer to not throw a lot of exceptions on old data.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sure. I think you can just cast to int everytime and it's ok for me

$block->setPosition((int) $content['position']);
}

$block->setSettings($content['settings']);
Expand Down
4 changes: 2 additions & 2 deletions src/Model/TransformerInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
* id: int|string|null,
* name?: string|null,
* enabled: boolean,
* position: int|null,
* position: int|string|null,
Copy link
Member

Choose a reason for hiding this comment

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

It didn't followed the whole but since you're doing a is numeric check, should it be numeric-string here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

numeric-string is more restrictive than string. IMO we should try to see if the string is numeric, so we cast it, or is not numeric, so we not setPosition.

* settings: array<string, mixed>,
* type: string|null,
* created_at: int|null,
Expand All @@ -32,7 +32,7 @@
* id: int|string|null,
* name?: string|null,
* enabled: boolean,
* position: int|null,
* position: int|string|null,
* settings: array<string, mixed>,
* type: string|null,
* created_at: int|null,
Expand Down
130 changes: 97 additions & 33 deletions tests/Entity/TransformerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,19 @@ final class TransformerTest extends TestCase
/**
* @var MockObject&SnapshotManagerInterface
*/
protected $snapshotManager;
private $snapshotManager;

/**
* @var MockObject&PageManagerInterface
*/
protected $pageManager;
private $pageManager;

/**
* @var MockObject&ManagerInterface<PageBlockInterface>
*/
protected $blockManager;
private $blockManager;

protected TransformerInterface $transformer;
private TransformerInterface $transformer;

protected function setUp(): void
{
Expand Down Expand Up @@ -147,6 +147,7 @@ public function testLoadSnapshotToPage(): void
$snapshot = new SonataPageSnapshot();
$snapshot->setContent($this->getTestContent($dateTime));
$snapshot->setUrl('/get-child');

$page = $this->transformer->load($snapshot);

static::assertSame('page_child', $page->getId());
Expand All @@ -155,23 +156,94 @@ public function testLoadSnapshotToPage(): void
static::assertSame('/get-child', $page->getUrl());
}

public function testLoadBlock(): void
/**
* @dataProvider blockProvider
*
* @param array<string, mixed> $content
*
* @phpstan-param BlockContent $content
*/
public function testLoadBlock(array $content): void
{
$this->blockManager->method('create')->willReturnCallback(static fn () => new SonataPageBlock());

$dateTime = new \DateTime();
$block = $this->transformer->loadBlock($content, new SonataPagePage());

$page = new SonataPagePage();
static::assertSame($content['id'], $block->getId());
}

/**
* @phpstan-return iterable<array{BlockContent}>
*/
public function blockProvider(): iterable
{
$datetime = new \DateTime();

$block = $this->transformer->loadBlock($this->getTestBlockArray($dateTime), $page);
// Normal block
yield [[
'id' => 1,
'name' => 'block1',
'enabled' => true,
'position' => 0,
'settings' => [],
'type' => 'type',
'created_at' => (int) $datetime->format('U'),
'updated_at' => (int) $datetime->format('U'),
'blocks' => [[
'id' => 2,
'name' => 'block2',
'enabled' => true,
'position' => 1,
'settings' => [],
'type' => 'type',
'created_at' => (int) $datetime->format('U'),
'updated_at' => (int) $datetime->format('U'),
'blocks' => [],
]],
]];

// Minimal block block
yield [[
'id' => null,
'enabled' => false,
'position' => null,
'settings' => [],
'type' => null,
'created_at' => null,
'updated_at' => null,
'blocks' => [],
]];

// Weird block data
yield [[
'id' => 'random_string',
'name' => 'block1',
'enabled' => true,
'position' => '',
'settings' => [],
'type' => 'type',
'created_at' => null,
'updated_at' => null,
'blocks' => [],
]];

static::assertSame('block123', $block->getId());
// Numeric string position block data
yield [[
'id' => 'block123',
'enabled' => false,
'position' => '0',
'settings' => [],
'type' => null,
'created_at' => null,
'updated_at' => null,
'blocks' => [],
]];
}

/**
* @phpstan-return PageContent
*/
protected function getTestContent(\DateTimeInterface $datetime): array
private function getTestContent(\DateTimeInterface $datetime): array
{
return [
'id' => 'page_child',
Expand All @@ -188,37 +260,29 @@ protected function getTestContent(\DateTimeInterface $datetime): array
'updated_at' => (int) $datetime->format('U'),
'slug' => null,
'parent_id' => 'page_parent',
'blocks' => [
$this->getTestBlockArray($datetime),
],
];
}

/**
* @phpstan-return BlockContent
*/
protected function getTestBlockArray(\DateTimeInterface $datetime): array
{
return [
'id' => 'block123',
'name' => 'block1',
'enabled' => false,
'position' => 0,
'settings' => [],
'type' => 'type',
'created_at' => (int) $datetime->format('U'),
'updated_at' => (int) $datetime->format('U'),
'blocks' => [
[
'id' => 'block234',
'name' => 'block2',
'id' => 'block123',
'name' => 'block1',
'enabled' => false,
'position' => 0,
'settings' => [],
'type' => 'type',
'created_at' => (int) $datetime->format('U'),
'updated_at' => (int) $datetime->format('U'),
'blocks' => [],
'blocks' => [
[
'id' => 'block234',
'name' => 'block2',
'enabled' => false,
'position' => 0,
'settings' => [],
'type' => 'type',
'created_at' => (int) $datetime->format('U'),
'updated_at' => (int) $datetime->format('U'),
'blocks' => [],
],
],
],
],
];
Expand Down