Propel2
Propel2 copied to clipboard
fix timestampable use of datetimeclass
The timestampable behavior doesn't use the generator.dateTime.dateTimeClass setting.
I took a few liberties adressing this:
- Reusing the getDateTimeClass method of
Propel\Generator\Builder\Om\ObjectBuilder. For this it had to becomepublic. PropelDateTime::createHighPrecisionexpects the output ofmicrotimeto be formatted with a dot. This places the formatting in a separate (static) method. This allows keeping the updated_at and created_at microtimes in preInsert exactly the same value (instead of generating them independently).
I've avoided simplifying code to not break things.
What do you think?
@dereuromark based on other PR's it seems the CI errors where not introduced here. I think it is better to address at them in a separate PR, no?
What do you think about this change?
I get little notifications of GitHub myself, so tagging @PhilinTv and @vol4onok.
@gechetspr it seems you are my best bet to ping :) Thank you!
This PR would enable the timestampable behaviour to use DateTimeImmutable too (instead of only DateTime).
The CI failures are the same as on all other recent PRs (see comment on #2015). I'm not sure what's happening with mysql, but I will try to have a look at the code-style error.
I've resolved your remarks. Good suggestions! Also phpstan and codesniffer weren't happy yet. I've fixed that.
@gechetspr it looks ok now I think.
Tagging @PhilinTv to follow up on this.
merged into perpl
@smhg Thanks again for this high-quality contribution! it's well thought-out and cleanly structured. To strengthen the regression safety and ensure config-driven behavior is fully verified, would you be open to adding a few tests?
- A test ensuring created_at and updated_at share the same microtime in preInsert().
- A test verifying that a custom class passed via dateTimeClass is correctly instantiated.
- A negative test asserting that invalid dateTimeClass values throw exceptions as expected.
Let me know if you need guidance - happy to assist!
@PhilinTv I've added the first 2 tests you mentioned above. It's definitely nice to have those.
If I understand right, the 3rd test you suggest would test the effect of an invalid dateTimeClass configuration property value?
If so, it should probably go into Propel\Tests\Generator\Builder\Om\GeneratedObjectWithDateImmutableClassTest.
But if that's the case, I suggest to keep this out of this PR as that is unrelated. Or do you mean something else?
Thanks @smhg for the quick update!