Closes #883: Fix .htaccess permission notice shown twice and undefined variable in remove()#1069
Open
Miraeld wants to merge 5 commits into
Open
Conversation
… .htaccess permission notice Bug #883 introduced two issues when .htaccess is not writable: 1. AbstractWriteDirConfFile::remove() used \$file_path without assigning it first, triggering a PHP Warning: "Undefined variable \$file_path" on line 101. 2. Both Webp\RewriteRules\Display and Avif\RewriteRules\Display subscribe to the same imagify_settings_webp_info action and each printed the "file not writable" notice independently, causing it to appear twice. A static request-scoped flag now prevents the second subscriber from printing when the first already has. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The per-method static $notice_printed variables in each Display class were independent — each class's static was reset independently, so the second subscriber would still print. Move the flag to a public static property on AbstractWriteDirConfFile so both Webp\RewriteRules\Display and Avif\RewriteRules\Display share a single request-scoped gate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
- PR title: add hard negative in release-agent to prevent conventional commit format - CRITICAL routing: give orchestrator discretion to attempt fix before escalating - E2E suite: decouple from frontend agent, always run as regression check - tools: add WebFetch + WebSearch to grooming, challenger, lead-reviewer, backend, frontend - Collapsible step detail panels in HTML log format - Step detail panel content documented per-step in orchestrator Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ent timeline Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…acts, event timeline" This reverts commit e1bcb8f.
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
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
Fixes #883
When
.htaccessis not writable and the user enables the "Display next-gen" option, two bugs occur: the permission notice is shown twice in the settings UI, and disabling the option triggers a PHP Warning for an undefined variable.This PR fixes both issues:
AbstractWriteDirConfFile::remove()method used$file_pathwithout ever assigning it, causingPHP Warning: Undefined variable $file_path. A single line is added to call$this->get_file_path()before the variable is used — mirroring the pattern already present inadd().Webp\RewriteRules\Display::maybe_add_webp_info()andAvif\RewriteRules\Display::maybe_add_avif_info()subscribe independently to the sameimagify_settings_webp_infoaction and each printed the "file not writable" notice on their own. A shared static flagAbstractWriteDirConfFile::$writable_notice_printedis introduced so that whichever subscriber prints first will prevent the second from printing during the same request.Type of change
Detailed scenario
What was tested
To be filled by QA.
How to test
Acceptance criterion 1 — Permission notice appears only once:
.htaccesspermissions to444(read-only).Acceptance criterion 2 — No PHP Warning when disabling:
.htaccessset to444, enable Display next-gen (rewrite method).Undefined variable $file_pathappears in the PHP error log.Affected Features & Quality Assurance Scope
.htaccess/web.config/imagify.confwrite operations when the display next-gen option is toggled.Technical description
Documentation
Bug 1 fix (
AbstractWriteDirConfFile::remove(), line ~101):The
remove()method built an error message using$file_pathbut the variable was never assigned in that method (unlikeadd()which correctly assigned it on the previous line). Fix: add$file_path = $this->get_file_path();before themake_path_relative()call.Bug 2 fix (shared static flag):
AbstractWriteDirConfFilegains a newpublic static bool $writable_notice_printed = falseproperty. Bothmaybe_add_webp_info()andmaybe_add_avif_info()check this flag on entry to theis_wp_errorbranch and return early if it is alreadytrue. The first subscriber to print sets it totrue. Because the flag is on the shared abstract base class, it is truly shared across all concrete subclasses in the same request.New dependencies
None.
Risks
Displayclass hooksimagify_settings_webp_info, it will automatically benefit from the shared gate.maybe_add_webp_info()lives in a separateelseifbranch and is not affected by the flag.Mandatory Checklist
Code validation
Code style
Unticked items justification
N/A — all items checked.
Additional Checks