Add lint rule to enforce PHP deprecated-function detection - #4205
dd-apm-ecosystems[bot] wants to merge 2 commits into
Conversation
Recurring code-review theme over the past ~400 days: reviewers repeatedly flagged deprecated/version-incompatible PHP function usage by hand (e.g. spl_object_hash on PHP 8.6 in PR #4145, missing PHP_VERSION_ID guards in PR #3514, PHP 7.4-only syntax in PR #3664). This sniff was already scaffolded but commented out in tooling/php-lint/phpcs.xml with a note that src/ has zero hits today, so flipping it on adds detection with no existing-code cleanup required.
Follow-up to the phpcs.xml change in this branch: keep the README's "what runs" section in sync so it no longer claims deprecated-function detection is left commented out.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41a0b290e8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| vetted as a zero-hit, low-risk candidate for src/ (see git history | ||
| of this file) — this change only turns it on. | ||
| --> | ||
| <rule ref="Generic.PHP.DeprecatedFunctions"/> |
There was a problem hiding this comment.
When the lint image advances from PHP 8.5 to 8.6, this token-based sniff will report the two existing spl_object_hash() calls in src/DDTrace/Integrations/Integration.php:67,72, even though they are guarded by PHP_VERSION_ID < 70200 and are unreachable on PHP 8.6. Because the blocking job always selects the newest supported PHP image, adding 8.6 will therefore make the existing tree fail lint; whitelist/suppress compatibility-only calls or use a version-aware check before enabling this rule.
Useful? React with 👍 / 👎.
| `Generic.PHP.DeprecatedFunctions`, which flags calls to any function | ||
| the PHP interpreter running phpcs reports as deprecated via | ||
| `ReflectionFunction::isDeprecated()`. That one is not purely a style | ||
| choice: it exists because reviewers repeatedly had to catch | ||
| deprecated/version-incompatible function usage by hand (e.g. | ||
| `spl_object_hash` on PHP 8.6, missing `PHP_VERSION_ID` guards). It does |
There was a problem hiding this comment.
This sniff only asks the PHP 8.5 lint process whether a function is deprecated; it has no information about when functions were introduced. For example, an unguarded spl_object_id() or str_contains() call passes this check even though those functions are unavailable on PHP 7.0 and PHP 7.4 respectively, both within the repository's supported range. Thus the rule does not catch the advertised version-incompatible-function cases; add a version-aware compatibility check or narrow this documentation to deprecated functions on the runner only.
Useful? React with 👍 / 👎.
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 5 Pipeline jobs failed
ℹ️ InfoNo other issues found (see more)🧪 All tests passed 🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 41a0b29 | Docs | View more details | Give us feedback! |
colin-higgins
left a comment
colin-higgins
left a comment
There was a problem hiding this comment.
Will likely close this PR as it doesn't properly handle version flags, this is just a test run for agentic rule suggestion.
| <!-- | ||
| Enabled: recurring review-feedback theme, now mechanically enforced. | ||
|
|
||
| Over the trailing ~400 days of PR review activity, human reviewers | ||
| repeatedly had to flag calls to functions that are deprecated (or | ||
| outright unavailable) on some PHP version this repo still supports, | ||
| e.g.: | ||
| - "spl_object_hash is deprecated since PHP 8.6. You can simply | ||
| use spl_object_id." (Drupal integration) | ||
| - "Needs a PHP_VERSION_ID < 80000 check to avoid deprecations" | ||
| (Symfony integration) | ||
| - "this is php 7.4 syntax" (repo supports PHP 7.0+) | ||
| - "`const` was missing on the PHP 7.0 and 7.1 versions" (stub | ||
| signatures) | ||
| This class of feedback is exactly what Generic.PHP.DeprecatedFunctions | ||
| is built to catch mechanically: it walks the token stream for calls | ||
| to any function PHP itself reports as deprecated via | ||
| ReflectionFunction::isDeprecated() on the interpreter running phpcs | ||
| (the newest supported PHP in CI, see .gitlab/generate-common.php). | ||
| It does not parse the file with the runner's parser the way | ||
| Generic.PHP.Syntax does, so it carries none of that sniff's | ||
| cross-version syntax risk (see the note below). It was already | ||
| vetted as a zero-hit, low-risk candidate for src/ (see git history | ||
| of this file) — this change only turns it on. | ||
| --> |
There was a problem hiding this comment.
that's a commit message, not a comment belonging into this file.
2 participants
Summary
Adds
Generic.PHP.DeprecatedFunctionsto the blocking CI PHP lint job (tooling/php-lint/phpcs.xml, run viacomposer ci-lint/.gitlab/generate-shared.php's "PHP lint" job) to catch calls to deprecated/version-incompatible PHP functions insrc/before merge.The recurring review theme
Over the trailing ~400 days of human-authored PR activity on this repo, reviewers repeatedly had to manually flag PHP version-compatibility issues in the same class of mistake — using a function or syntax that is deprecated, or unavailable, on some PHP version this repo still supports (PHP 7.0 through current, per
tooling/php-lint/README.md):PHP_VERSION_ID < 80000check to avoid deprecations"constwas missing on the PHP 7.0 and 7.1 versions"That's 5 independent PRs, by different authors/reviewers, over the review window — a genuine recurring pattern, not a one-off.
Why this is a good candidate for automated enforcement
Generic.PHP.DeprecatedFunctionssniff walks the token stream for calls to any function the running PHP interpreter reports as deprecated (viaReflectionFunction::isDeprecated()). No semantic/business judgment required.Generic.PHP.Syntax(also mentioned in this ruleset) has, because it doesn't re-parse the file with the runner's parser — it only inspects token names against the interpreter's own deprecation list.What tool/config was used and how it wires into CI
This does not introduce new tooling.
tooling/php-lint/phpcs.xmlalready had this exact sniff commented out, with a note left by the team: "current src/ has no hits, but the sniff list grows with PHPCS releases and tracks language deprecations" — i.e. it was already vetted as a safe, zero-hit candidate, just never flipped on. This PR:<rule ref="Generic.PHP.DeprecatedFunctions"/>intooling/php-lint/phpcs.xml.tooling/php-lint/README.mdso the "what runs" section reflects the change.This ruleset is already wired into the blocking
"PHP lint"GitLab CI job (see.gitlab/generate-shared.php), which runstooling/php-lint/run.sh→phpcs -s --standard=tooling/php-lint/phpcs.xml. That job is not listed in.gitlab/flaky-jobs.txt, so per.gitlab/merge-gate.sha failure here blocks merge like any other required job — no new workflow needed.Scope stays limited to first-party
src/(per the existing<file>directive), so generated bridge files and test fixtures are unaffected, and the check only inspects newly-authored/modifiedsrc/— consistent with the "don't break the whole codebase" guardrail, sincesrc/already has zero hits for this sniff today (verified via code search acrosssrc/for common deprecated functions/patterns such asspl_object_hash,each(,create_function,utf8_encode,get_magic_quotes*,money_format,FILTER_SANITIZE_STRING— none found).How to verify locally
composer ci-lint # or directly: bash tooling/php-lint/run.shBoth should currently pass with no new violations, since
src/has no deprecated-function calls today. Introducing one (e.g.spl_object_hash(...)) in a follow-up branch will now fail the lint job.This PR was opened by an automated code-review-efficiency agent that analyzes recurring PR review feedback on this repo. No merge action was taken — please review and merge manually.