Add PHPCS, Clean up violations, Drop support for < PHP5.6

[timrourke]

This PR cleans up docblocks and makes a couple formatting changes based on PSR1, PSR2, and PEAR coding standards. (PEAR changes limited to nicer formatting of docblocks.)

EDIT: Updated this PR to also install PHPCS.

EDIT: Updated this PR to drop support for PHP versions lower than 5.6.

[timrourke]

PHPCS still reports some violations of PSR1 + PSR2, but because they involve changes to library code, I believe those changes should be deferred until the PR to add unit tests has landed: #70

Here are the outstanding violations:

FILE: /Users/tim/php-datadogstatsd/examples/expandedExample.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | WARNING | A file should declare new symbols (classes, functions,
   |         | constants, etc.) and cause no other side effects, or
   |         | it should execute logic with side effects, but should
   |         | not do both. The first symbol is defined on line 35
   |         | and the first side effect is on line 3.
----------------------------------------------------------------------


FILE: /Users/tim/php-datadogstatsd/src/BatchedDogStatsd.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 33 | ERROR | Method name "BatchedDogStatsd::flush_buffer" is not in
    |       | camel caps format
----------------------------------------------------------------------


FILE: /Users/tim/php-datadogstatsd/src/DogStatsd.php
----------------------------------------------------------------------
FOUND 3 ERRORS AND 1 WARNING AFFECTING 4 LINES
----------------------------------------------------------------------
  53 | WARNING | Property name "$__eventUrl" should not be prefixed
     |         | with an underscore to indicate visibility
 212 | ERROR   | Method name "DogStatsd::serialize_tags" is not in
     |         | camel caps format
 271 | ERROR   | Method name "DogStatsd::service_check" is not in
     |         | camel caps format
 301 | ERROR   | Method name "DogStatsd::escape_sc_message" is not in
     |         | camel caps format
----------------------------------------------------------------------

Time: 248ms; Memory: 8Mb

[timrourke]

Now that unit tests have landed, I fixed the private method names that were not properly camelCased. I also added comments to ignore non-conforming methods that are already part of the public API. Don't want to change those right now as that will break the client for current users.

[timrourke]

@masci any thoughts on this one?

[timrourke]

@masci @hush-hush PHPCS is failing to install for PHP 5.3. Downgrading PHPCS to 2.9.1 would allow us to support PHP5.1+, at the cost of using a much older version of this library, which doesn't support:

• xml configuration files
• comments to ignore specific sniffs

The upshot is that we can install PHPCS 2.9.1, but we won't be able to fail builds that don't pass the sniffs because the following methods should be camel-cased per PSR1 & PSR2, but are not, and are already public methods:

• BatchedDogStatsd::flush_buffer
• DogStatsd::service_check

How should we proceed? I do think it is important to establish some sanity around PHP syntax standards for the long haul.

Here's the output of PHPCS - you can see that it fails on the aforementioned method names:

$ vendor/bin/phpcs --standard=PSR1,PSR2 ./src

FILE: /Users/tim/php-datadogstatsd/src/BatchedDogStatsd.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 36 | ERROR | Method name "BatchedDogStatsd::flush_buffer" is not in
    |       | camel caps format
----------------------------------------------------------------------


FILE: /Users/tim/php-datadogstatsd/src/DogStatsd.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 278 | ERROR | Method name "DogStatsd::service_check" is not in camel
     |       | caps format
----------------------------------------------------------------------

Time: 125ms; Memory: 8Mb

[masci]

@timrourke what's the value of testing against 5.3 in your opinion?
If not worth it, we could maybe drop 5.3 from the test matrix. If it's important to keep it, we might fix the problematic signatures and release a major version of the library, I'm fine with both.

[timrourke]

@masci Hey there! I spent some time thinking about this. It feels sensible to drop official support for < PHP 5.6. That should allow us to update some things.

Should I just ping you on the PR to let you know about cutting a major release version after the fact?

[timrourke]

@masci I've updated this PR to drop support for versions of PHP lower than 5.6, which has allowed us to upgrade PHPUnit.

PLEASE NOTE that releasing these changes should include a meaningful version change (and some release notes) to let users know that two public method names have changed to be compliant with PSR1 + PSR2:

BatchedDogStatsd::flush_buffer is now BatchedDogStatsd::flushBuffer
DogStatsd::service_check is now DogStatsd::serviceCheck

[timrourke]

CC @hush-hush

[jian-wu]

@timrourke thanks for letting me know about this PR. 👍

I have 2 cents for you. This PR has quite a few things going on - PHPCS fixes, composer.json changes, README, travis config change, etc. I would recommend having them in their own PR's so that they are all atomic and easier for reviewers.

w/r/t PHPSC, how about having a line in travis.yml for style check. For instance, ./vendor/phpcs --extensions=php -p --standard=PSR2 ./src instead of the config file you have?

[timrourke]

@jian-wu those are an awesome 2 cents, i will split this up. got carried away! 🏃

[inverse]

Any update on this - really think we should drop anything below 5.6 now

[inverse]

@timrourke wanna split this? Def feel there are some awesome things going on in here.

Should we bundle linters/static analysis into the dev composer dependencies or is it worth leveraging something like: https://github.com/jakzal/phpqa which you can plug in on CI?

[tmotyl]

any progress?

[inverse]

@tmotyl I would say it's dead

[gh123man]

Hey all,

I'll be taking over maintaining this repo.
Happy to get this merged - but will request a few changes. First - we have already dropped support for older PHP versions and are now supporting 5.6+ so this PR needs a rebase.

Second - we should not rename the public methods even to comply with PHPCS as we risk breaking other users. I'd request that we explicitly ignore these public methods from PHPCS.

Unsure if @timrourke is still willing to update this very old PR - if not I'll make the proposed changes myself.

[tmotyl]

@gh123man what about marking them as deprecated now, and remove them in some next major version?

[inverse]

@gh123man any reason to support 5.6 still since even PHP 7.2 is EOL

https://www.php.net/supported-versions.php

[gh123man]

@tmotyl Marking them as deprecated sounds good for now.

@inverse I think it's best we leave the PHP version for now to best align with other Datadog repos.
Do please let me know if the PHP version is a blocking issue - I'd love to gather more insights.

[inverse]

@gh123man it's not a blocking issue - just wanted to raise the fact that anything below PHP 7.3 is EOL.

[gh123man]

Hi all - I've revived this branch here with some changes to bring it up to date.