Define a coding style.

[tarekdj]

We have to define a coding style. This will provide a clean and uniform workflow. Code review will be easier.

http://www.php-fig.org/psr/psr-1/
http://www.php-fig.org/psr/psr-2/

Useful links but we can define our custom coding style.

[SaWey]

It is easiest to just follow PSR-1 and PSR-2 imo.
A big part of the code should already be compliant with these.

[bizmate]

I am building some interest in the PHPList OO rewrite. I commented a few days ago in the rest plugin and a OO rewrite I am sure will help to improve the API too.
phpList/phplist-plugin-restapi#33
I can create a PR with a Grumphp script to force PSR2 standards and run basic security checks.
Also it would make it really easy to introduce other checks, like Gerkin formatting etc. Interested?

[michield]

I must say I do have my doubts about these automated tools. With phpList3 we are signed up with StyleCI which uses phpcsfixer and it never parses correctly. I have even had it break the code and it was used to fix bugs in csfixer.

But yes, presumably it's a good idea to start with validating code on a new project and considering this project has been dormant for a while that may be a good idea.

[bizmate]

@michield grumphp is just a runner with the ability to perform a specific task, out of all the ones available, to achieve a specific quality check, fix, etc. You dont need to use csfixer if you dont want to, but also if previously you had any problems it might be for several different reasons.

I have never experienced such problems after I started following PSR2. A small note, at the beginning i didnt like the spaces VS tabs as I was using tabs years ago. However, I really value the use of a clearly defined standard. This would help the project have a clear guideline but of course it is a personal decision.

Without examples it is very difficult to evaluate but for simplicity forcing checking (not fixing) PSR-2 formatting in the code is a great way to keep it consistent. I would also usually like to have phpcsfixer enabled.

I have made a small effort with this PR #13 to show you how grumphp works. Pls check and let me know either here or on the PR. I can add MD documentation too if the main part of the PR is approved

[liayn]

Full ACK to what @bizmate said. Coding Guidelines (where code formatting is part of - aka PSR-2) are the bare minimum for reaching code quality. Keep in mind though, that PSR-2 (and implicitly PSR-1) still only has rules for a subset of things you should set out rules for.
A few examples:

• All binary operators MUST have spaces around, e.g. "mystring" . "myotherstring";
• Ternaries SHOULD NOT be nested.
• ::class SHOULD be used instead of strings to reference class names (PHP 5.5)
• There MUST NOT be trailing whitespace at the end of a line (also empty ones)
• Class property and method names MUST use camelCase style and MUST NOT begin with an underscore. (stricter than PSR-2 rules)
• ...

Other than that I can tell you that we migrated the whole TYPO3 project (3 million LOC) from our own code style to PSR-2 with various tools without bigger issues.

[michield]

Yes, I think it's fine. The main thing is how to keep it up-to-date. I will set up StyleCI on this repo, to see if that can help. From then on, we can ensure the coding style is maintained and also enforced.

You'd actually really want to add hooks to block commits that do not comply with the style, so that the responsibility is passed to each developer, instead of continuously parsing the code and correcting mistakes.

[michield]

I did

git clone https://github.com/bizmate/phpList.git
composer install
vendor/bin/grumphp run

but get

vendor/bin/grumphp run
bash: vendor/bin/grumphp: No such file or directory

did you forget to push something?

[bizmate]

Hi @michield

in my repo i used the branch PSR2 instead of master. So forgot to say to checkout that branch. https://github.com/bizmate/phpList/blob/PSR-2/composer.json#L25

If you have my repo locally you can checkout the branch and rerun composer install.

I have noticed you have merged the pull request so now you should be able to do the same in the phplist original repo

[bizmate]

About hooks to run grumphp before commit, this can be done also within grumphp itself if you want to.

[michield]

ah, yes, but now grumphp is grumpy, because it can't find phpcs. Can't that just be another dependency?

[bizmate]

that is explicitly included in composer.json,
https://github.com/phpList/phpList/blob/master/composer.json#L26
you should get it in the vendor folder when running composer install.

in the folder you should have all the command line tools

ls -la vendor/bin/
total 8
drwxrwxr-x  2 bizmate bizmate 4096 Jun 23 01:06 .
drwxrwxr-x 15 bizmate bizmate 4096 Jun 23 01:06 ..
lrwxrwxrwx  1 bizmate bizmate   33 Jun 23 00:54 composer -> ../composer/composer/bin/composer
lrwxrwxrwx  1 bizmate bizmate   28 Jun 23 00:54 grumphp -> ../phpro/grumphp/bin/grumphp
lrwxrwxrwx  1 bizmate bizmate   29 Jun 23 00:53 jsonlint -> ../seld/jsonlint/bin/jsonlint
lrwxrwxrwx  1 bizmate bizmate   43 Jun 23 01:06 phpcbf -> ../squizlabs/php_codesniffer/scripts/phpcbf
lrwxrwxrwx  1 bizmate bizmate   42 Jun 23 01:06 phpcs -> ../squizlabs/php_codesniffer/scripts/phpcs
lrwxrwxrwx  1 bizmate bizmate   39 Jun 23 00:55 phpunit -> ../phpunit/phpunit/composer/bin/phpunit
lrwxrwxrwx  1 bizmate bizmate   46 Jun 23 00:54 validate-json -> ../justinrainbow/json-schema/bin/validate-json

Also try composer update and see what happens. I should have removed the composer.lock from gitignore and checked it in to fix the dependencies versions

[michield]

I got this:

Skipped installation of bin scripts/phpcs for package squizlabs/php_codesniffer: name conflicts with an existing file
Skipped installation of bin scripts/phpcbf for package squizlabs/php_codesniffer: name conflicts with an existing file

G

I'll figure out what's wrong.

[oliverklee]

We'll use PSR-2 and check it with PHPCS. I'll take care of documenting this in the CONTRIBUTUNG file.