Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PDO php script implementation #486

Merged
merged 6 commits into from
Nov 20, 2023
Merged

Conversation

AlbusRex
Copy link
Contributor

I have tried to rewrite the current mysql.php to a more generic implementation using php's PDO features and I think I have succeeded.
This implementation has been tested with NGINX and a Postgre storage backend and has been accessed by chromium and firefox based browsers.
On the backend getting PDO to work is very easy. Just by installing the corresponding php database module the PDO driver will become available.
I have not had the opportunity to test a different database storage unfortunately.

Copy link
Member

@TBlueF TBlueF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice! Thank you for the efforts so far :)

I'll still need to test this on my end, but here are some things i noticed when reading over this quickly..

Additionally to the comments, i think naming the file sql.php instead of map_data_provider.php would be easier and more understandable for many users. If you agree, could you please rename the file to that instead? :)

BlueMapCommon/webapp/public/map_data_provider.php Outdated Show resolved Hide resolved
BlueMapCommon/webapp/public/map_data_provider.php Outdated Show resolved Hide resolved
BlueMapCommon/webapp/public/map_data_provider.php Outdated Show resolved Hide resolved
BlueMapCommon/webapp/public/map_data_provider.php Outdated Show resolved Hide resolved
Copy link
Contributor Author

@AlbusRex AlbusRex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented the changes that were suggested.
I am a bit slow on the update since I am new to this workflow on github.

@AlbusRex AlbusRex requested a review from TBlueF October 13, 2023 07:29
@TBlueF
Copy link
Member

TBlueF commented Oct 13, 2023

Thanks for the changes, looks good now :)
I'll test it as soon as i find some time and if everything works as expected it will be merged 👍

@TBlueF
Copy link
Member

TBlueF commented Nov 18, 2023

@AlbusRex Sorry for the very long time it took me to test this..

I had to change a couple of things to make this work on my end with mysql.
Can you please confirm that my changes didn't break anything, and it still works with PostgreSQL ? :)

@AlbusRex
Copy link
Contributor Author

I have tested the changes and everything works as intended. I suspect we had one of those "by value vs by reference" kind of problems.

@TBlueF TBlueF merged commit 0bad1b6 into BlueMap-Minecraft:master Nov 20, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants