Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 120 additions & 0 deletions .github/scripts/phpcs-added-lines.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
#!/usr/bin/env python3
"""Report only the PHPCS findings that sit on lines a pull request added.

This theme predates the WordPress coding standards by roughly a decade, so its files carry
hundreds of pre-existing findings each. Running PHPCS over whole files would therefore fail
every pull request that touches legacy code, whether or not it made anything worse -- and a
job that is always red is a job nobody reads.

Filtering to added lines keeps the signal: a finding is reported when this change introduced
it, and the historical backlog stays a separate, deliberate piece of work.

Even scoped this way it is advisory, not a gate. Some of what it reports cannot reasonably
be fixed: the sniffs cannot see through `apply_filters()` to an escaper inside it, cannot
know that an interpolated ORDER BY direction was whitelisted, and object to the
`$before_title` / `$after_title` arguments that every WordPress widget emits. Failing a build
on those would mean scattering `phpcs:ignore` annotations through unrelated changes, so the
job reports and moves on. Pass --strict to exit non-zero instead.

Usage: phpcs-added-lines.py [--strict] <phpcs-report.json> <unified-diff>
"""

import collections
import json
import os
import re
import sys

HUNK = re.compile(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@')


def added_lines(diff_path):
"""Map each file to the set of line numbers this diff adds to it."""
added = collections.defaultdict(set)
current = None
new_line = 0
with open(diff_path, encoding='utf-8', errors='replace') as fh:
for line in fh:
if line.startswith('+++ '):
path = line[4:].strip()
current = None if path == '/dev/null' else re.sub(r'^b/', '', path)
continue
if line.startswith('@@'):
m = HUNK.match(line)
if m:
new_line = int(m.group(1))
continue
if current is None:
continue
if line.startswith('+'):
added[current].add(new_line)
new_line += 1
elif not line.startswith('-'):
# Context line. With --unified=0 these are rare, but count them anyway so
# the line numbering stays correct if the diff is ever generated with context.
new_line += 1
return added


def main():
args = [a for a in sys.argv[1:] if a != '--strict']
strict = '--strict' in sys.argv[1:]
if len(args) != 2:
print(__doc__, file=sys.stderr)
return 2

report_path, diff_path = args
if not os.path.exists(report_path) or os.path.getsize(report_path) == 0:
print('PHPCS produced no report; nothing to check.')
return 0

with open(report_path, encoding='utf-8', errors='replace') as fh:
report = json.load(fh)

added = added_lines(diff_path)

def to_repo_path(reported_path):
"""Map a path in the PHPCS report onto the repo-relative path used by the diff.

PHPCS reports absolute paths. Those normally sit under the working directory, but
not always -- so fall back to matching the longest path suffix that the diff knows
about, rather than silently treating every finding as untouched and reporting a
false all-clear.
"""
rel = os.path.relpath(reported_path, os.getcwd())
if not rel.startswith('..'):
return rel
norm = reported_path.replace(os.sep, '/')
for candidate in added:
if norm.endswith('/' + candidate) or norm == candidate:
return candidate
return rel

reported = 0
suppressed = 0
for abs_path, data in report.get('files', {}).items():
rel = to_repo_path(abs_path)
touched = added.get(rel, set())
for msg in data.get('messages', []):
if msg.get('type') != 'ERROR':
continue
if msg.get('line') in touched:
reported += 1
print('{}:{}:{} {}\n {}'.format(
rel, msg.get('line'), msg.get('column'), msg.get('source'), msg.get('message')))
else:
suppressed += 1

print()
if suppressed:
print('{} pre-existing finding(s) on untouched lines were not reported.'.format(suppressed))
if reported:
print('{} finding(s) on lines this change adds -- worth a look, but note that some '
'are unavoidable; see the header of this script.'.format(reported))
return 1 if strict else 0
print('No PHPCS findings on added lines.')
return 0


if __name__ == '__main__':
sys.exit(main())
104 changes: 104 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
name: CI

on:
push:
branches: [ master ]
pull_request:

permissions:
contents: read

jobs:
# Syntax check across every PHP version we claim to support. This is the cheapest useful
# signal in the repo: the theme's recent history is PHP 8 compatibility work, and until
# now nothing verified it on more than one interpreter.
lint-php:
name: php -l (PHP ${{ matrix.php }})
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
php: [ '7.4', '8.0', '8.1', '8.2', '8.3', '8.4' ]
steps:
- uses: actions/checkout@v4
- uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
coverage: none
- name: Lint every PHP file
run: |
find . -name '*.php' -not -path './vendor/*' -print0 \
| xargs -0 -n1 -P4 php -l

test:
name: PHPUnit (PHP ${{ matrix.php }})
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
php: [ '8.2', '8.3', '8.4' ]
steps:
- uses: actions/checkout@v4
- uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
coverage: none
- name: Install dependencies
run: composer update --no-interaction --prefer-dist
- name: Run tests
run: composer test

# Scoped to the LINES a pull request actually adds, not merely the files it touches.
#
# Scoping by file is not enough here. This theme predates the WordPress coding standards
# by about a decade, and its files carry hundreds of pre-existing findings apiece -- so a
# per-file run goes red the moment a PR edits a legacy file, regardless of whether the PR
# made anything worse. A job that is red by default is a job everyone learns to scroll
# past, which is worse than not having it.
#
# Reporting only on added lines means the job stays quiet unless someone introduces a new
# finding, and cleaning up the historical ones stays an independent choice.
#
# Advisory, not a gate. Some of what it reports cannot reasonably be fixed -- the sniffs
# cannot see through apply_filters() to an escaper inside it, cannot know an interpolated
# ORDER BY direction was whitelisted, and object to the $before_title/$after_title that
# every WordPress widget emits. Gating on those would mean scattering phpcs:ignore
# annotations through unrelated changes. Add --strict below to make it blocking.
phpcs:
name: PHPCS (advisory, changed lines)
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
- uses: shivammathur/setup-php@v2
with:
php-version: '8.3'
coverage: none
- name: Install dependencies
run: composer update --no-interaction --prefer-dist
- name: Report PHPCS findings on added lines
run: |
BASE="${{ github.event.pull_request.base.sha }}"
if [ -n "$BASE" ]; then
# Compare against the merge base, so a base branch that has moved on since the
# pull request opened does not drag unrelated commits into the diff.
BASE="$(git merge-base "$BASE" HEAD)"
else
BASE="$(git rev-parse HEAD~1 2>/dev/null || true)"
fi
if [ -z "$BASE" ]; then
echo "No base commit to compare against; skipping."
exit 0
fi
echo "Comparing against $BASE"
git diff --unified=0 "$BASE" HEAD -- '*.php' > /tmp/pr.diff
FILES="$(git diff --name-only --diff-filter=ACMR "$BASE" HEAD -- '*.php' | grep -v '^vendor/' || true)"
if [ -z "$FILES" ]; then
echo "No PHP files changed; nothing to check."
exit 0
fi
echo "$FILES" | tr '\n' ' '; echo
# -q so the progress output does not corrupt the JSON report.
echo "$FILES" | xargs ./vendor/bin/phpcs -q --report=json > /tmp/phpcs.json || true
python3 .github/scripts/phpcs-added-lines.py /tmp/phpcs.json /tmp/pr.diff
7 changes: 7 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.DS_Store
/vendor/
/.phpunit.cache/
/.phpcs-cache
phpunit.xml
phpcs.xml
composer.lock
26 changes: 26 additions & 0 deletions composer.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"name": "frumph/easel",
"description": "Easel — a WordPress theme for publishing a webcomic.",
"type": "wordpress-theme",
"license": "GPL-3.0-or-later",
"require": {
"php": ">=7.4"
},
"require-dev": {
"phpunit/phpunit": "^11.5",
"squizlabs/php_codesniffer": "^3.10",
"wp-coding-standards/wpcs": "^3.1",
"dealerdirect/phpcodesniffer-composer-installer": "^1.0"
},
"config": {
"allow-plugins": {
"dealerdirect/phpcodesniffer-composer-installer": true
}
},
"scripts": {
"test": "phpunit",
"lint": "phpcs",
"lint:fix": "phpcbf",
"lint:php": "find . -name '*.php' -not -path './vendor/*' -not -path './.git/*' -print0 | xargs -0 -n1 php -l"
}
}
30 changes: 30 additions & 0 deletions phpcs.xml.dist
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?xml version="1.0"?>
<ruleset name="Easel">
<description>
Escaping and security sniffs for the Easel theme.

Scoped deliberately narrow. The theme predates these standards by a decade and a
full WordPress-Extra run reports thousands of pre-existing findings, which would
bury the ones that matter. Start with the sniffs that catch the bug classes this
theme has actually had -- unescaped output in templates, unprepared SQL, missing
nonce and capability checks -- and widen later if anyone has the appetite.
</description>

<file>.</file>
<exclude-pattern>/vendor/*</exclude-pattern>
<exclude-pattern>/tests/*</exclude-pattern>
<!-- Third-party JavaScript the theme bundles rather than authors. -->
<exclude-pattern>/js/*</exclude-pattern>

<arg name="extensions" value="php"/>
<arg name="colors"/>
<arg value="sp"/>

<config name="minimum_wp_version" value="4.6"/>
<config name="testVersion" value="7.4-"/>

<rule ref="WordPress.Security"/>
<rule ref="WordPress.DB.PreparedSQL"/>
<rule ref="WordPress.DB.PreparedSQLPlaceholders"/>
<rule ref="WordPress.WP.EnqueuedResources"/>
</ruleset>
16 changes: 16 additions & 0 deletions phpunit.xml.dist
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?xml version="1.0" encoding="UTF-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd"
bootstrap="tests/bootstrap.php"
cacheDirectory=".phpunit.cache"
colors="true"
failOnWarning="true"
failOnNotice="true"
failOnDeprecation="true"
beStrictAboutOutputDuringTests="true">
<testsuites>
<testsuite name="easel">
<directory>tests</directory>
</testsuite>
</testsuites>
</phpunit>
Loading