updated used versions and unified written php version style #252
Conversation
|
Damn that is a long list of psalm errors. Can I ask somebody to check on that? |
… cannot work properly without it.
|
This is way better. Ready for review @transistive |
| @@ -74,6 +74,9 @@ | |||
| "allow-plugins": { | |||
| "composer/package-versions-deprecated": true, | |||
| "php-http/discovery": true | |||
| }, | |||
| "platform": { | |||
There was a problem hiding this comment.
this is great! I never knew about this
docker-compose-neo4j-4.yml
Outdated
| @@ -47,7 +47,7 @@ services: | |||
| context: . | |||
| dockerfile: Dockerfile | |||
| args: | |||
| PHP_VERSION: "${PHP_VERSION-8.1}" | |||
| PHP_VERSION: "${PHP_VERSION}" | |||
There was a problem hiding this comment.
why would you remove the default version here? maybe use the more concrete solution you are proposing here and go for: ${PHP_VERSION-8.1.31}?
There was a problem hiding this comment.
This line defines php version based on provided variable from github workflow. But the name is different here and in workflow, therefore this is more like bugfix.
There was a problem hiding this comment.
The ${PHP_VERSION} is a placeholder for an environment variable.
In the build section, ${PHP_VERSION} will be replaced with the actual PHP version specified in an environment file (.env) or passed during the Docker build process. This allows for flexible configuration of the PHP version without hardcoding it into the Docker Compose file.
There was a problem hiding this comment.
You create that variable within workflow
There was a problem hiding this comment.
Thanks Michal, but we also use it for local development running docker compose up, in which case it is still very useful to have a default variable. I'll revert that section and merge it now
There was a problem hiding this comment.
from what i understand if there is ${PHP_VERSION-8.1} it needs env variable with this name. if the env variable is missing it will leave requested php version empty, therefore it propably use some default.
if what I did is fix I get that it can broke some stuff which relayed on this bug.
There was a problem hiding this comment.
This does not only get used in the CI. We also use it locally. Anyone working on the project is advised to do a simple docker compose up in their terminal and it will set up an entire working environment and cluster for testing, as well as a correct PHP environment containing the minimum requirements for the library.
docker compose has 4 features interacting when you run docker compose up with the environment variables.
- if there is a matching value in the .env file, it will interpolate the variables found there in the docker-compose configuration
- if not, it will find try to find a matching value in the shell environment to interpolate it (this is the case in the CI as you have pointed out).
- if not, it will look if there is a default version provided in the configuration declaration itself (this is the case in most local development situations)
- finally if none is found it will provide a warning none has been found
If we take away option 3, most developers will immediatly have to deal the with warning produced in option 4. This lowers the barrier to entry significantly, because now we have to explain them docker concepts or make them do an extra environment configuration step.
In short I don't see an advantage to remove the default PHP value, but I do know from experience it will have disadvantages to the dev experience of most people starting out.
There was a problem hiding this comment.
I didn't know docker has it this complex when dealing with it. 👍
There was a problem hiding this comment.
I like the stability of having concrete versions. Good stuff!
I also never knew about "platform" in composer.json. The one part I'm confused about is why we removed the default PHP_VERSION for docker compose? Now if someone wants to run it locally they have to explicitly add it either as a shell variable or in their .env file. What is your input @stefanak-michal ? otherwise we should consider reintroducing the default PHP_VERSION but with the more precise one
No description provided.