Skip to content

Modernise PHP codebase to PHP 7.4 - #780

Open
abhijitb wants to merge 16 commits into
developfrom
update/php-7.4-modernization
Open

Modernise PHP codebase to PHP 7.4#780
abhijitb wants to merge 16 commits into
developfrom
update/php-7.4-modernization

Conversation

@abhijitb

Copy link
Copy Markdown
Contributor

Proposed changes

  • Align first-party PHP tooling to the declared PHP 7.4 floor (composer platform, PHPCS testVersion, CI --runtime-set).
  • Mechanically modernise in-scope code: short arrays [], null coalescing, light fn() usage.
  • Add native param/return types (and typed properties where safe) across helpers, Admin/widgets, REST non-overrides, compat-check classes, and updates/bootstrap — without typing WP core parent overrides.
  • Document contributor standards; defer PHPStan; leave vendor/ and out-of-scope files untouched.

Type of Change

Production

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update
  • Refactoring / housekeeping (changes to files not directly related to functionality)

Development

  • Tests
  • Dependency update
  • Environment update / refactoring
  • Documentation Update

Visual

Checklist

  • I have read the CONTRIBUTING doc
  • I have viewed my change in a web-browser
  • Linting and tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

abhijitb and others added 6 commits July 17, 2026 23:12
Align tooling to PHP 7.4, adopt short arrays and null coalescing, add native types to helpers/utilities, and document the remaining phased plan.

Co-authored-by: Cursor <cursoragent@cursor.com>
Safely read nested PayPal attribution headers, clarify short-array PHPCS rules, and restore HTTPS VCS source URLs in composer.lock.

Co-authored-by: Cursor <cursoragent@cursor.com>
Phase 4 of the PHP 7.4 modernization: type Admin and SitePreview methods without changing hook registration behavior.

Co-authored-by: Cursor <cursoragent@cursor.com>
Phase 5: type non-override REST methods only; leave WP_REST_Controller overrides untyped to avoid PHP fatals.

Co-authored-by: Cursor <cursoragent@cursor.com>
Phase 6: typed properties/methods on early-load compat checks, updates helpers, bootstrap activation hooks, and AutoIncrement::$wpdb.

Co-authored-by: Cursor <cursoragent@cursor.com>
Defer PHPStan until stubs/CI are owned separately; lock PHPCS exclusions and contributor rules for first-party PHP 7.4 code.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown

AI code review

✅ Strengths

  • Brings the codebase in line with the declared PHP 7.4 floor across multiple touchpoints: composer platform, PHPCS runtime flag in CI, and modernized syntax (short arrays, null coalescing, light fn()).
  • Sensible native typing: adding : void on first-party functions that aren’t WP core overrides is safe and improves readability without risking fatals.
  • Safer PayPal BN code header handling by defensively reading headers with ?? [] and is_array() before accessing the key — reduces notices and odd edge cases when plugins pass a string.
  • Good housekeeping: removal of the unused CachingController require lowers surface area and confusion given the module-level route exists; CLAUDE.md is excellent, comprehensive contributor guidance tailored to this repo.
  • Developer experience: added PHPUnit phar workflow via scripts, updated .gitignore for local PHPUnit artifacts, and consistent PHPCS runtime testVersion usage in CI and composer scripts.

⚠️ Suggestions

  • Double-check the plugin header Requires PHP in wp-plugin-web.php still matches 7.4 to avoid mixed signals to WordPress.org and sites. If it’s already 7.4, all good.
  • Consider moving --runtime-set testVersion 7.4- into phpcs.xml (if not already there) and keeping CI flags minimal; this keeps local and CI results aligned even if scripts drift.
  • You’ve clearly documented that vendor/bin/phpunit is broken for this plugin. To reduce confusion:
    • Consider dropping phpunit/phpunit from require-dev so vendor/bin/phpunit doesn’t even exist, or
    • Keep it but add a CI test job and a local test:ci script that runs composer run test:install-phpunit && composer run test, and call that from your workflows, making the intended path unmistakable.
  • Ensure there are no remaining references to the removed inc/RestApi/CachingController.php (e.g., in inc/RestApi/rest-api.php, any router registration, or JS callers). If any stubs remain, follow up with a quick cleanup.
  • Minor: in the BN code logic, you already guard nulls and non-arrays; optionally add is_string( $bn_code ) before stripos() for extra belt-and-suspenders safety, though the current short-circuiting makes it functionally safe.

❌ Issues

  • None blocking found in this diff. The platform bump to PHP 7.4 will cause some prod deps (e.g., credis) to resolve to 7.4+ builds, which matches our stated floor. Just ensure the release process continues to use composer install --no-dev --optimize-autoloader as documented (it is, via npm run php-deps).
  • Test coverage isn’t shown here; the only behavioral change in-scope is the more defensive BN header handling, which is low-risk. If we have an integration test around the PayPal header stamping, it could be updated to assert the null/non-array header cases too — non-blocking.

⚠️ Verdict: Merge with minor follow-ups

Overall this is a solid, low-risk modernization pass aligned with our 7.4 policy and contributor guidance. I recommend merging once you confirm the plugin header Requires PHP: 7.4, verify there are no lingering references to CachingController, and consider the PHPUnit dependency/script clarifications to prevent accidental vendor/bin/phpunit usage.

abhijitb and others added 8 commits July 22, 2026 10:16
Use fully qualified \wpdb under the Web namespace, and correct strpos so the brand footer only runs on web plugin admin pages.

Co-authored-by: Cursor <cursoragent@cursor.com>
Documents PHP load order, Newfold module system, PHP typing standard,
and common build/lint/test commands for future Claude Code sessions.
- Admin::add_to_runtime() coerces non-array filter values instead of
  hard-failing with a TypeError.
- Admin::__construct() drops the redundant $_GET isset check now that
  filter_input() is used directly, removing the phpcs:ignore.
- Admin::assets() falls back to empty dependencies/current version when
  the build manifest is missing, instead of leaving $asset undefined.
- base.php explicitly casts both operands before subtracting timestamps.
- tests/phpunit/bootstrap.php now loads the plugin via muplugins_loaded
  so plugin classes are available to PHPUnit tests.
- Add lightweight integration tests for the settings/caching REST
  controllers, the Admin asset fallback, and the NFD compat-check
  deactivation paths.
- Add phpunit/phpunit and yoast/phpunit-polyfills as dev dependencies,
  plus composer scripts (`composer run test`, `test:install-phpunit`)
  that run the suite via a downloaded tools/phpunit.phar.
- tests/phpunit/bootstrap.php no longer eagerly requires
  vendor/autoload.php: doing so runs every Newfold module's
  Composer `autoload.files` bootstrap.php before ABSPATH is defined,
  which silently no-ops their ABSPATH guards and leaves services like
  `comingSoon`/`cachePurger` unregistered in the container.
  vendor/bin/phpunit is now documented as broken for this repo since
  it triggers that same autoload before our --bootstrap file ever runs;
  the phar avoids it entirely.
- Add tests/phpunit/wp-tests-config-sample.php as the template for the
  gitignored, machine-local wp-tests-config.php.
- Fix CachingController::purge_all(), which called the cache purging
  service's purgeAll() — a method that doesn't exist on
  NewfoldLabs\WP\Module\Performance\Cache\CachePurgingService (only
  purge_all() does). This fatally errored on every real
  DELETE /web/v1/caching request; caught by the new REST integration
  tests once the container was wired up correctly.
CachingController::purge_all() still calls the non-existent purgeAll()
on CachePurgingService (only purge_all() exists, per usage elsewhere
in wp-module-performance and wp-module-onboarding). Confirmed the
route's only frontend caller, webPurgeCacheApiFetch in
src/app/util/helpers.js, is currently unused/unwired, so this route
isn't reachable from the UI today. Leaving the bug in place per
decision; the corresponding integration test is now skipped with a
comment explaining why, so the suite stays green without hiding the
issue.
web/v1/caching (CachingController::purge_all()) had no live caller:
its only frontend consumer, webPurgeCacheApiFetch in
src/app/util/helpers.js, was itself never imported or invoked
anywhere. History confirms this was intentional fallout from an
incomplete migration ("Remove: caching controller and cacheLevel
option usage, both things are now managed at module level") to
wp-module-performance's own cache REST controller
(newfold-performance/v1/cache), which is what the admin UI's "Clear
Cache" action actually calls today.

Removes inc/RestApi/CachingController.php, its require in
bootstrap.php, its entry in rest-api.php's controller list, the
orphaned webPurgeCacheApiFetch export, and the corresponding
(skipped) test.
@abhijitb
abhijitb requested review from ajayadav09, diwanshuster and ramyakrishnai and removed request for ramyakrishnai August 5, 2026 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant