Problem with UPDATE on DATE primary key

[jkavalik]

Hello @hrach , today I stumbled onto another instance of the problem we already discussed in #320 when using

nextras/dbal	v4.0.5
nextras/orm	v4.0.7

Seems that \Nextras\Orm\Mapper\Dbal\DbalMapper::persist() was not fixed and it still contains

$id = (array) $entity->getPersistedId();

which converts the Datetime instance into 3-item array and then tries to use the first part (string) as the PK and fails with Modifier %?dts expects value to be DateTime, string given. again.

I was able to fix it locally with

$id = (array) $entity->getPersistedId();
if ($entity->getPersistedId() instanceof \DateTimeInterface) {
    $id = [$entity->getPersistedId()];
}

I can prepare a pull request with that change in persist(), processMySQLAutoupdate() and remove() if it helps.

[jkavalik]

The problem shows up when the actual column type is DATETIME or DATE, but not for TIMESTAMP which is used in tests for the Logs table/entity.

[hrach]

I'd say the proper fix is

$id = $entity->getPersistedId();
if (!is_array($id)) {
    $id = [$id];
}

that was AFAIR the intention. PR welcomed :)

[jkavalik]

So I managed to fix the tests

• mysql8.4 has different option for native-password
• mssql changed path to sqlcmd binary and needs -C option to trust the server
• I added required tables etc for new tests on all platforms
• and I added the fix you suggested

[jkavalik]

@hrach is there any chance to get it into the 4.0 branch and release?

[hrach]

Any chance you would rather consider updating to 5.0@dev? I tested it on my big project though haven't enough time to release it finally.

[jkavalik]

I can suggest it, as I see no BC breaks. But we have a few projects with a common core which are running in production but with continuous new development and updating major version of libraries takes time (and ORM is quite an entrenched one).

[hrach]

Make sense. I can take a look at how feasible backporting is.

[hrach]

@jkavalik Ok, I tried backporting this but there is a serious problem:

The getBy() call is processing the DateTimeImmutale object as it would be a zoned timestamp and therefore it is not correctly finding the wanted result.

This was fixed in Nextras Orm 5 in this commit:
9cb5ec6

But to apply this on Nextras Orm 4, I would have to do much more serious refactoring/cherry-picking, because the ExpressionResult does not contain there the dbalModifier property, etc.

Simply said, that does not make sense and would be problematic for 4.0 to change behavior.

CI failure can be seen here:
https://github.com/nextras/orm/actions/runs/11262725335/job/31319047807

Removing the commit from 4.0 branch.

[jkavalik]

@hrach thanks for trying :)

But it is weird because I tried it already locally in my project (mysql/mariadb, orm 4.0.7) and it worked fine for my use case at least :) Gotta check what is different in the test..

[jkavalik]

I see, it works for me because of connectionTz: "Europe/Prague".

[jkavalik]

and I can make the tests work by setting tefault TZ to UTC in bootstrap, which makes other tests fail ofc :)

I made the tests work by using the same timezone in the datetime objects as in the db but I was not able to access the actual connectionTz value anywhere from ORM Model or repositories because it is set by the config container and not publicized anywhere that is available from the orm layer.

I understand though that that's not a solution because it is quite cumbersome in the case the db and php timezones differ.
I will either do a fork to temporarily allow it like this for our project or find some workaround until we can update to v5 (probably use string instead of datetime in orm :) )

[hrach]

The core of this is a "mega" issue that Orm pre-5.0 was not using a modifier. Therefore there was autodetection of DateTime type and DATE columns were always malformed. :(

IMO it doesn't make sense to put it to 4.0 officially, if you are able to keep a fork/PR that you can use for your composer, good :)

[jkavalik]

I agree, thank you for your time :)