Update docker environment and CONTRIBUTING.md instructions

[ajgarlag]

Alternative to #56 updating current docker environment.

Fix #55

[chalasr]

@ajgarlag I'm divided. I would drop the docker setup, but since you updated it here I guess we can keep it if some find it useful. What's your preference?

[ajgarlag]

@chalasr I've decided to submit two pull requests so you can evaluate the pros and cons of each.

When I contributed the authorization_code grant to trikoder/oauth2-bundle, the docker environment was a great help mainly because the timecop extension was used for time-sensitive tests. Without the docker environment, maybe I would not have contributed the PR. But it has a main drawback: it requires docker as a development dependency (I know several PHP developers that are not comfortable with this tool).

In this project, I've not needed docker at all, and the contributing workflow was similar to other PHP projects. But @X-Coder264 already commented the advantages of keeping it.

The main problem I see with keeping the docker environment is that is not being used in GitHub actions. I think that if we keep the docker environment as the official workflow for contributing to the bundle, we should update GitHub actions to make use of it so we can ensure that the docker environment is working as expected.

[chalasr]

Oh, I missed Antonio's comment on your issue sorry. Let's keep it.

The main problem I see with keeping the docker environment is that is not being used in GitHub actions. I think that if we keep the docker environment as the official workflow for contributing to the bundle, we should update GitHub actions to make use of it so we can ensure that the docker environment is working as expected.

Agree 👍

[ajgarlag]

I've added two GitHub actions to check CS and execute tests using the docker environment.

Does the same statement from Antonio Pauletich's (I'm Antonio too) comment about not having PHP-CS-Fixer as part of the dev dependencies applies to Psalm? Should we move Psalm from composer.json to docker environment?

[chalasr]

(I'm Antonio too)

Haha right, let's use gh usernames then.

Should we move Psalm from composer.json to docker environment?

That makes sense to me.

[ajgarlag]

Ok, one last question. I think we can execute checking coding standards, running static analysis, and running unit tests sequentially in one GitHub action to avoid building the docker environment three times. WDYT?

[chalasr]

I like the fact they represent distinct checks on a PR so better keep duplicating (or reuse steps somehow?) IMO

[ajgarlag]

In that case, IMO the PR is ready for me. If it's OK for you, I'll squash the commits.

[chalasr]

@ajgarlag The added docker-* workflows can replace the existing (non docker) ones isn't it?
I don't think it's relevant to have the same job runs with and without docker. Let's just run everything on docker?

[ajgarlag]

@ajgarlag The added docker-* workflows can replace the existing (non docker) ones isn't it? I don't think it's relevant to have the same job runs with and without docker. Let's just run everything on docker?

Yes, but in that case, the code will be tested only with the versions defined in Dockerfile. So my previous proposition was to keep current GitHub actions that will test the code with the latest version of each tool, and to add a new docker.yml file to run checking coding standards, running static analysis, and running unit tests sequentially just to ensure that the docker environment is working. If this action fails, but the other one passes, then the Dockerfile should be updated.

[X-Coder264]

@ajgarlag The Dockerfile could be changed so that it fetches the latest versions of Composer/Flex/Xdebug/PHP CS fixer by default and then the same Docker file could be used to test across all supported PHP versions.

[ajgarlag]

@X-Coder264 Thanks, I'll try to update de Dockerfile.

[ajgarlag]

I've updated the Dockerfile to install always the latest version of development tools. The only build argument for the docker image is the PHP version.

WDYT?

[chalasr]

@ajgarlag Looks great. One thing, can we reduce the number of checks?
The coverage before this PR was good enough to me, it's ok not to test with low/high deps for all symfony/php versions.

[ajgarlag]

@chalasr After some vacation days, I've rebased and finished my work here. WDYT?

[chalasr]

Awesome!

[ajgarlag]

I've changed the 8.1-rc docker tag so the latest available version is used always

[chalasr]

Thank you @ajgarlag.

[ajgarlag]

@chalasr Off-topic: While coding this PR, I got a failed test that I cannot explain. See https://github.com/thephpleague/oauth2-server-bundle/runs/4177437425?check_suite_focus=true. Maybe a time sensitive assertion?