Propel2 icon indicating copy to clipboard operation
Propel2 copied to clipboard

Breaking change in timeformat between alpha11 and alpha12

Open mstie opened this issue 4 years ago • 3 comments

In alpha11, the ObjectBuilder used format('c') to format timestamps.

https://github.com/propelorm/Propel2/blob/2.0.0-alpha11/src/Propel/Generator/Builder/Om/ObjectBuilder.php#L2979

However, in alpha12 this changed to format('Y-m-d H:i:s.u') for the default platform with no apparent way to override. This causes the following error when trying to format the time. The "timezone" error appears to be incorrect as it actually can't format the timestamp properly

Carbon\Exceptions\InvalidFormatException: Unexpected data found.
The timezone could not be found in the database

This has prevented us from moving to alpha12.

mstie avatar Aug 26 '21 16:08 mstie

If you check https://github.com/propelorm/Propel2/pull/1470 and the linked issue, you will see that this was done to fix an issue. I wonder what could be done here to also resolved your issue.

dereuromark avatar Aug 26 '21 20:08 dereuromark

I believe it was done because two different formats were being used for timing (correct me if I'm wrong). The problem is that the format is hardcoded in both alpha11 and alpha12 but they are different formats.

When we build our Propel model, there is a line of code:

$result[$fieldName] = \Carbon\Carbon::createFromFormat(\DateTime::ATOM, $result[$fieldName], 'UTC')->toDateTimeString();

Carbon can't format this since the $result[$fieldName] returns a different format in alpha11 than in alpha12. Carbon actually throws the error about timezones which is a problem as well since it's not a timezone issue.

In propel.yml.dist there is the option to add propel.generator.database.defaultTimeStampFormat but this isn't used for this case. Rather the timestamp format is hardcoded for each provider.

My recommendation would be to provide a way to write a custom provider or provide an override in the yml file. I'm not sure how this passed testing since it is a breaking change from what we are seeing (although i'm not entirely sure why).

I'm brand new to propel (it's in a legacy codebase we have that I'm just now getting into) so I apologize if this doesn't make all that much sense.

mstie avatar Aug 26 '21 21:08 mstie

We are supposed to release today a beta1 actually, I am not sure if we can stop that process and try to fix this issue before that, or if it would have to wait till beta2 in a few months.

@mringler What do you think?

dereuromark avatar Aug 27 '21 08:08 dereuromark