GH Actions: minor tweaks

• Be more selective about which packages are removed in CI for incompatibility cross-version.
• As PHPUnit is no longer a root requirement, it doesn't need to be removed anymore, but the config - platform setting needs to be removed instead to allow for updating the test dependencies.

Minor doc update

Remove two small code blocks

... which were obviously only in place to support PHP 5.6.

Revert "Maybe"

This reverts commit 64d4fb598dfadd2b8220dab78510d0a237f1042a.

Composer: update to YoastCS 2.3.1

The YoastCS 2.3.1 release sets the minimum supported PHP version for the PHPCompatibility checker to 7.2, which will allow for making code changes which are possible now support for PHP < 7.2 is being dropped.

Ref: https://github.com/Yoast/yoastcs/releases/tag/2.3.1

Composer: update the PHPUnit related dependencies

This commit:

• Removes the hard-coded PHPUnit dependency from the composer.json file (which was only there to keep the locked version compatible with PHP 5.6).
• And updates the locked test dependencies to versions compatible with PHP 7.2 (the new minimum supported PHP version).

Effectively, this updates the locked PHPUnit version from version 5.7.27 to version 8.5.33.

Composer: update miscellaneous test dependencies

Patchwork, an underlying dependency for BrainMonkey, which is a dependency of WP Test Utils, has released new versions.

This updates the Patchwork dependency to the latest release.

Ref: https://github.com/antecedent/patchwork/releases

Composer: remove explicit dependencies on the Symfony polyfill libs

The Symfony polyfill libs are not our dependency, but a dependency of Guzzle via the Oauth2 library.

The only reason these were in the require-dev section, was to prevent these sub-dependencies from being updated to a version incompatible with PHP 5.6, which due to the DI container code, couldn't previously be safeguarded via a config - platform setting.

As 1) compatibility with PHP 5.6 is no longer an issue and 2) in the new reality, there will be an appropriate config - platform setting, the explicit requirements for these polyfill libs can now be removed and these dependencies (including their sub-dependencies) can now be updated, or in some cases, removed.

Drop compatibility with PHP 5.6, 7.0 and 7.1

Improvements

Set 7.2.5

Set 7.2.5

lift dependency of PHP version from ^7.2 to ^8.0 (where T_MATCH is introduced)

That sound completely unnecessary to me. PHP_CodeSniffer backfills the T_MATCH token since PHPCS 3.6.0 (all the way back to PHP 5.4). So the only thing needed would be to raise the minimum PHPCS version to 3.6.0 (or later).

Applied suggested changes to phpunit.xml using vendor/bin/phpunit tests --migrate-configuration

Not a good idea as that will prevent the tests from running on PHPUnit < 9.3 and with a PHP minimum of PHP 7.2, PHPUnit 8.x is still needed.

I'm not sure why I can't just merge this, we didn't have auto-merge before 😕

Auto-merge is a feature which has been around for a while now, but the reason this can't be merged is the failing check on PHP 5.5 against PHPCS 3.7.1 (which I couldn't reproduce locally).

I've just restarted the build, who knows it may have been a fluke (update: it was not :x:).

So to get round this, there are a couple of options:

• Raising the minimum PHPCS version to 3.7.2 is the simplest and as we've raised the minimum PHPCS version already and this is a major, this should be fine.
• Alternatively, we could decide to drop support for PHP < 5.6 to be in line with WP (but no longer in line with PHPCS),
• Or if someone is willing to spend significant time on this, someone could try to reproduce the failure and debug it.

Opinions ?

FYI: it's not even just anonymous classes... looks like named classes and other closed structures can all be declared within a closure: https://3v4l.org/Zb22K

The error message for the NoEmptyStrings error code has been updated to be more informative. Old: Strings should have translatable content New: The $%s text string should have translatable content. Found: %s => This may read a little awkward for the $text parameter. Suggestions for further improvement welcome.

Maybe:

The content of the text string $%s should be translatable. Found: %s

Thanks for the suggestion, except I'm not sure if that makes things any clearer - "yes, but it is translatable, I'm using a translation function". I believe it's important for the "translatable content" phrasing to stay to make it clear what the message is about.

@totten Thanks for reporting this. I have confirmed the issue and lined up a fix, which will be pulled & merged before the 10.0.0 release.

@nikolowry I'm not aware of any such sniffs, though I suppose a sister-sniff could be created, though I'd suggest to then only enforce alternative syntax when when there is inline HTML within the control structure.

Maybe open an issue to discuss this further ?

PHP 8.1 | gensalt_blowfish(): prevent deprecation notice

The PHP chr() function expects an integer as the input parameter, but the output of ord('0') + $this->iteration_count_log2 / 10 might be a floating point number.

While PHP type juggles between floats and integers, the number will likely loose precision when going from float to integer.

As of PHP 8.1, these type of "implicit float to int" conversions will throw a deprecation notice when precision is lost.

As this loss of precision is not an issue for this code, this small change make the type juggling explicit instead of implicit, which gets rid of the deprecation notice.

Ref:

• https://www.php.net/manual/en/function.chr.php
• https://www.php.net/manual/en/migration81.deprecated.php#migration81.deprecated.core.implicit-float-conversion

CI: add automated test runs via GH Actions

This adds a workflow to automatically run the tests against PHP 5.3 - 8.3 on all pushes and pull requests to this repo.

Notes:

• The test run against PHP 8.3 is allowed to fail (for now) as PHP 8.3 is not stable yet.
• As the setup-php action runner does not support PHP 5.0, 5.1 and 5.2, the tests are not run against those PHP versions.

The commit includes a small change to the test.php script to send an exit code depending on whether the test run succeeded or failed. This is necessary to allow GH Actions to mark a build as failed.

Move source files to src subdirectory

This will make it more straight-forward to set up autoloading with Composer and as the Package is not published as a "Package manager ready" package prior to this, IMO this directory layout change not should be seen as a breaking change.

Move test file to tests subdirectory

This allows for all test related files to be excluded from git archives in one go and will also allow (in a potential future) for additional test scripts/classes to be added and test autoloading via Composer to be set up.

Add composer.json configuration file

A composer.json file defines the project specifications for the currently most commonly used package manager in the PHP world.

Aside from the obvious:

• I've set the license to the Creative Commons Public Domain Dedication and Certification license, which is the closest I could find to an official "public domain" license.
• I've added an autoload section to allow dependents on the package to use the Composer generated autoload file to load the PassworfHash class (instead of having to manually require the file).
• I've added a script to make it self-evident to potential contributors how to run the tests.

Running composer install will generate a composer.lock file and create the autoload files in a vendor directory. If the project would have dependencies itself, those would also be installed in the vendor directory.

• As a general best-practice it is recommended not to commit the vendor directory to git.
• Whether or not to commit the composer.lock file is highly dependent on the package type. For a generic library package which supports a wide range of PHP versions, I would recommend not to commit the composer.lock file.

With the above in mind, I'm also adding a .gitignore file to prevent these files from being accidentally committed to the repo.

Add a .gitattributes file

GitHub/Git allows for creating package archives as zip files. Those archives are also used by Packagist.

It is generally considered good practice for those package to only contain source files. Tests, CI related files and tooling configuration files should be excluded from those archives to keep them lean.

This .gitattributes files does exactly that and prevents files which are not needed by dependencies from being included in the archives.

As a bonus, line ending normalization has been added as well.

CS: remove PHP close tags

As discussed in 3:

The ?> at the end of a file is optional and nowadays, the consensus is to leave it out as it will prevent typical issues with "headers already sent" due to the whitespace at the end of the file being interpreted as HTML whitespace and sent to the server.

Refs:

• https://www.php.net/manual/en/language.basic-syntax.phptags.php
• https://www.php-fig.org/psr/psr-12/#22-files

If a file contains only PHP code, it is preferable to omit the PHP closing tag at the end of the file. This prevents accidental whitespace or new lines being added after the PHP closing tag, which may cause unwanted effects because PHP will start output buffering when there is no intention from the programmer to send any output at that point in the script.

(From the PHP manual)

CS: use curly braces for all control structures

Control structures without curly braces are quite error prone as they will only apply to the first PHP statement after the condition. This means that a second statement - even when indented the same - will be run unconditionally, but it also means that anyone working with the code needs a really good understanding of what a "statement" is in PHP.

In other words, it's best practice by now to always use curly braces with control structures.

Ref:

• PSR12: https://www.php-fig.org/psr/psr-12/#5-control-structures

  The body of each structure MUST be enclosed by braces. This standardizes how the structures look and reduces the likelihood of introducing errors as new lines get added to the body.

Yes, I was thinking to release a 0.5.2 with just the #2 fix as well

In hindsight, we should have updated the source code comment to say 0.5.2 in that commit, or at least before adding more commits. Now we'd need a branch for that, or release 0.5.2 that doesn't exactly match any git commit (would differ in that comment only). Well, or we could rewrite history, assuming that very few people pulled this repo in these two days.

Considering there are only 7 forks and only one of those is up to date (mine), I'd say: go for the history rewrite and I'll sync my fork.

CI: add automated test runs via GH Actions

This adds a workflow to automatically run the tests against PHP 5.3 - 8.3 on all pushes and pull requests to this repo.

Notes:

• The test run against PHP 8.3 is allowed to fail (for now) as PHP 8.3 is not stable yet.
• As the setup-php action runner does not support PHP 5.0, 5.1 and 5.2, the tests are not run against those PHP versions.

The commit includes a small change to the test.php script to send an exit code depending on whether the test run succeeded or failed. This is necessary to allow GH Actions to mark a build as failed.

Move source files to src subdirectory

This will make it more straight-forward to set up autoloading with Composer and as the Package is not published as a "Package manager ready" package prior to this, IMO this directory layout change not should be seen as a breaking change.

Move test file to tests subdirectory

This allows for all test related files to be excluded from git archives in one go and will also allow (in a potential future) for additional test scripts/classes to be added and test autoloading via Composer to be set up.

Add composer.json configuration file

A composer.json file defines the project specifications for the currently most commonly used package manager in the PHP world.

Aside from the obvious:

• I've set the license to the Creative Commons Public Domain Dedication and Certification license, which is the closest I could find to an official "public domain" license.
• I've added an autoload section to allow dependents on the package to use the Composer generated autoload file to load the PassworfHash class (instead of having to manually require the file).
• I've added a script to make it self-evident to potential contributors how to run the tests.

Running composer install will generate a composer.lock file and create the autoload files in a vendor directory. If the project would have dependencies itself, those would also be installed in the vendor directory.

• As a general best-practice it is recommended not to commit the vendor directory to git.
• Whether or not to commit the composer.lock file is highly dependent on the package type. For a generic library package which supports a wide range of PHP versions, I would recommend not to commit the composer.lock file.

With the above in mind, I'm also adding a .gitignore file to prevent these files from being accidentally committed to the repo.

Add a .gitattributes file

GitHub/Git allows for creating package archives as zip files. Those archives are also used by Packagist.

It is generally considered good practice for those package to only contain source files. Tests, CI related files and tooling configuration files should be excluded from those archives to keep them lean.

This .gitattributes files does exactly that and prevents files which are not needed by dependencies from being included in the archives.

As a bonus, line ending normalization has been added as well.

CS: remove PHP close tags

As discussed in 3:

The ?> at the end of a file is optional and nowadays, the consensus is to leave it out as it will prevent typical issues with "headers already sent" due to the whitespace at the end of the file being interpreted as HTML whitespace and sent to the server.

Refs:

• https://www.php.net/manual/en/language.basic-syntax.phptags.php
• https://www.php-fig.org/psr/psr-12/#22-files

If a file contains only PHP code, it is preferable to omit the PHP closing tag at the end of the file. This prevents accidental whitespace or new lines being added after the PHP closing tag, which may cause unwanted effects because PHP will start output buffering when there is no intention from the programmer to send any output at that point in the script.

(From the PHP manual)

CS: use curly braces for all control structures

Control structures without curly braces are quite error prone as they will only apply to the first PHP statement after the condition. This means that a second statement - even when indented the same - will be run unconditionally, but it also means that anyone working with the code needs a really good understanding of what a "statement" is in PHP.

In other words, it's best practice by now to always use curly braces with control structures.

Ref:

• PSR12: https://www.php-fig.org/psr/psr-12/#5-control-structures

  The body of each structure MUST be enclosed by braces. This standardizes how the structures look and reduces the likelihood of introducing errors as new lines get added to the body.

Are we planning to have a more comprehensive test suite? What would it add?

Not sure about the "we" or the "planning", but it is a suggestion I made in a previous comment and I would be happy to contribute to it.

What with the huge amount of changes, especially deprecations, over the last few years in PHP, I can imagine, it might also be interesting/worth exploring to expand the test suite so it can function as an early warning system for new issues which crop up in new PHP versions. Would you be open to that ?

Done.

CS: remove PHP close tags

As discussed in 3:

The ?> at the end of a file is optional and nowadays, the consensus is to leave it out as it will prevent typical issues with "headers already sent" due to the whitespace at the end of the file being interpreted as HTML whitespace and sent to the server.

Refs:

• https://www.php.net/manual/en/language.basic-syntax.phptags.php
• https://www.php-fig.org/psr/psr-12/#22-files

If a file contains only PHP code, it is preferable to omit the PHP closing tag at the end of the file. This prevents accidental whitespace or new lines being added after the PHP closing tag, which may cause unwanted effects because PHP will start output buffering when there is no intention from the programmer to send any output at that point in the script.

(From the PHP manual)

CS: use curly braces for all control structures

Control structures without curly braces are quite error prone as they will only apply to the first PHP statement after the condition. This means that a second statement - even when indented the same - will be run unconditionally, but it also means that anyone working with the code needs a really good understanding of what a "statement" is in PHP.

In other words, it's best practice by now to always use curly braces with control structures.

Ref:

• PSR12: https://www.php-fig.org/psr/psr-12/#5-control-structures

  The body of each structure MUST be enclosed by braces. This standardizes how the structures look and reduces the likelihood of introducing errors as new lines get added to the body.

I've just pulled another PR with some small CS improvements (as discussed in #3) - PR #5.

Well, I expressed my preference and convention so far, but I don't insist on it. I'll leave it up to you what to do about it going forward.

I adjusted the commit messages for PR #5 as they were presented to me during the rebase anyway, but I'm bound to be careless with that going forward.

Is phpass ready for a new release now, perhaps 0.6 or 1.0? Should we push a tag?

If I look at the history since the last release:

• My fix from #2 could go into a patch release as it's only a bug fix, so 0.5.2.
• This PR could be seen as a functional enhancement though, though it's not a code change, so could be either 0.5.2 or 0.6.0.

As for 0.6 vs 1.0 - I'd say 0.6 for now and wait with the 1.0 until there is a more comprehensive test suite ?

CS: remove PHP close tags

As discussed in #3:

The ?> at the end of a file is optional and nowadays, the consensus is to leave it out as it will prevent typical issues with "headers already send" due to the whitespace at the end of the file being interpreted as HTML whitespace and send to the server.

Refs:

• https://www.php.net/manual/en/language.basic-syntax.phptags.php
• https://www.php-fig.org/psr/psr-12/#22-files

If a file contains only PHP code, it is preferable to omit the PHP closing tag at the end of the file. This prevents accidental whitespace or new lines being added after the PHP closing tag, which may cause unwanted effects because PHP will start output buffering when there is no intention from the programmer to send any output at that point in the script.

(From the PHP manual)

CS: use curly braces for all control structures

Control structures without curly braces are quite error prone as they will only apply to the first PHP statement after the condition. This means that a second statement - even when indented the same - will be run unconditionally, but it also means that anyone working with the code needs a really good understanding of what a "statement" is in PHP.

In other words, it's best practice by now to always use curly braces with control structures.

Ref:

• PSR12: https://www.php-fig.org/psr/psr-12/#5-control-structures

  The body of each structure MUST be enclosed by braces. This standardizes how the structures look and reduces the likelihood of introducing errors as new lines get added to the body.

It sure is a problem if that would discourage you from contributing. Maybe I should just accept that phpass would become more of your project than mine from this point on, and then you do whatever in terms of conventions.

I'm not looking to become a maintainer of yet another package, though I sure would be happy to contribute and collaborate if we can figure out a way to make this point workable for both.

I think this is ready to merge. I do notice some typos in the commit messages (like "be be"), but I don't care that much.

Fixed that one ;-) And yes, I'm dyslectic, so typos and such happen to me, no matter how often I re-read commit messages... 🤷🏻‍♀️

Add a .gitattributes file

GitHub/Git allows for creating package archives as zip files. Those archives are also used by Packagist.

It is generally considered good practice for those package to only contain source files. Tests, CI related files and tooling configuration files should be excluded from those archives to keep them lean.

This .gitattributes files does exactly that and prevents files which are not needed by dependencies from being included in the archives.

As a bonus, line ending normalization has been added as well.