Fix pipeline TransformerInterface

[eerison]

Hello Hello

I was trying to fix the pipelines But I saw that it is complain about id,created_at and updated_at.

Here it is defined that those fields are not optional: https://github.com/sonata-project/SonataPageBundle/blame/4.x/src/Model/TransformerInterface.php#L48

But in this line it is removing in case those fields are null https://github.com/sonata-project/SonataPageBundle/blob/4.x/src/Entity/Transformer.php#L153

Just to be sure those fields must exist even if it's null , right @Hanmac ?

[Hanmac]

Ugh I need to test again how the datetime normalizer handles the code

I will check this out soon

For me, I think the property can be marked as missing created_at?

[eerison]

I guess those fields are need, there are others places expecting that it exists.

[Hanmac]

I guess those fields are need, there are others places expecting that it exists.

I need to test it if that's really the case

Can't do it right now

I would try to make an PR with the changes in the Interface, and then try to write test cases for what happens if the value are null

Or have you found places where it explicit looks for the values?

[eerison]

https://github.com/sonata-project/SonataPageBundle/blob/4.x/src/Entity/Transformer.php#L153

I guess it is in this line: https://github.com/sonata-project/SonataPageBundle/blob/4.x/src/Entity/Transformer.php#L153

[VincentLanglet]

Hi. I already take a look.

This can be done: https://github.com/sonata-project/SonataPageBundle/pull/1729/commits/6854a19299bf05712b0737d9e9324a417ed39169

But we still get an error with phpstan because of https://phpstan.org/r/7c91cd9d-2db3-4fb1-bdfb-a35685bd8921

On the other side, removing the var annotation solve the issue because phpstan is good enought to compute the type, but psalm doesn't.

Static analysis Ci can be solved by removing the filter of null values https://github.com/sonata-project/SonataPageBundle/pull/1729/commits/e97d3f1ac6c18f810091b7046855f2d821d22b78 thougth. I'm not sure why we filter them. (But test need to be fixed)

Anyway I will end ignoring psalm issue I think, but I opened https://github.com/phpstan/phpstan/discussions/10185

[eerison]

Hey @VincentLanglet I guess there is an issue there,

I created a test for this and the current transformers are not handling id, created_at and updated_at

I also initiate a new PR I I didn't have time to finish :( https://github.com/sonata-project/SonataPageBundle/pull/1730/files

[Hanmac]

I'm not sure why we filter them. (But test need to be fixed)

Because I did: AbstractObjectNormalizer::SKIP_NULL_VALUES => true

and I needed them to be consistent.

My goal there was to skip unnecessary data in the database

[eerison]

Ok I looked into the Messages from your PR https://github.com/sonata-project/SonataPageBundle/pull/1522

and I didn't find that those fields are required (or not required 😄 ), maybe the definition in phpstan did a confusion. then we can assume that those fields are also optional ;)

[VincentLanglet]

I'm not sure why we filter them. (But test need to be fixed)

Because I did: AbstractObjectNormalizer::SKIP_NULL_VALUES => true

and I needed them to be consistent.

My goal there was to skip unnecessary data in the database

That may be a mistake: Prior to your PR the array_filter wasn't here: https://github.com/sonata-project/SonataPageBundle/commit/bb9a92c28b77d4a4b6ec97bd7d5855d70b1a3c2e#diff-2b5886b85965d7a571112e42d67c7076a1f4914c1844239e468fe7bd6d51c6d4L91-L107

1) Your PR kinda introduce a BC break in the way the snapshot are not the same before and after your PR. By removing null values, all keys are not set anymore (like created_at, updated_at, id). Nobody opened an issue, so this doesn't seems to be a big deal...

2) The else part is "deprecated" since we inject a Serializer and the else part is only accessible for someone doing new Transformer(...) without a Serializer ; we don't have to add extra work for deprecated code path.

[Hanmac]

would it help if we say that https://github.com/sonata-project/SonataPageBundle/blob/4.x/src/Entity/Transformer.php#L131 this $data variable is of the Type PageContent too?

[VincentLanglet]

No I tried, but I think I'll just remove the phpdoc and ignore the psalm issue ; anyway it's a deprecated code path.

The other solution would be to:

• remove the AbstractObjectNormalizer::SKIP_NULL_VALUES => true, line, which ensure all the key are here
• remove the array_filter call

I wonder if it does more sens ? @Hanmac @eerison

[Hanmac]

i don't use PageBundle anyway, so I'm fine with either i just thought that less data to store would be better

i think one other reason might be in case the getter can return null (if a value isn't set) but the setter doesn't support setting null. but i don't remember if we had such a case (see BlockTypeExtractor::NULLABLE_STRINGS for examples)

in such case it would be better to skip null values, so the setter can be skipped instead

[eerison]

No I tried, but I think I'll just remove the phpdoc and ignore the psalm issue ; anyway it's a deprecated code path.

The other solution would be to:

• remove the AbstractObjectNormalizer::SKIP_NULL_VALUES => true, line, which ensure all the key are here
• remove the array_filter call

I wonder if it does more sens ? @Hanmac @eerison

Hey @VincentLanglet

I guess we could keep the same logical that we have, we already have people using version 4 and no one complain about this. I can check if someone from my last company is already using version 4 and post here.

But if there isn't issue until now I would keep this as it's.

[eerison]

No I tried, but I think I'll just remove the phpdoc and ignore the psalm issue ; anyway it's a deprecated code path. The other solution would be to:

• remove the AbstractObjectNormalizer::SKIP_NULL_VALUES => true, line, which ensure all the key are here
• remove the array_filter call

I wonder if it does more sens ? @Hanmac @eerison

Hey @VincentLanglet

I guess we could keep the same logical that we have, we already have people using version 4 and no one complain about this. I can check if someone from my last company is already using version 4 and post here.

But if there isn't issue until now I would keep this as it's.

Just to point out, I just confirmed and they are using version 4, and this project comes from a old sonata page bundle version. then let's keep the current approach ;)

if someone open an issue then we can figure out this :)

[VincentLanglet]

Solved by https://github.com/sonata-project/SonataPageBundle/pull/1729