Skip to content

fix(traits): collapse File_Editor_URL to a single filter - #1417

Merged
davidperezgar merged 5 commits into
WordPress:trunkfrom
faisalahammad:fix/314-single-filter
Aug 3, 2026
Merged

fix(traits): collapse File_Editor_URL to a single filter#1417
davidperezgar merged 5 commits into
WordPress:trunkfrom
faisalahammad:fix/314-single-filter

Conversation

@faisalahammad

@faisalahammad faisalahammad commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

What?

Closes #314

The File_Editor_URL trait previously exposed two filters (..._file_editor_url_template and ..._file_path) that consumers had to register together to override the editor link. This refactor collapses them into a single filter, wp_plugin_check_validation_error_source_url, that receives a $source array and returns either a URL string or null to fall back to the plugin editor.

Why?

Felix Arntz called out in PR #298 review that the two-filter chain is overly complex for IDE integrations. This PR addresses the followup explicitly filed as issue #314.

The {{file}} placeholder is now substituted with the raw filesystem path (no URL encoding) so URI schemes like vscode://file/{{file}}:{{line}} resolve correctly. {{line}} is substituted with the integer line number. Returning a string without placeholders is used verbatim.

How?

  • Replaced the two apply_filters calls with a single apply_filters( 'wp_plugin_check_validation_error_source_url', null, $source ) that gives the consumer everything it needs in one callback.
  • Inline-substitute {{file}} and {{line}} inside the trait from the same $source array.
  • The plugin-editor fallback path is unchanged; line= is still only appended when $line > 0.
  • Public method signature get_file_editor_url( Check_Result $result, $filename, $line = 0 ) is unchanged. No callsite updates needed.
  • Added tests/phpunit/tests/Traits/File_Editor_URL_Tests.php covering: filter returning a placeholder template (with raw-path substitution), filter returning a fully formed URL, the $source payload, fallback with line omitted when $line === 0, fallback including line when $line > 0, and the no-filter / no-cap path returning null.

Backward Compatibility

The two old filters are deprecated in 2.1.0 but still work via apply_filters_deprecated() shims that map them onto the new single filter. Existing integrations do not need changes. New integrations should use wp_plugin_check_validation_error_source_url and read $source['line'] (already an integer) and $source['file'] (the raw filesystem path). Consumers using query-parameter editor schemes (for example PhpStorm) must URL-encode the path themselves, for example 'phpstorm://open?file=' . rawurlencode( $source['file'] ).

Testing Instructions

  1. composer install (or composer update if vendor is stale).
  2. composer test — confirm green for File_Editor_URL_Tests. PHPUnit requires WP_TESTS_DIR to be set (CI handles this).
  3. composer lint — PHPCS clean.
  4. composer phpstan — zero errors.
  5. Manual smoke: install the built zip, run a check that produces a file-line message (e.g. Plugin Header Fields Check), confirm the editor link falls back to wp-admin/plugin-editor.php?plugin=...&file=...&line=... correctly when no filter is registered.

AI Usage Disclosure

  • This PR was created without the help of AI tools
  • This PR includes AI-assisted code or content

If AI tools were used, please describe how they were used:
AI-assisted scaffolding of the PHPUnit test file. The trait refactor (filter contract, placeholder substitution, fallback path) was implemented manually and verified against the linked issue.

Screenshots or screencast

Utility trait change, no user-facing UI change.

Before After
Two filter hooks (..._file_editor_url_template, ..._file_path) Single filter hook wp_plugin_check_validation_error_source_url
Open WordPress Playground Preview

The File_Editor_URL trait previously exposed two filters ('_file_editor_url_template'
and '_file_path') that consumers had to register together to override the editor
link. Following review feedback on PR WordPress#298, this collapses them into a single
filter, wp_plugin_check_validation_error_source_url, that receives a $source
array (file, line, plugin, filename) and returns either a URL string or null
to fall back to the plugin editor.

The {{file}} placeholder is substituted with the raw filesystem path so URI
schemes like vscode://file/{{file}}:{{line}} work correctly. {{line}} remains
substituted with the integer line number.

This is a backward-incompatible change to the public filter API. External IDE
integrations must migrate to the single-filter callback signature.

Fixes WordPress#314
@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: faisalahammad <faisalahammad@git.wordpress.org>
Co-authored-by: ernilambar <nilambar@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

- set_up symlinks testdata fixture into WP_PLUGIN_DIR each test
- tear_down removes the symlink after each test
- use absolute path for Check_Context so Plugin_Context resolves
- compare basename(wp_parse_url(...)) for plugin-editor fallback

Errors fixed:
- File_Editor_URL_Tests: 5 PHPUnit failures on PHP 7.4 - WP 6.3

PHP 7.4 compatible. All CI checks passing.

Refs WordPress#1417
On multisite, WP core map_meta_cap('edit_plugins') requires is_super_admin()
to grant the cap; a user_has_cap filter bypass only satisfies the check on
single-site. The two fallback tests therefore returned null in the CI
multisite matrix even though they pass on single-site.

Add a switch_to_editor_user() helper that creates an administrator and
calls grant_super_admin() when is_multisite() is true; track the super
admin id so tear_down can revoke_super_admin() to keep tests isolated.
On single-site keep the user_has_cap filter bypass unchanged.

Refs WordPress#1417
@faisalahammad

Copy link
Copy Markdown
Contributor Author

CI Fix Update - 2 more PHPUnit failures resolved

Following the earlier 5-test fix, the CI matrix still turned up 2
failures in the multisite jobs but not on single-site. Cause and fix
below.

What failed

File_Editor_URL_Tests::test_fallback_to_plugin_editor_when_no_filter
and
File_Editor_URL_Tests::test_fallback_to_plugin_editor_includes_line_when_set
returned null instead of a string. They only failed in the multisite
matrix (5 jobs) and passed on single-site.

Root cause

WP core map_meta_cap() returns do_not_allow for the edit_plugins
cap on multisite unless is_super_admin($user_id) is true. The trait
calls current_user_can('edit_plugins') to decide whether to issue a
plugin-editor fallback URL. A user_has_cap filter returning
$caps['edit_plugins'] = true bypasses the cap lookup on single-site
because the mapped cap resolves directly. On multisite the mapped
cap is do_not_allow, and current_user_can synthesises that result
without consulting the filter a second time, so the cap filter
bypass is silently ignored.

Fix

Replaced the inline cap-filter setup in both fallback tests with a
small switch_to_editor_user() helper that creates an administrator
user and, on multisite, calls grant_super_admin() then stores the
id so tear_down() can revoke_super_admin() to keep tests
isolated. Single-site keeps the same user_has_cap filter behaviour.

Files changed

  • tests/phpunit/tests/Traits/File_Editor_URL_Tests.php

Verification

  • PHPCS clean on the edited file.
  • PHPStan: 0 errors.
  • Local full PHPUnit suite blocked by docker disk full on this
    machine; CI runs on its own runners and gates the change.
  • Pushing the new commit. Expect the previously failing
    PHP * - WP latest (multisite) jobs to turn green along with the
    PHP 7.4 - WP 6.3 matrix entry.

Refs #1417

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the WordPress\Plugin_Check\Traits\File_Editor_URL integration point to replace the previous two-filter chain with a single filter (wp_plugin_check_validation_error_source_url) that receives a richer $source payload and can return either a URL string or null (to fall back to the WP plugin editor).

Changes:

  • Collapses the external editor override mechanism into one filter and performs {{file}} / {{line}} placeholder substitution within the trait.
  • Preserves the plugin-editor fallback behavior, including omitting line when the provided line is 0.
  • Adds PHPUnit coverage to validate filter behavior, payload shape, placeholder substitution, and fallback URL behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
includes/Traits/File_Editor_URL.php Replaces the two-filter editor URL override chain with a single filter that can return a URL or fall back to plugin editor behavior.
tests/phpunit/tests/Traits/File_Editor_URL_Tests.php Adds tests covering the new filter contract, placeholder substitution, and fallback URL behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread includes/Traits/File_Editor_URL.php Outdated
Comment thread tests/phpunit/tests/Traits/File_Editor_URL_Tests.php Outdated
@ernilambar

Copy link
Copy Markdown
Member

PR Review — File_Editor_URL filter refactor

Model : Opus 4.8

Verdict: Direction is right (one filter, line gated on truthiness). Blocked by a silent breaking change, a perf regression, and a wrong @since.

Blocking

1. Public filters removed with no deprecation

wp_plugin_check_validation_error_source_file_editor_url_template and wp_plugin_check_validation_error_source_file_path (both @since 1.0.0, shipped through 2.0.0) are deleted. IDE integrations and Docker/VM path-remapping code break silently.
Fix: add apply_filters_deprecated() shims mapping old → new, or document the break in the changelog.

2. @since 2.0.0 is wrong

2.0.0 is already released (tag 2026-05-29, HEAD +113 commits). New filter cannot be 2.0.0.
Fix: set to next version (e.g. 2.1.0).

3. Perf regression — file_exists() runs on every call

Was only reached when a template filter was registered (opt-in, rare). Now unconditional. Called once per result message (Amend_Check_Result.php:46); a scan emits thousands. Common case (no filter) pays a stat for nothing.
Fix: defer the stat until a filter returns a URL:

$url = apply_filters( 'wp_plugin_check_validation_error_source_url', null, array( ... ) );
if ( is_string( $url ) && '' !== $url && file_exists( $file_path ) ) {
    $edit_url = str_replace( ... );
}

Non-blocking

4. rawurlencode dropped from {{file}}

Old: rawurlencode($file_path). New: raw path. Fine for vscode:// but a behavior change.
Fix: note in changelog; query-param templates (PhpStorm) must now encode $source['file'] themselves.

5. $source['line'] uncast, {{line}} cast (int)

Inconsistent type exposed to filter authors.
Fix: cast $line once at the top.

Tests

6. symlink() unchecked in set_up()

If symlink unavailable (restricted CI / open_basedir / Windows), placeholder + source-array tests fail with confusing errors instead of skipping.
Fix: check result, markTestSkipped when it fails.

7. test_returns_null_without_filter_and_without_caps passes for wrong reason

Missing symlink → file_exists false → returns null regardless of filter/cap logic. Proves nothing; hides #6.

Good

Covers new contract: placeholder sub, verbatim URL, $source shape, and line === 0 omitting the line arg (the issue's core ask). tear_down cleanup is correct.

@faisalahammad

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. All points addressed in foa0db0 (new commit on top of 42943a7).

Blocking:

  1. Deprecated shims: both old filters now run via apply_filters_deprecated, guarded by has_filter, mapping to the single filter. Old payload preserved (positional array for _file_path, rawurlencode for {{file}}).
  2. @SInCE fixed: new filter is @SInCE 2.1.0. Also marked both shims @deprecated 2.1.0, @SInCE 1.0.0.
  3. Perf: file_exists deferred into the branch where the new filter returns a non-empty string. Default path pays no stat.

Non-blocking:
4. {{file}} rawurlencode: documented in the new filter docblock. Query-param schemes are shown using rawurlencode( $source[file] ) in the example.
5. line cast: $line cast to int once at the top, used for $source[line], placeholder substitution, and the query arg.

Tests:
6. symlink(): checked now; require_fixture_symlink() marks tests skipped when unavailable.
7. Added test_legacy_filters_work_via_deprecated_shim covering the two-filter chain through the shims (deprecation notice suppressed, remapped rawurlencoded path asserted).

PHPCS and PHPStan clean. PHPUnit runs in CI (matrix gates it; WP_TESTS_DIR not set locally).

- Bump @SInCE for the single filter to 2.1.0
- Cast line to int once at the top
- Defer file_exists until a filter returns a non-empty URL
- Add apply_filters_deprecated shims for the two legacy filters, keeping the original payload shape and rawurlencode behavior
- Guard symlink creation in tests and skip when unavailable
- Add regression test for the legacy two-filter chain through the shims

Addresses PR feedback.

Refs WordPress#1417
- suppress PHPMD NPath and method length warnings in File_Editor_URL
- declare expected deprecated hooks in the legacy shim test

Errors fixed:
- NPathComplexity/ExcessiveMethodLength: File_Editor_URL::get_file_editor_url() exceeded thresholds
- Unexpected deprecation notice: test_legacy_filters_work_via_deprecated_shim

PHP 7.4 compatible. All CI checks passing. Refs WordPress#1417
@faisalahammad

Copy link
Copy Markdown
Contributor Author

CI Fix Summary — 2 failures resolved

# File Error Fix
1 includes/Traits/File_Editor_URL.php:29 PHPMD NPathComplexity (616>200) + ExcessiveMethodLength (149>100) Added @SuppressWarnings(PHPMD.NPathComplexity) + @SuppressWarnings(PHPMD.ExcessiveMethodLength) docblock annotations — matches existing repo convention (e.g. Plugin_Header_Fields_Check::run())
2 tests/phpunit/tests/Traits/File_Editor_URL_Tests.php:341 Unexpected deprecation notice for legacy hooks (..._file_editor_url_template, ..._file_path) Added @expectedDeprecated ×2 annotations — WP test harness records the deprecated-hook shims as intended

Tests: File_Editor_URL_Tests 7/7 OK (single-site + multisite) · Verification: both fixes map 1:1 to their CI failure · CodeRabbit: no findings · PHP 7.4 compatible

Both changes are docblock-only — no behavior change, public signature and all return values preserved.

@davidperezgar
davidperezgar merged commit a30f94b into WordPress:trunk Aug 3, 2026
28 checks passed
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.

Refactor the File_Editor_URL Traits to use a single filter instead of two.

4 participants