Add test for weird cases #1532
Add test for weird cases #1532
Conversation
jordisala1991
force-pushed
the
hotfix/tests
branch
from
ae907d6 to
54f4103
Compare
|
As you can see, the first commit shows the issues mentioned by @eerison . And the second commits fixes them. I have added 2 string cases:
|
| @@ -22,7 +22,7 @@ | |||
| * id: int|string|null, | |||
| * name?: string|null, | |||
| * enabled: boolean, | |||
| * position: int|null, | |||
| * position: int|string|null, | |||
There was a problem hiding this comment.
It didn't followed the whole but since you're doing a is numeric check, should it be numeric-string here ?
There was a problem hiding this comment.
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.
src/Entity/Transformer.php
Outdated
| if (isset($content['position'])) { | ||
| $block->setPosition($content['position']); | ||
| if (isset($content['position']) && is_numeric($content['position'])) { | ||
| $block->setPosition(\is_string($content['position']) ? ((int) $content['position']) : $content['position']); |
There was a problem hiding this comment.
What about always casting to int ?
There was a problem hiding this comment.
I will try, I tried to be clever so I don't get complains for phpstan/psalm , for already int casting.
| @@ -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'])) { | |||
There was a problem hiding this comment.
Should we throw an exception when it's set but not a numeric ?
There was a problem hiding this comment.
Not sure TBH, old snapshot data, if it is not easily fixable, might contains some weird data.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah sure. I think you can just cast to int everytime and it's ok for me
|
we should probably do a similar MR in 3.x to stop such values if possible or do we need a doctrine migration script / or Command for this? |
I suggested this few minutes ago in your PR :D I guess we just need to check if there are position or some data saved wrong, we could raise a deprecate message, the guy just need to generate a new snapshot. |
|
Ok I guess it is a good news (I hope 😄 ) I tested this branch, and I don't see the setPosition error anymore! I just see this error But after merge this, probably this PR #1524 gonna fix this issue too :D I guess we could merge this, don't you 😃 |
Yes, I just wait for @jordisala1991 to take a look at my suggestions ;) |
Subject
I am targeting this branch, because this fixes a bug in 4.x, introduced while we upgraded to SonataBlockBundle 4.0
Closes #1496