Add support for using a custom Date class #532

MasonM · 2016-04-01T16:54:40Z

If your project uses a date library that doesn't extend \DateTime (e.g. Zend_Date), then you
currently have to convert to and from ActiveRecord\DateTime when interacting with models, which is a
pain and error prone. We've had to fix quite a number of bugs due to missing conversions. This
commit removes the need for that by adding support for using a custom date class.

The custom class is set in ActiveRecord\Config, which uses similar logic to the logger methods for
the getter and setter. The ActiveRecord\Config->set_date_class() method checks if the provided class
has "format" and "createFromFormat" methods, which are all that are required.

I changed everything that created new date objects to use "createFromFormat()" instead of the
constructor, because many date libraries don't accept a date string in the constructor. Besides, I
think createFromFormat() is better in general because it's more explicit.

For the "attribute_of" method, I made that an optional feature by introducing an interface that
defines it, which ActiveRecord\Model->assign_attribute() checks for. The reason I made this optional
is because if you use an immutable date class (e.g. PHP's DateTimeImmutable), you don't need to
worry about the dirty flagging, so there's no need to have that method.

If your project uses a date library that doesn't extend \DateTime (e.g. Zend_Date), then you currently have to convert to and from ActiveRecord\DateTime when interacting with models, which is a pain and error prone. We've had to fix quite a number of bugs due to missing conversions. This commit removes the need for that by adding support for using a custom date class. The custom class is set in ActiveRecord\Config, which uses similar logic to the logger methods for the getter and setter. The ActiveRecord\Config->set_date_class() method checks if the provided class has "format" and "createFromFormat" methods, which are all that are required. I changed everything that created new date objects to use "createFromFormat()" instead of the constructor, because many date libraries don't accept a date string in the constructor. Besides, I think createFromFormat() is better in general because it's more explicit. For the "attribute_of" method, I made that an optional feature by introducing an interface that defines it, which ActiveRecord\Model->assign_attribute() checks for. The reason I made this optional is because if you use an immutable date class (e.g. PHP's DateTimeImmutable), you don't need to worry about the dirty flagging, so there's no need to have that method.

koenpunt · 2016-04-07T15:21:43Z

test/helpers/config.php

This change is unrelated

Sorry, that's been fixed. Let me know when/if you'd like me to squash these commits into one.

Have you considered adding a .editorconfig file? That would prevent these sorts of issues. I can enter another PR with one if that sounds like something you'd like to see.

In this case editorconfig is unrelated, because this wouldn't have been edited anyway :) Even though if we would add a editorconfig, it would be set to insert a newline, which then will gradually roll out through. So yeah I'm pro-editorconfig.

koenpunt · 2016-04-07T15:24:18Z

This could be valuable, but I don't like the name for DateTimeLinkedModelInterface. Can't it just be DateTimeInterface?
Also, I think createFromFormat should be in that interface as well.

…ble-date-upstream

MasonM · 2016-04-07T18:32:47Z

Yeah, I realize now that DateTimeLinkedModelInterface isn't a good name. It's not specific to DateTime at all, since any class can implement it and the code in Model->assign_attribute() will call attribute_of() on any such objects. However, this functionality could be useful if one wants to use non-date objects that link to models, e.g. for dirty flagging or validation purposes. That's a bit outside the scope of this PR, but if that sounds like something you'd like to see, then renaming the interface to LinkedModelInterface would make the most sense IMO.

Otherwise, DateTimeInterface sounds good. I'm assuming you're fine keeping the duck-typing approach in ActiveRecord\Config->set_date_class(). I don't think it should require that date classes implement DateTimeInterface, because there's no point in implementing attribute_of() if you use an immutable date class, and if you use library like Zend_Date then you'd have to add a wrapper class to implement the interface.

koenpunt · 2016-04-08T09:10:01Z

lib/Model.php

@@ -460,12 +460,15 @@ public function assign_attribute($name, $value)
 		}

 		// convert php's \DateTime to ours
-		if (!($value instanceof DateTime) && $value instanceof \DateTime)
-			$value = new DateTime($value->format('Y-m-d H:i:s T'));
+		if (is_object($value) && get_class($value) === 'DateTime') {


The double instanceof has been introduced just recently, and according to a (not so scientific?) benchmark the instanceof was performing better than get_class with string comparison.

See that conversation here: #531 (comment)

…tribute()

@koenpunt

As suggested by @koenpunt. I also added createFromFormat() and format() to the interface.

MasonM · 2016-04-11T20:17:20Z

Okay, I renamed DateTimeLinkedModelInterface to DateTimeInterface like you suggested. I guess you weren't too hot on the LinkedModelInterface idea, which is fine. I also changed the checks in ActiveRecord\Model->assign_attribute() to use instanceof.

Thanks for looking at this, and let me know if there's anything else.

jpfuentes2 · 2016-04-22T19:25:53Z

This conflicts with #533 -- I would choose this over the other since the extension is helpful. Thoughts, @koenpunt ?

MasonM · 2016-04-22T19:30:11Z

@jpfuentes2 I don't mind resolving the conflict, so feel free to merge #533 first. It can be resolved by just passing the timezone as the third parameter to createFromFormat().

jpfuentes2 · 2016-04-22T19:33:53Z

@MasonM great! I was going to wait for a test from #533 but your test additions will cover the change.

jpfuentes2 · 2016-04-22T19:34:47Z

@MasonM Just merged #533 -- your turn :)

…e-date-upstream

MasonM · 2016-04-23T00:19:54Z

Okay, it should be good to merge now.

koenpunt · 2016-04-23T22:55:51Z

Thanks! 🚀

visavi · 2016-05-12T18:43:37Z

PHP 7.0, mysql 5.7, ubuntu
after the upgrade stopped working

I understand you need somewhere to set the date format?

PDOException: SQLSTATE[22007]: Invalid datetime format: 1292 Incorrect datetime value: '2016-05-12 21:40:03 MSK' for column 'updated_at' at row 1 in ***/vendor/php-activerecord/php-activerecord/lib/Connection.php:322 Stack trace: #0

MasonM · 2016-05-12T18:58:19Z

@visavi I'm pretty sure that error isn't the result of this PR. The message indicates it happened in ActiveRecord\Connection->query(), which is typically called by ActiveRecord\Table. The Table class converts Date objects to strings using the ActiveRecord\Connection->datetime_to_string() method. That method uses ActiveRecord\Connection::$datetime_format to format the string.

The problem is that $datetime_format includes the timezone by default (via the T placeholder), which it seems MySQL doesn't accept in strict mode (as determined by the sql_mode setting). You can either override ActiveRecord\Connection::$datetime_format or change your sql_mode to disable strict mode.

Rican7 · 2016-05-19T15:32:53Z

Damn. @koenpunt the merge order of this PR and #533 caused a regression here. Now the time-zone is back to being pulled from the string.

PR incoming... /cc: @jpfuentes2

koenpunt · 2016-05-19T16:36:58Z

Hm, how did I miss that..

Rican7 · 2016-05-19T16:57:29Z

Eh, it happens. More tests always helps these kinds of things, but I've got that covered in the PR:
#539

MasonM · 2016-05-19T16:57:31Z

@Rican7 sorry about that. I wasn't aware that the timezone abbreviation represented by "T" could be different than the "full" timezone. That's pretty weird.

Rican7 · 2016-05-19T17:00:34Z

@MasonM no problem. Yea, time-zones are hard. Abbreviations can be ambiguous. The only safe way is to use IANA identifiers. Regardless of what's used, though, the trick is to always grab the object as is, instead of serializing and then deserializing, you know?

Bugfix - Explicit Time Zone assignment regression from #532

If your project uses a date library that doesn't extend \DateTime (e.g. Zend_Date), then you currently have to convert to and from ActiveRecord\DateTime when interacting with models, which is a pain and error prone. We've had to fix quite a number of bugs due to missing conversions. This commit removes the need for that by adding support for using a custom date class. The custom class is set in ActiveRecord\Config, which uses similar logic to the logger methods for the getter and setter. The ActiveRecord\Config->set_date_class() method checks if the provided class has "format" and "createFromFormat" methods, which are all that are required. I changed everything that created new date objects to use "createFromFormat()" instead of the constructor, because many date libraries don't accept a date string in the constructor. Besides, I think createFromFormat() is better in general because it's more explicit. For the "attribute_of" method, I made that an optional feature by introducing an interface that defines it, which ActiveRecord\Model->assign_attribute() checks for. The reason I made this optional is because if you use an immutable date class (e.g. PHP's DateTimeImmutable), you don't need to worry about the dirty flagging, so there's no need to have that method.

koenpunt reviewed Apr 7, 2016
View reviewed changes

MasonM added 2 commits April 7, 2016 13:52

Merge remote-tracking branch 'remotes/upstream/master' into configura…

afb332e

…ble-date-upstream

Remove unnessary EOF newline additions

637efd1

koenpunt reviewed Apr 8, 2016
View reviewed changes

MasonM added 2 commits April 11, 2016 15:42

Change to use instanceof for checking Date values in Model->assign_at…

a5bfee0

…tribute()

Rename DateTimeLinkedModelInterface -> DateTimeInterface

532a394

As suggested by @koenpunt. I also added createFromFormat() and format() to the interface.

jpfuentes2 mentioned this pull request Apr 22, 2016

Fix timezone bug in active record DateTime creation #521

Closed

Merge https://github.com/jpfuentes2/php-activerecord into configurabl…

000cf06

…e-date-upstream

koenpunt merged commit d4b980f into jpfuentes2:master Apr 23, 2016

MasonM deleted the configurable-date-upstream branch May 13, 2016 00:07

brianmuse mentioned this pull request May 19, 2016

Bugfix: assign_attribute timezone fix robinpowered/php-activerecord#7

Merged

Rican7 mentioned this pull request May 19, 2016

Bugfix - Explicit Time Zone assignment regression from #532 #539

Merged

jpfuentes2 added a commit that referenced this pull request May 20, 2016

Merge pull request #539 from Rican7/bugfix/timezone-regression

a9349ea

Bugfix - Explicit Time Zone assignment regression from #532

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for using a custom Date class #532

Add support for using a custom Date class #532

MasonM commented Apr 1, 2016

koenpunt Apr 7, 2016

MasonM Apr 7, 2016

koenpunt Apr 8, 2016

koenpunt Apr 8, 2016

koenpunt commented Apr 7, 2016

MasonM commented Apr 7, 2016

koenpunt Apr 8, 2016

MasonM commented Apr 11, 2016

jpfuentes2 commented Apr 22, 2016 •

edited

Loading

MasonM commented Apr 22, 2016

jpfuentes2 commented Apr 22, 2016

jpfuentes2 commented Apr 22, 2016 •

edited

Loading

MasonM commented Apr 23, 2016

koenpunt commented Apr 23, 2016

visavi commented May 12, 2016

MasonM commented May 12, 2016

Rican7 commented May 19, 2016

koenpunt commented May 19, 2016

Rican7 commented May 19, 2016

MasonM commented May 19, 2016

Rican7 commented May 19, 2016

Add support for using a custom Date class #532

Add support for using a custom Date class #532

Conversation

MasonM commented Apr 1, 2016

koenpunt Apr 7, 2016

Choose a reason for hiding this comment

MasonM Apr 7, 2016

Choose a reason for hiding this comment

koenpunt Apr 8, 2016

Choose a reason for hiding this comment

koenpunt Apr 8, 2016

Choose a reason for hiding this comment

koenpunt commented Apr 7, 2016

MasonM commented Apr 7, 2016

koenpunt Apr 8, 2016

Choose a reason for hiding this comment

MasonM commented Apr 11, 2016

jpfuentes2 commented Apr 22, 2016 • edited Loading

MasonM commented Apr 22, 2016

jpfuentes2 commented Apr 22, 2016

jpfuentes2 commented Apr 22, 2016 • edited Loading

MasonM commented Apr 23, 2016

koenpunt commented Apr 23, 2016

visavi commented May 12, 2016

MasonM commented May 12, 2016

Rican7 commented May 19, 2016

koenpunt commented May 19, 2016

Rican7 commented May 19, 2016

MasonM commented May 19, 2016

Rican7 commented May 19, 2016

jpfuentes2 commented Apr 22, 2016 •

edited

Loading

jpfuentes2 commented Apr 22, 2016 •

edited

Loading