Add GitHub actions

[jooola]

This adds some CI PHP code analyzing. It contains a huge set of reformatting change, And CI is still broken.

See https://github.com/jooola/api/pull/1/checks

[jooola]

I didn't format the config/_example.php file, and saw a few weird changes I didn't added to the formatting commits.

This is why the CI is still complaining about some unformatted files.

[WoLfulus]

This is really awesome and something I really wanted to do for quite some time, so thanks for this! I just think this will be really hard to make a proper review due to the amount of files changed.

I'll take some time later to do this and I'll probably add these into future branch too.

[jooola]

@WoLfulus This is why I tried to split the reformatting into multiple chunk. I admit I didn't completely review the changes for the src folder. It was too much at once. But I did check the 4 first formatting commits.

What could be done is that your make the formatting yourself and just cherry pick the CI files installation.

[WoLfulus]

I'll try to review this later, so I'll keep this open for now ok? I'd love to merge these on the current codebase too.

[rijkvanzanten]

Based on the fact that the script errors out at exactly 30MB memory use, I'm assuming we're hitting some sort of limit in GH Actions

[jooola]

@rijkvanzanten No the error is from the files not being correctly formatted. You can see the diff above the error line. Also the error code is 8, which means some files need some formatting.

https://github.com/FriendsOfPHP/PHP-CS-Fixer#exit-code

I don't think Github Actions would limit to 30Mo, it feels way not enough even for free plans.

I previously mentioned that I didn't format some files because I wasn't sure if the formatting was right so I left them unchanged.

[rijkvanzanten]

ah gotcha. just felt suspicious to me that it was exactly 30.00 😁

[jooola]

@rijkvanzanten @WoLfulus I mean I need your input on those failing files so we can fix this.

The first is the configuration file, should we exclude it from the PHP-cs-fixer formatter ?

The second problem comes from a file named public/extensions/custom/hashers/_CustomHasher.php and the class has been renamed to _CustomHasher to match the file name.

Not sure what to do.

-class CustomHasher implements \Directus\Hash\Hasher\HasherInterface
+class _CustomHasher implements \Directus\Hash\Hasher\HasherInterface

[rijkvanzanten]

The configuration file is an example for users, and the formatter is tripping over the way comments are structured. Can we add a rule that ignores all files prefixed by a _? Those are meant as example files in Directus and aren't actually used.

[jooola]

Now that Php-CS-Fixer passes, we have PHP stan complaining about some unimplemented methods from a parent abstract class. @rijkvanzanten What should we do ? This project doesn't even seem to use SQLite anyway right ?

Fatal error: Class Directus\Database\Schema\Sources\SQLiteSchema contains 26
abstract methods and must therefore be declared abstract or implement the remaining
methods (
Directus\Database\Schema\Sources\SchemaInterface::getConnection, 
Directus\Database\Schema\Sources\SchemaInterface::getSchemaName,
Directus\Database\Schema\Sources\SchemaInterface::getCollections,
...
) in
/home/runner/work/api/api/src/core/Directus/Database/Schema/Sources/SQLiteSchema.php
on line 414

[rijkvanzanten]

There was a good effort a while ago to support more SQL based databases in the past, this must be a remnant of that. I don't think it's being used for anything right now, but I'll have to defer to @bjgajjar to confirm that.

[jooola]

@rijkvanzanten Is :+1: an answer ?

[rijkvanzanten]

@bjgajjar ?

[jooola]

@rijkvanzanten I took the liberty of removing the file myself. This allowed me to add some configuration to PHPStan, which is now yelling a huge amount of errors that should be fixed.

See by yourself https://github.com/jooola/api/runs/378290711

We could either ignore some of those, or fix them. Reducing the analysis level is also something we could do to reduce some warnings. Or print all the warnings but make the test pass anyway using a || true.

[binal-7span]

@jooola - Thanks for this amazing PR.

I don't think it's being used for anything right now,

Agree. It's not being used anymore.

[rijkvanzanten]

@jooola should we apply this to the new laravel branch instead? Retroactively trying to fix all the warnings and errors in the current codebase is going to be a huge task

[jooola]

I can do that, but it looks like Wolfulu has already pushed the CI system in the laravel branch.

Le 9 janvier 2020 18:02:53 GMT+01:00, Rijk van Zanten [email protected] a écrit :

@jooola should we apply this to the new laravel branch instead? Retroactively trying to fix all the warnings and errors in the current codebase is going to be a huge task

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/directus/api/pull/1615#issuecomment-572657094

-- Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.