Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial functionality and tests #1

Merged
merged 15 commits into from
Jun 28, 2022
26 changes: 26 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#
# Exclude these files from release archives.
#
# This will also make the files unavailable when using Composer with `--prefer-dist`.
# If you develop using Composer, use `--prefer-source`.
#
# Via WPCS.
#
/phpcs.xml export-ignore
/phpunit.xml export-ignore
/.github export-ignore
/tests export-ignore

#
# Auto detect text files and perform LF normalization.
#
# http://davidlaing.com/2012/09/19/customise-your-gitattributes-to-become-a-git-ninja/
#
* text=auto

#
# The above will handle all files not found below.
#
*.md text
*.php text
*.inc text
21 changes: 21 additions & 0 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
## Summary

As titled.

## Notes for reviewers

None.

## Changelog entries

### Added

### Changed

### Deprecated

### Removed

### Fixed

### Security
31 changes: 31 additions & 0 deletions .github/workflows/composer.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
name: Composer Validation

on:
pull_request:

jobs:
validate:
runs-on: ubuntu-latest
strategy:
fail-fast: true
matrix:
php: [8.1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No 8.0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my knowledge, Composer config validation depend on the PHP version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah: Composer config validation doesn't depend on the PHP version

steps:
- name: Cancel previous runs of this workflow (pull requests only)
if: ${{ github.event_name == 'pull_request' }}
uses: styfle/[email protected]
with:
access_token: ${{ github.token }}

- name: Check out code
uses: actions/checkout@v3

- name: Set up PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
tools: composer:v2
coverage: none

- name: Run Composer config validation
run: composer validate --strict
41 changes: 41 additions & 0 deletions .github/workflows/cs.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
name: Coding Standards

on:
pull_request:

jobs:
cs:
runs-on: ubuntu-latest
strategy:
fail-fast: true
matrix:
php: [7.4]
montchr marked this conversation as resolved.
Show resolved Hide resolved
steps:
- name: Cancel previous runs of this workflow (pull requests only)
if: ${{ github.event_name == 'pull_request' }}
uses: styfle/[email protected]
with:
access_token: ${{ github.token }}

- name: Check out code
uses: actions/checkout@v3

- name: Set up PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
tools: composer:v2
coverage: none

- name: Install composer dependencies
uses: nick-invision/retry@v1
with:
timeout_minutes: 5
max_attempts: 5
command: composer install

- name: Run PHPCS
run: composer run-script phpcs

- name: Run PHP-CS-Fixer
run: vendor/bin/php-cs-fixer fix -v --dry-run --stop-on-violation --using-cache=no --allow-risky=yes
56 changes: 56 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
name: Tests

on:
pull_request:

jobs:
phpunit:
runs-on: ubuntu-latest
strategy:
fail-fast: true
matrix:
php: [8.1, 8.0, 7.4]
wp_version: ["latest"]
can_fail: [false]
multisite: [0,1]
montchr marked this conversation as resolved.
Show resolved Hide resolved
services:
mysql:
image: mysql:5.7
env:
MYSQL_ALLOW_EMPTY_PASSWORD: yes
ports:
- 3306:3306
options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3
options: |
--health-cmd="mysqladmin ping" \
--health-interval=10s \
--health-timeout=5s \
--health-retries=3

continue-on-error: ${{ matrix.can_fail }}

name: WordPress ${{ matrix.wp_version }} @ PHP ${{ matrix.php }} (WP_MULTISITE=${{ matrix.multisite }})
montchr marked this conversation as resolved.
Show resolved Hide resolved
env:
CACHEDIR: /tmp
WP_CORE_DIR: /tmp/wordpress
WP_VERSION: ${{ matrix.wp_version }}
montchr marked this conversation as resolved.
Show resolved Hide resolved
WP_MULTISITE: ${{ matrix.multisite }}
WP_DB_HOST: 127.0.0.1
WP_DB_USER: root
WP_DB_PASSWORD: '""'

steps:
- name: Check out code
uses: actions/checkout@v3

- name: Set up PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
extensions: json, mysqli, curl, dom, exif, fileinfo, hash, imagick, mbstring, openssl, pcre, xml, zip
tools: composer:v2
coverage: none

- name: Install Composer dependencies
uses: nick-invision/retry@v2
with:
timeout_minutes: 5
max_attempts: 5
command: composer install

- name: Run PHPUnit
run: composer run-script phpunit
5 changes: 5 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
vendor
composer.lock
.php_cs.cache
.phpunit.result.cache
.php-cs-fixer.cache
34 changes: 34 additions & 0 deletions .php-cs-fixer.dist.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php
/**
* PHP-CS-Fixer configuration
*
* (c) Alley <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*
* @package wp-find-one
Comment on lines +3 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend including an SPDX expression containing this basic metadata as a quick human-and-machine-readable reference.

https://github.com/david-a-wheeler/spdx-tutorial#spdx-license-expressions-in-source-code-files
https://spdx.github.io/spdx-spec/

Suggested change
* PHP-CS-Fixer configuration
*
* (c) Alley <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*
* @package wp-find-one
* Find One: PHP-CS-Fixer configuration
*
* Copyright (c) 2022, Alley and the Find One contributors
* SPDX-License-Identifier: GPL-2.0-or-later
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*
* @package wp-find-one
Suggested change
* PHP-CS-Fixer configuration
*
* (c) Alley <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*
* @package wp-find-one
* PHP-CS-Fixer configuration
*
* (c) Alley <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*
* @package wp-find-one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This construction of comment is modeled off of file headers of other PHP libraries, with a few tweaks to match the WordPress core standards for file headers. For example:

So I'm inclined to leave off an SPDX header for consistency with those.

As a personal aside, while the SPDX header might be machine-readable, it doesn't come off to me as super human-readable. If I don't know what SPDX is (I had to Google it), the line won't mean anything to me. Moreover, it seems redundant: Why include both the license name and the invitation to view LICENSE in the same block?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This construction of comment is modeled off of file headers of other PHP libraries, with a few tweaks to match the WordPress core standards for file headers.

So I'm inclined to leave off an SPDX header for consistency with those.

Consistency bolsters familiarity, but without an eye towards the distinctly separate principle of actual documented best practices, consistency reinforces a copy-pasta status quo.

I know we can do better. To be quite honest, I think those projects do a terrible job of explaining their users' rights in those headers. They might as well not include a header at all if they're going to point to a file somewhere else. Which, by the way, I think is totally fine! It's the "I won't tell you the license, go find it" that I find frustrating. How do you know they didn't copy that "look somewhere else" wording without asking whether it was a good idea?

Perhaps that works out for those respected monolithic projects whose components are unlikely to be distributed in fragments, but when the library primarily consists of a single file which can easily be lifted into another project, I would argue that we should expect that the license will never be recirculated in those cases, so the message about some other file becomes useless upon later reference.

While the SPDX header might be machine-readable, it doesn't come off to me as super human-readable.

While GPL-2.0-or-later is a strict string, its meaning is obvious to many. If someone is unfamiliar, the benefit of a strict string is that it's easily referenced in a search engine query e.g. license GPL-2.0-or-later.

If I don't know what SPDX is (I had to Google it), the line won't mean anything to me.

That seems to me like an exaggeration.

Sometimes we know a thing before we know its name. To me, the addition of the name SPDX provided a name for a set of rules I never thought about before but which all of us, as developers, use all the time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without an eye towards the distinctly separate principle of actual documented best practices, consistency reinforces a copy-pasta status quo

Where is it documented that SPDX is a best practice, other than by the SPDX workgroup itself? Got links? 😄

when the library primarily consists of a single file which can easily be lifted into another project, I would argue that we should expect that the license will never be recirculated in those cases, so the message about some other file becomes useless upon later reference

This convinces me.

If I don't know what SPDX is (I had to Google it), the line won't mean anything to me.

I'm not exaggerating. Yes, I can make some guesses based on familiar strings, but I wouldn't want to bet the compliance of my code with a license on it. "I won't tell you the license, go find it" is curt but pretty unambiguous in telling you "the answer is not here."

In any case, I'm not quite prepared to make the call by myself to switch to SPDX, so I'll bring this discussion to the open source working group to find an improvement, SPDX or otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Press release: SPDX Becomes Internationally Recognized Standard for Software Bill of Materials - Linux Foundation

Backed by many of the world’s largest companies for more than a decade, SPDX formally becomes an internationally recognized ISO/IEC JTC 1 standard during a transformational time for software and supply chain security

Also see: Solving License Compliance at the Source: Adding SPDX License IDs - Linux Foundation

SPDX identifiers are included in the Wikipedia sidebars for software licenses:

Specifically in the context of WordPress:

In terms of source file headers specifically… I think that's certainly more subjective, since file headers can lend a bit of character to a project. But at the very least I would expect a file header, if there is one, would include the name of the license. Rather than winging it, we have the official license naming schemes provided by SPDX to lean on. The "Solving License Compliance at the Source" article linked above highlights some early examples.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the links!

The content of source file header, I thought, was the heart of the discussion 😄 But using an SPDX identifier for the license if the header includes the name of the license certainly makes sense to me.

*/

$finder = PhpCsFixer\Finder::create()->in(
[
__DIR__ . '/src/',
__DIR__ . '/tests/',
]
);

$config = new PhpCsFixer\Config();
$config->setRules(
[
'@PHP81Migration' => true,
'method_argument_space' => false, // Enabled by '@PHP81Migration' but generates invalid spacing for WordPress.
dlh01 marked this conversation as resolved.
Show resolved Hide resolved

'native_constant_invocation' => true,
'native_function_casing' => true,
'native_function_invocation' => true,
'native_function_type_declaration_casing' => true,
]
);
$config->setFinder($finder);

return $config;
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Changelog

This library adheres to [Semantic Versioning](https://semver.org/) and [Keep a CHANGELOG](https://keepachangelog.com/en/1.0.0/).

## 1.0.0

Initial release.
Loading