DarkRP
DarkRP copied to clipboard
FPtje
Fix error when tmysql4 is preferred but not installed
Also I disagree with how the fix is implemented. tmysql should be checked to see if it's an existing table before usage, but an error should still be thrown that the preferred module isn't installed instead of silently moving on.
You made a typo.
I checked again and I don't see it? Where is the typo so I can fix it?
Also I disagree with how the fix is implemented. tmysql should be checked to see if it's an existing table before usage, but an error should still be thrown that the preferred module isn't installed instead of silently moving on.
I can reverse the checks in the if statement when I get back to my pc. I agree with you about throwing an error, should probably call ErrorNoHalt if the preferred module is not installed. I just wanted to fix the current implementation without making any changes. I can add this too if need be.
Please note that MySQLite has its own repo: https://github.com/FPtje/mysqlite
This is why the check-modified-subtree check fails.
You made a typo.
I checked again and I don't see it? Where is the typo so I can fix it?
~~tmsql~~
Nevermind I misread.
@Kefta that looks good to me, am I mistaken?
local moo, tmsql = file.Exists("bin/gmsv_mysqloo_*.dll", "LUA"), file.Exists("bin/gmsv_tmysql4_*.dll", "LUA")
if MySQLite_config.Preferred_module == "tmysql4" and tmsql then
Please note that MySQLite has its own repo: https://github.com/FPtje/mysqlite
This is why the check-modified-subtree check fails.
Do you want me to close this and make a PR in that repo instead? Or would you like me to make an identical PR in that repo as well as this one? Also, should I reverse the order of the checks and throw an error like @Kefta suggested?
Do you want me to close this and make a PR in that repo instead? Or would you like me to make an identical PR in that repo as well as this one? Also, should I reverse the order of the checks and throw an error like @Kefta suggested?
A PR in the other repo would work. I have a script to update DarkRP, so I'll take care of that 👍
I also agree with @kefta, in that you should throw an error instead of silently moving on. The current error might not be as clear to people, but at least it shows something is wrong. With this PR, the code pretends that all is ok. This can lead people to think that the MySQL connection is working, and that all is well, even though everything is still being done with the SQLite file.
An error should explain what's wrong and more importantly, what a user can do to fix it. Something like:
The preferred module for MySQL is selected to be tmysql4. However, tmysql4 does not appear to be installed. Please either:
- Install tmysql. It can be obtained from https://github.com/SuperiorServers/gm_tmysql4.
- Select MySQLOO as the preferred module for MySQL. This module appears to be installed.
Due to this error, MySQL is disabled. This means that SQLite is used instead to store data.
This can indeed be an ErrorNoHalt. You can throw a similar error for MySQLOO. Judging by the code, we're already past the check that either is installed. This error would just be that the wrong one is selected.
In the same vein, the require silently goes past the problem where the wrong one is selected. That should be fixed as well.
@FPtje What do you think of the changes I just made? If they look good I will make the same changes on the mysqlite repo.
Looks good for a PR on the other repo. I'll take a closer look later 👍
Made the PR in the other repo, I think it would be easiest for you to simply merge both since they have the same code, but you can do whatever you like. Let me know if you want any additional changes.
With the PR in MySQLite, I'm closing this PR.
See https://github.com/FPtje/MySQLite/pull/16