Refactor using SlimFramework and adhere to PSR-1, PSR-2 and PSR-4

r15ch13 commented 6 years ago

Hi, I created a refactored version of your code using the SlimFramework.

Changes:

It uses .env files for the basic configuration. This makes it easier to host this project in a Docker Container
The code should now mostly adhere to the PSR-1, PSR-2 and PSR-4 standards
Routes can be added to src/routes.php
Commands can be added to src/commands.php

Use it:

Install composer https://getcomposer.org/doc/00-intro.md#installation-linux-unix-osx
Run composer install
Run cp .env.sample .env and change database settings
Run composer migrate
Run composer dev

Requirements:

Routes:

POST http://localhost/upload
POST http://localhost/activate

Commands:

php ./bin/cli.php migrate (or composer run migrate)
php ./bin/cli.php cleanup (or composer run cleanup)
php -S 127.0.0.1:8080 -t ./public (or composer run dev)

exploide commented 6 years ago

Hi. Wow, I haven't expected such a PR. At first, I want to thank you for taking a look on this project and spending such an effort in implementing your proposed changes!

The thing is, I would have preferred you had opened an issue beforehand to talk about your plans. Let me try to explain why:

The README.md states that

Programming language, repository structure and other design decisions are based on the requirements imposed by the intended hosting infrastructure for StreetComplete.

This includes that there deliberately is no framework (like slim) or toolchain (like composer). This is because a primary design goal of this project is to be as simple and maintenance free as possible. The software is primarily intended to run on the webspace of @westnordost and as far as I know, he does not want to spend time in maintaining the software once it got successfully deployed. Introducing dependencies, especially a framework requires to constantly monitor the dependencies for updates, updating the composer file and performing the actual update on the production server. Otherwise we will most likely end up with unpatched security vulnerabilities.

Additionally, the folder structure was flat so it is most easy to deploy by just cloning this repository. The webspace provider does not allow to configure document roots explicitly to my knowledge.

I even don't know if @westnordost has composer at hand there.

It's clearly not optimal how beautiful the software is in it's current state. Both, in terms of coding standards (I do not write software in PHP on a regular basis) and in repository structure (I explained why). I also fully agree it's bad practice to tailor a project to very specific deployment needs. Though, most design decisions have a reasoning behind it.

Without all these requirements, I would have created a Python project, probably using Django or Flask, with a reasonable structure and the coding standards I'm used to adhere to. But this is not the case for this project.

Besides, it is a very small project with only 400 LOC, so using a framework here only has little advantage, as it will not evolve into something huge.

Again, thank you for investing time into this but I think the changes are not beneficial for the intended purpose of this project :/

That's why I would have preferred discussing this in an issue first.

r15ch13 commented 6 years ago

Hi, I didn't know about these tight deployment requirements because I skipped reading the README and the discussion on the StreetComplete project. You're right, discussing this would have been better. I was just eager to improve the PHP Code at that moment. Not seeing at least PSR-1 in use makes me do such things. :smile:

It's no problem if this doesn't get used. I had fun :grin:

exploide commented 6 years ago

:grin: Thanks for your understanding. I guess I also would go crazy when seeing someone coding Python as bad as I do PHP :wink:

I don't know much about PSR, but if there are smaller improvements you would like to see and which are applicable to the minimalist project structure we have here, feel free to propose them.

westnordost commented 6 years ago

Interesting. So this is how modern PHP code looks like(?) I have to say, it's not more beautiful, but I like how it seems to be streamlined towards REST. I get how this all makes sense for an application based on PHP, but sc-photo-service is just a few scripts with a SQL-DB behind so the way @exploide implemented this is IMO the most straightforward to do.

Anyhow, PHP is the language you choose if it should run with zero configuration on any normal webspace. I do not think that this requirement is especially tight. If you have docker/virtualized servers, why on earth would you use PHP? Not trying to dis PHP here - it just escapes my mind. If one is outside the scripts-on-a-webspace world, there are certainly more swift solutions than PHP, aren't there? I do not have experience in that field myself.

r15ch13 commented 6 years ago

Yeah, for a few scripts this is okay. I was just assuming it could be more. Quick reaction without thinking about it. :smile: With "normal webspace" you mean FTP upload only? If a hoster provides command line access with GIT and PHP, then it would still be a quite easy deployment. Clone/Pull it, composer install and that's it.

For running it with docker I would have used NodeJS or so.

westnordost commented 6 years ago

Yup, with no shell access.

What about golang or some other modern language which is still typed?

r15ch13 commented 6 years ago

TypeScript? 😁

I don't have much experience with golang. Or "Python using Django or Flask" like @exploide mentioned.

Edit: https://thenewstack.io/make-a-restful-json-api-go/ Starts like a typical NodeJS tutorial. 5 lines of code, webserver running :smile:

westnordost commented 6 years ago

Me neither, just curious.

streetcomplete / sc-photo-service