search

propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 398 forks source link

Breaking change in timeformat between alpha11 and alpha12 #1780

Open mstie opened 3 years ago

mstie commented 3 years ago

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.

dereuromark commented 3 years ago

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.

mstie commented 3 years ago

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.

dereuromark commented 3 years ago

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?