Propel2 icon indicating copy to clipboard operation
Propel2 copied to clipboard

fix timestampable use of datetimeclass

Open smhg opened this issue 1 year ago • 2 comments

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 become public.
  • PropelDateTime::createHighPrecision expects the output of microtime to 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?

smhg avatar Sep 26 '24 12:09 smhg

@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?

smhg avatar Sep 27 '24 11:09 smhg

I get little notifications of GitHub myself, so tagging @PhilinTv and @vol4onok.

smhg avatar Oct 04 '24 09:10 smhg

@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.

smhg avatar Dec 10 '24 13:12 smhg

I've resolved your remarks. Good suggestions! Also phpstan and codesniffer weren't happy yet. I've fixed that.

smhg avatar Dec 10 '24 16:12 smhg

@gechetspr it looks ok now I think.

smhg avatar Dec 10 '24 17:12 smhg

Tagging @PhilinTv to follow up on this.

smhg avatar Apr 23 '25 14:04 smhg

merged into perpl

mringler avatar Apr 28 '25 21:04 mringler

@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 avatar May 14 '25 16:05 PhilinTv

@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?

smhg avatar May 14 '25 21:05 smhg

Thanks @smhg for the quick update!

PhilinTv avatar May 19 '25 14:05 PhilinTv