Map Date/Time types to string to avoid issues importing seeds

[ajibarra]

I think the easier way to fix #558 is mapping date/time types to string so the data is generated as it comes from db. Please let me know if you see any issue with this approach.

I will work on fixing unit tests / adding new ones if needed so I will keep the pull request as draft until then.

Thank you

[ndm2]

The "issue" I see with this is that it only fixes the problem for date/time types, while it exists for basically any type that casts data into non-scalar/null values. The problem also doesn't end with built-in types, it extends to user defined types as well.

I think it would be nice if we could find something that covers a wider range of complex types.

[ajibarra]

@ndm2 yes it makes sense, I will think a bit more about a more generic solution.

[ADmad]

Instead of converting the objects back to scalar why not just disable results casting for the query?

[ajibarra]

Instead of converting the objects back to scalar why not just disable results casting for the query?

yeah it looks like a better approach, I had never used the option and I thought it only applied to built-in types.

[ajibarra]

Instead of converting the objects back to scalar why not just disable results casting for the query?

However it makes that even scalar types to be strings in PHP <8.1 which causes issues with unit tests since id's in PHP <8.1 maps the values as strings and PHP 8.1 maps them correctly as integers. You can see the last two executions for actual examples. See https://www.php.net/manual/en/migration81.incompatible.php#migration81.incompatible.pdo.mysql

I would like to know which approach do you prefer. If disableResultsCasting I will need to check how to make unit tests working between different PHP versions. Otherwise the original approach does not cause issue with tests.

@ADmad what do you think?

[ADmad]

Personally I wouldn't unnecessarily complicate the code to get consistent types across PHP versions for testing. Instead the expected data can be adapted based on PHP version check.

[ndm2]

Since disabling casting was my first thought too, I'd second ADmad.

If we wanted some more compatibility, we could possibly manually convert only the specific types affected by the version difference, which I guess would be integers, floats and booleans?

[ajibarra]

@ADmad I have fixed unit tests. The problem with the build now is that psalm is requiring 7.2 because composer requirement is >= 7.2.0 but it looks it is installing a version of laminas-diactoros which requires php >=7.4. Do you know what could be happening here?

[ADmad]

@ajibarra Try running the "Coding Standard & Static Analysis" job on PHP 7.2 instead of PHP 7.4 so that lower version of diactoros is installed?

[ajibarra]

@ajibarra Try running the "Coding Standard & Static Analysis" job on PHP 7.2 instead of PHP 7.4 so that lower version of diactoros is installed?

According to execution it is already on 7.2:

Run vendor/bin/psalm.phar --output-format=github
Target PHP version: [7](https://github.com/cakephp/migrations/actions/runs/3065629339/jobs/4949920470#step:9:8).2 (inferred from composer.json)
Scanning files...
Analyzing files...

Not sure if I am missing something...

[ADmad]

The job is being run on PHP 7.4

migrations/.github/workflows/ci.yml

Line 110 in b9c1eeb

php-version: '7.4'

[ajibarra]

The job is being run on PHP 7.4

migrations/.github/workflows/ci.yml

Line 110 in b9c1eeb

php-version: '7.4'

Good catch! I was sure it was on 7.2 already because of the output in actions.

Thank you! Both PR are ready #561 and #562