Don't call a string view_class that shares a PHP function name - #1709
Merged
Conversation
`view_class` is a documented key on the public `gfpdf_one_time_action_routes` filter, and its value is a CSS class string. #1708 made the key accept a callable so the deprecation notice's style resolves when the notice displays rather than when the route table is built, and resolved it with a bare `is_callable()`. `is_callable()` says true for any string naming a PHP function, so a route styling its notice `link`, `key`, `header`, `current` or `compact` — all ordinary-looking CSS class names, all PHP functions — had its class name called instead of rendered. `call_user_func( 'link' )` raises an uncaught `ArgumentCountError`, so the admin screen white-screens for every page the notice can appear on. Core is unaffected: `notice-warning` and `notice-error` carry a hyphen, so they were never callable. This only reaches a site whose add-on or snippet supplies its own `view_class` through the filter. A string is now always taken as the class itself, and only a non-string callable is resolved. `Closure`, and the array and object forms, still work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1
added a commit
that referenced
this pull request
Sep 2, 2026
Cherry-picked from 9783123 on `development`, adapted for the 6.16.x line. The v3 backwards compatibility layer is scheduled for removal in Gravity PDF 7.0, but there is currently no way to know which sites still depend on it. Legacy (v3) templates render blank under Gravity Forms 3.0 with no fatal, no warning and nothing in the debug log; the `gfpdfe_*` filters fire silently; and the deprecated methods call `_doing_it_wrong()`, which does not trigger `deprecated_function_run` and so never reaches WordPress' deprecation logging or Query Monitor. This adds detection first, so the removal in 7.0 can be judged on real data rather than assumption. `GFPDF\Statics\Deprecation` is a registry rather than a fixed list: it knows how to detect, group and warn about whatever its providers declare, and nothing in it names the v3 layer or the release removing it. `GFPDF\Statics\Deprecation_V3` is the one provider today, declaring four features and the version each goes in, so a later round of removals registers itself rather than being wired through the engine, the model and the controller. The v3 provider declares legacy templates (any template `Helper_Templates` classifies in the `Legacy` group), Business Plus templates (the same set, split on whether the file calls `$mpdf->`), legacy download URLs (a `LIKE` for `gf_pdf=1` over the three columns Gravity Forms splits a form across, unioned with the forms the endpoint records for itself), and deprecated filters (the known `gfpdfe_*` hooks and v3-shaped `gfpdf_*` aliases, looked up in `$wp_filter`). Detections surface on three screens: a Deprecated section at the head of the System Report, a Site Health test and matching Info tab section, and one admin notice covering every detected feature. The notice reads the last recorded detection rather than detecting itself, since detection walks the form table and the template directory. The report and Site Health screens refresh that record as they go, and `Controller_Upgrade_Routines` takes it at install and on every version change. Also included: a shim aliasing `GFForms` back to `RGForms` so v3 templates stop rendering blank under GF 3.0; log warnings on Advanced Templating processing and on any `gfpdfe_*` filter with a listener; and the `_doing_it_wrong()` to `_deprecated_function()` conversions across the surface that is deprecated on this line. Two `Helper_Notices` fixes came out of building the notice, both on the pre-existing `view_class` route key any add-on can already use: a caller-supplied `notice-*` class now replaces the default `updated` state rather than being appended to it (`div.updated` out-specifies `.notice-warning`), and queued notices are stored as message/class pairs rather than keyed by their class, which silently dropped a second notice sharing one. It also carries the follow-ups from #1708 on `development`, so the two lines stay aligned rather than drifting apart the moment this lands. `Deprecation::get_signals()` is memoised, behind a `Deprecation::flush_cache()` that fans out to every provider — `Helper_Interface_Deprecated_Features` gains `flush_cache()` so a provider owns invalidating whatever it holds, and callers no longer have to know which class caches what. The notice's `view_class` route key now accepts a callable, resolved when the notice displays rather than when the route table is built, so nothing queries the site to style a notice that never renders. The string form still works, since that key is public API on `gfpdf_one_time_action_routes` — and only a NON-string callable is resolved, because `is_callable()` is true for any string naming a PHP function, so a route styling its notice `link` or `key` would have had its class name called and fatalled the admin (fixed on `development` in #1709). The unused `Deprecation` import is dropped from `Controller_Actions`. Differences from 9783123, which was written against `development`: - `development` carries unreleased work this line does not — a PDF cache, `Statics\Cache`/`Statics\Debug`, fully scoped `Psr\Log`, and ~20 deprecated methods already gutted into stubs. Where the upstream commit converted one of those stubs to `_deprecated_function()`, the method is still live here and keeps its body with no notice: the `pdf.php` canonical-release notices, `Helper_Misc::maybe_add_multicurrency_support()`, `Controller_Pdf_Queue::queue_cleanup_task()`, `Queue_Callbacks::cleanup_pdfs()`, `Model_PDF`'s three cleanup methods, and `View_PDF::generate_pdf()` and `::get_template_filename()`. Where upstream only added a warning to a still-live method, the warning is applied. - `Controller_PDF::sgoptimizer_html_minification_fix()` is the exception to the rule above, and follows `development` instead: the SiteGround buffer fix has not been required since 6.12, when all buffers began being auto-closed before a PDF is sent to the browser. Its two `add_filter()` registrations are removed and the method deprecates. That matters beyond tidying, because one of those registrations was on `gfpdf_legacy_pre_view_or_download_pdf` — a hook this change declares deprecated — so while it stood, the scan counted core's own listener and reported every install as using the v3 layer. `test_a_clean_install_reports_no_deprecated_filters()` fails if core ever registers on another deprecated hook. - The Playwright suite does not exist on this line, so the deprecation spec and its helpers are omitted. - The PHPUnit suite here predates the harness the upstream tests were written against. `Test_Deprecation`, `Test_Deprecation_V3` and the `Concerns` traits are ported onto `WP_UnitTestCase`, with a `UsesFactory` trait supplying `gf_factory()` from the `GF_UnitTest_Factory` this branch already has, autoloaded from the test bootstrap. `Test_Controller_System_Report` moves from positional table indexes to a name-based lookup, since a section dropped above now renumbers the ones below it. #1708 flushes the detection memo from the shared `tests/phpunit/integration/TestCase::set_up()`; there is no shared base class here, so the four test classes that touch detection flush in their own `set_up()` instead. `test-actions.php` gains this line's equivalent of the `view_class` coverage that lives in upstream's un-ported `Test_Controller_Actions`, and `Test_Controller_System_Report` keeps the pre-port assertion that our report is appended to Gravity Forms' own rather than replacing it, which the move to a name-based section lookup had dropped. It carries the upstream tests for the Site Health registration, the `debug_information` section and the `gravityforms_view_settings` gate on both, plus the clean-site assertions that keep those gates from passing vacuously. The upstream tests asserting what each surface reports once a signal is detected are not ported. Integration suite: 1181 tests, 3682 assertions. Multisite: 1181 tests, 3778 assertions. The 3 errors and 2 failures in `Helper\Mpdf\Test_Request` reproduce identically on an untouched worktree and are an artifact of a borrowed `vendor_prefixed` build, not this change. PHPCS and PHP 7.3 compatibility clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Coverage report for commit: f62fc8f Summary - Lines: 92.99% | Methods: 88.31% | Branches: 81.37%
🤖 Jest coverage report |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage report for commit: f62fc8f Summary - Lines: 83.47% | Methods: 89.60%
🤖 PHPUnit coverage report |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to #1708.
view_classis a documented key on the publicgfpdf_one_time_action_routesfilter and its value is a CSS class string. #1708 made the key accept a callable, so the deprecation notice's style is resolved when the notice displays rather than when the route table is built, and it resolved that with a bareis_callable().is_callable()returnstruefor any string naming a PHP function. A route styling its noticelink,key,header,currentorcompact— all ordinary-looking CSS class names, all PHP functions — had its class name called instead of rendered.call_user_func( 'link' )raises an uncaughtArgumentCountError, which white-screens every admin page the notice can appear on.Core is unaffected, because
notice-warningandnotice-errorcontain a hyphen and were never callable. This only reaches a site whose add-on or snippet supplies its ownview_classthrough the filter.A string is now always taken as the class itself; only a non-string callable is resolved, so
Closureand the array/object forms still work.Found while verifying the 6.17 port of this feature (#1707), which carries the same code.
Testing instructions
Add a snippet that supplies a
view_classcolliding with a PHP function name, then load any admin page where a Gravity PDF one-time-action notice displays:Before this change the screen is blank with
ArgumentCountError: link() expects exactly 2 arguments, 0 given. After it, the notice renders withclass="notice updated link".Confirm the deprecation notice still styles itself correctly too — it passes a
Closure, so it should be unaffected.Checklist:
Additional Comments
Test_Controller_Actions::test_a_string_view_class_is_never_calledcovers it. Mutation-verified: reverting the guard makes it error withArgumentCountError: link() expects exactly 2 arguments, 0 given, and it passes with the guard in place.Test_Controller_Actionsis green at 12 tests / 39 assertions; PHPCS and PHP compatibility clean.