Antelope-Valley-College / polr

GNU General Public License v2.0
24 stars 10 forks source link

Upgrade Overall Experience #11

Open technowhizz opened 1 year ago

technowhizz commented 1 year ago

The changes are intended to improve the usability and functionality of the project.

Environment Configuration

Looking further into https://github.com/Antelope-Valley-College/polr/issues/9 I found that the useCurrent() function was returning 0000-00-00 00:00:00 instead of CURRENT_TIMESTAMP. The same with useCurrentOnUpdate(). This should hopefully avert this.

Open to other ideas as to why this might be happening but in the meantime this should work

(Messed up the previous PR hence opening this one)

technowhizz commented 1 year ago

From #10

@technowhizz This patch looks great; thank you for your efforts. I have one question about database/migrations/2023_01_24_234401_add_secrets_key_default.php. The code in my repository uses Schema modification commands to remain DB agnostic. I would like to allow for other DBs to be used with POLR in the future. The update I'm seeing is an ALTER statement that only works for MySQL. I don't have much experience with Laravel; this upgrade was a knee-jerk to get to PHP 8. Do you see any technical reason that we could not keep the declarative syntax using Schema?

technowhizz commented 1 year ago

@mwilmes-at-avc I too would love to keep the laravel code to interact with the DB but for some reason it just didnt seem to work. I think others have also faced this issue as noted by @nitz in #9

The issue is also noted at https://github.com/laravel/framework/issues/3602

However it seems like telling laravel that you have a strict db in database.php may fix it. However, this is the default in the repo and hasnt fixed it for me.

technowhizz commented 12 months ago

@mwilmes-at-avc After searching around quite a bit, the code should work but for some reason it just doesn't. As far as I can tell the only way to fix this for now is with the raw SQL. I understand if that means that you no longer want to merge this in as it doesn't permit the use of postgres.

If anyone does manage to fix it do let me know