[CI] Working round PHPStan dependencies

[TomasVotruba]

CI check for #1917 (comment)

[TomasVotruba]

Should be part of website build script or Dockerfile, not core code composer.json

[bobdenotter]

I'm not sure about this specific change. As a developer, I use bolt/core directly when working on Bolt. Did you remove it because it's problematic, or just as part of cleaning up?

[TomasVotruba]

Understood.

This part it problematic, as it requires whole project to be runnable, before even running codding standard or PHPStan or PHP linter in CI.

So if code is broken, the composer install will fail here and no CI tool will help us.

[bobdenotter]

Is that really a problem, though? If the code is broken, CI should fail, and we'd need to fix that regardless.

[TomasVotruba]

It is, because it fails on composer install and not on one of 8 jobs we have now. It's false positive actually. The point of CI is to have the minimal scope the error appears in. That's why the PHPStan should be decouple from project's container.

In a correct project setup, this shoud be part of build script, Docker, npm run etc. Same way assets build is.

[bobdenotter]

Several are breaking with the following error 🤔

 PHP Fatal error:  Uncaught Symfony\Component\Dotenv\Exception\PathException: Unable to read the "/home/runner/work/core/core/bin/../.env" environment file. in /home/runner/work/core/core/vendor/symfony/dotenv/Dotenv.php:565

[TomasVotruba]

That's the problem I got at first place. The .env file should not be required to install dependencies. Only after

This the issue stacking I talked about. --no-scripts probably disables some process outside composer.json that some package rely on by convention.

[bobdenotter]

Right, it's breaking here: https://github.com/bolt/core/blob/master/bin/console#L18-L23

if (!isset($_SERVER['APP_ENV'])) {
    if (!class_exists(Dotenv::class)) {
        throw new \RuntimeException('APP_ENV environment variable is not defined. You need to define environment variables for configuration or add "symfony/dotenv" as a Composer dependency to load variables from a .env file.');
    }
    (new Dotenv())->load(__DIR__.'/../.env');
}

$_SERVER['APP_ENV'] isn't set, so it attempts to read .env, which doesn't exist.

In Travis the env settings are set from .env.dist by:

export $(grep -v '^#' .env.dist | xargs -d '\n')

Should the github workflow do something similar?

[TomasVotruba]

Why not have the file with name .env by default? It would remove this hidden dependency.

The convention is to have environment-based configs if needed:

• .env.prod
• .env.local
• .env.test

etc

See Symfony post about it: https://symfony.com/doc/4.1/configuration/dot-env-changes.html

[TomasVotruba]

I commited it, just to try it out. I don't think it will be enough

[TomasVotruba]

Travis now fails on another single random Behat tests. It seems unrelated to this PR.

Does it happen often with Behat? It seems not very useful to have random fails all the time 🥺

[bobdenotter]

Does it happen often with Behat? It seems not very useful to have random fails all the time

Yes, it does happen quite often, but is goes in waves. Some weeks it's more than others (this week is definitely a "more" week), and it also happens more often when we assume Travis is busy, like during working hours in USA time.

[TomasVotruba]

I see. That must be very frustrating for daily development of whole Bolt project.

After we migrate the basic stuff to Github Actions, maybe Travis will have less work to handle and handle it better. If not, I'll try look into it. CI must be flawless and reliable :)

[bobdenotter]

"Frustrating" is a bit too much, but it can surely be annoying.. We sometimes call it "the little Travis dance" 🤣

[I-Valchev]

Joining the composer.json related discussion, from here #1919

My 2 cents:

I totally get what @TomasVotruba says about decoupling. And gosh wouldn't I love the CI running smoother/faster 😍

If it's one or the other, faced with better CI setup vs better experience for contributors , I'd much rather have the latter. IMHO, when people want to contribute, we should make it as easy as possible, which means less steps to setup (so no extra setup.sh).

Contributors by and large will know how to clone/composer install, but they'll have to read to know about running a setup.sh script. We already know people don't always read that for projects they should use bolt/project and for contributing bolt/core 😞

[TomasVotruba]

I agree. What about best of both worlds? :)

• best developer experience
• clean CI

Have a single script that will be skipped if in CI.
There is a package for that:
https://github.com/OndraM/ci-detector

[bobdenotter]

I haven't looked at OndraM/ci-detector in detail yet, but that might just be what we're looking for!

Also, I agree with the both of you: We do want to make the threshold for developers who want to contribute on Bolt to be as low as possible. And, as silly as it may sound: people are terrible at reading instructions. So, if we could make it so that we have a pleasant developer experience and make it to CI runs smoothly, that'd be the absolute best! ^_^

[bobdenotter]

Not trying to rush you, but I was curious on what the status on this PR is. Can you add OndraM/ci-detector to make it behave how we wish, or are you waiting on something on our end? :-)

[TomasVotruba]

No problem :). I'm now focused on another project, so I'll look into it on Wednesday.

If you have time, feel free to try it. It's just adding 1 package to composer and custom PHP script that would handle the CI check first.

[TomasVotruba]

Surpassed by #1961