Resolves #28, #29: "Update timestamps columns on links and tags tables" #38
Conversation
ialexreis
commented
Oct 7, 2020
ialexreis
commented
- drop regular timestamps columns
- create new ones with timestampsTz
|
@josepostiga Here is my draft PR, I will continue to work on that! |
josepostiga
changed the title
josepostiga
changed the title
- drop regular timestamps columns - create new ones with timestampsTz
|
@josepostiga I have a doubt... When you say: "Besides that, ensure that the application layer sets the database timezone based on the incoming request.", what do you expect me to do? Maybe send this information inside the db query, like including the timestamps field inside the query? |
|
@ADeVR when dealing with timezones, the best, less error-prone, strategy, is to ensure that all date/time properties are always stored and get as UTC, calculating the difference from whatever timezone the client sends, and expose that date always as UTC and let the clients being responsible to show it and calculate any time difference as appropriate. However, there are other valid strategies:
I admit the wording, there, wasn't the best and I'll rectify it. But the option I'd like to see implemented is the "save and show always as UTC", as described in the first paragraph of this response. |
|
@josepostiga oh ok, no problem at all! Thinking about a good solution, maybe I should implement a custom cast type for that, and use it in every date, to force that specific behaviour. But in that case, in order to not break the domain driven structure, where do you think that the cast should be located? |
|
@ADeVR that's the default framework's behavior. It'll save any date/time declared property to UTC and get that same property back with the calculated difference to the configured framework timezone (that by default is UTC, too). We just need to ensure that other date/time properties besides the framework "default" ones ( |
|
@josepostiga thanks for the help! I learned a lot with this PR! Anything you want me to improve feel free to comment about it! I'm looking forward to help! |
There was a problem hiding this comment.
Thanks for submitting this PR @ADeVR. There's just a simple detail we need to work out but, after that, I believe we'll be ready to merge this! 👍
| public function up(): void | ||
| { | ||
| Schema::table('links', function (Blueprint $table) { | ||
| $table->dropColumn(["created_at", "updated_at", "approved_at", "deleted_at"]); |
There was a problem hiding this comment.
In a continuous integration mindset, we never drop columns that might be in use, because that leads to loose of data. That's a valid strategy for unreleased software, which is not the case of this API.
Notice we already have some releases, and although we might assume that doing this is very unlikely to break anyone's code, we need to always think about what if someone already has data inserted that they don't want to lose?
I suggest we change this to use the change() method and let the database engine execute the proper SQL to update the already existent columns to the new type.
There was a problem hiding this comment.
@josepostiga sorry for the delay answering this. Since the documentation, states that the "Only the following column types can be changed", I've decided to not use the timestampsTz method and instead used the datetimeTz method, which allows me to change the types of the column!
Hope to contribute more! 😄
Sure! I can do that now! |
timestamps columns (on links and tags) changed to datetime w/timezone
|
Changelog updated! 💯 |
… columns on links and tags tables" (laravel-portugal#38) * feat(Tags): add support to timestamps w/timezones - drop regular timestamps columns - create new ones with timestampsTz * feat(Links): add support to timestamps w/timezones added doctrine/dbal dependency * fix: add method return type * fix(Links): add missing timestamp (approved_at) * chore: converted timestampTz to softDeletesTz * feat(links): added approved_at datetime cast * fix: replaced dropColumn with change method * chore(Changelog): add latest updates timestamps columns (on links and tags) changed to datetime w/timezone
* init * Adds Question create feature resolves #25 Removes unnecessary code in AccountsStoreController. * Added '/discussions' prefix As per #25 (comment) * Minor fixes as per review #40 (review) * Cleanup * Added question update * Added tests * Resolves #28, #29: "Update timestamps columns on links and tags tables" (#38) * feat(Tags): add support to timestamps w/timezones - drop regular timestamps columns - create new ones with timestampsTz * feat(Links): add support to timestamps w/timezones added doctrine/dbal dependency * fix: add method return type * fix(Links): add missing timestamp (approved_at) * chore: converted timestampTz to softDeletesTz * feat(links): added approved_at datetime cast * fix: replaced dropColumn with change method * chore(Changelog): add latest updates timestamps columns (on links and tags) changed to datetime w/timezone * Resolves #25 - An authenticated user can post a question (#40) * init * Adds Question create feature resolves #25 Removes unnecessary code in AccountsStoreController. * Added '/discussions' prefix As per #25 (comment) * Minor fixes as per review #40 (review) * Cleanup * Resolve conflicts + test tweaks * Code cleanup + minor improvements * minor tweaks * final tweaks/cleanup * style(Discussions): minor code style fixes * ci(PHPUnit): add blade files to the code coverage exclusion list Co-authored-by: Alexandre Reis <[email protected]> Co-authored-by: José Postiga <[email protected]>
