CodeSleeve / stapler

ORM-based file upload package for php.
http://codesleeve.com
Other
538 stars 144 forks source link

Date format hard-coded into Attachment #155

Open hackel opened 8 years ago

hackel commented 8 years ago

https://github.com/CodeSleeve/stapler/blob/master/src/Attachment.php#L134

When using a different date format (such as ISO dates for MongoDB), this causes a frustrating InvalidArgumentException: "Unexpected data found. Data missing"

For Laravel, the correct date format is stored in the model instance's protected $dateFormat property, which unfortunately can only be retrieved by the protected getDateFormat method. The trick is to access the method statically, which gets handled by the __callStatic magic method:

$this->instanceWrite('updated_at', date(call_user_func([get_class($this->instance), 'getDateFormat'])));

I'm not sure how this could be best generalized to a non-Laravel-specific solution, I guess adding the format as a config property? In the meantime, I had to override the setAttribute method on my model class to detect and correctly parse the SQL date format.

tabennett commented 8 years ago

Can you show me an example of what you're talking about here? It would really help me understand what the issue so that I can get this one addressed too.

hackel commented 8 years ago

The issue is this line:

$this->instanceWrite('updated_at', date('Y-m-d H:i:s'));

That doesn't use whatever custom date format is defined in the model. So the model tries to set the updated_at attribute by first converting it to a DateTime in Model::asDateTime($value) by calling Carbon::createFromFormat($this->getDateFormat(), $value).

In my case, getDateFormat() returns 'Y-m-d\TH:i:s.uP', so Carbon isn't able to correctly parse the SQL-style date, generating the error. Hopefully this makes sense!

tabennett commented 8 years ago

I'm going to update this so that the the Attachment class is just setting that column as a simple DateTime instance like this:

$this->instanceWrite('updated_at', new DateTime);

This should default the formatting of that column to whatever the default format is on the DB's storage connection, which allows Stapler to be more format agnostic and helps to keep control of that kind of thing in the hands of the developer. In your case, using this change you should just be able to add that field to your the protected $dates property of your model to have it passed through the model's fromDateTime() method and formatted correctly. If you see any reason why this might not work please let me know. I'm going to be writing some tests for this and pushing it out in the next release.