Skip to content

[Image Proxy] Replace deprecated HTML entity conversion#493

Merged
Frenzie merged 1 commit into
FreshRSS:mainfrom
JamBalaya56562:fix/image-proxy-html-entities-deprecation
Jul 12, 2026
Merged

[Image Proxy] Replace deprecated HTML entity conversion#493
Frenzie merged 1 commit into
FreshRSS:mainfrom
JamBalaya56562:fix/image-proxy-html-entities-deprecation

Conversation

@JamBalaya56562

Copy link
Copy Markdown
Contributor

Summary

  • replace the deprecated mb_convert_encoding(..., 'HTML-ENTITIES', ...) call with mb_encode_numericentity()
  • preserve PHP 8.1+ support and the existing DOMDocument processing path
  • add a dependency-free ImageProxy regression test runner with 19 assertions
  • run the regression suite from Composer and GitHub Actions
  • bump Image Proxy to 1.0.1

Fixes #225

Root cause

ImageProxy converts non-ASCII UTF-8 characters to HTML entities before passing article fragments to DOMDocument::loadHTML(). Using HTML-ENTITIES as an mb_convert_encoding() target has been deprecated since PHP 8.2, so every processed article could emit a deprecation warning.

mb_encode_numericentity() performs the required non-ASCII-to-entity conversion without using the deprecated pseudo-encoding. This keeps HTML tags and existing entities intact and avoids raising the minimum supported PHP version.

Regression coverage

The new standalone test runner converts E_DEPRECATED notices into exceptions and verifies:

  • ASCII, accented Latin text, Japanese, Chinese, emoji, and supplementary-plane characters
  • existing named, decimal, and hexadecimal entities without double encoding
  • invalid UTF-8 without discarding the complete article
  • empty input, HTML documents, fragments, tag soup, and element attributes
  • multiple images, src, srcset, and original-URL data attributes
  • HTTP, HTTPS, protocol-relative, encoded, and unsupported URI schemes

Validation

  • composer run test-image-proxy via Docker: 19 assertions passed
  • PHP syntax checks for the extension and test runner
  • npm run eslint
  • npx stylelint "**/*.css"
  • npm run markdownlint

PHPCS and PHPStan are validated by GitHub Actions.

@JamBalaya56562 JamBalaya56562 force-pushed the fix/image-proxy-html-entities-deprecation branch 2 times, most recently from 9d03c62 to 73c885a Compare July 12, 2026 03:59
@Frenzie

Frenzie commented Jul 12, 2026

Copy link
Copy Markdown
Member

I tried that already and it didn't work very well. I'd much rather just switch to the PHP 8.4 HTML parser.

@JamBalaya56562

Copy link
Copy Markdown
Contributor Author

Thanks for the context. I found the earlier discussion in #272, but not the exact patch or input that failed.

I tested the ²³⁸Umbrella policy example mentioned there with both conversions. In isolation, both survived the DOM round trip, so the failure may depend on the surrounding feed HTML.

Could you clarify two points?

  1. Do you still have a concrete feed entry or HTML fixture that demonstrates the failure? I would like to preserve it as a regression test.
  2. Would you prefer making ImageProxy require PHP 8.4 and replacing DOMDocument::loadHTML() entirely with Dom\HTMLDocument::createFromString(), or should we retain a fallback for PHP 8.1–8.3?

I agree that switching to the standards-compliant PHP 8.4 parser is preferable to keeping the legacy parser and only suppressing the deprecation warning. Once the compatibility expectation is clear, I can revise this PR accordingly.

@JamBalaya56562 JamBalaya56562 force-pushed the fix/image-proxy-html-entities-deprecation branch from 73c885a to b8ac3bc Compare July 12, 2026 07:51
@Frenzie

Frenzie commented Jul 12, 2026

Copy link
Copy Markdown
Member

I prefer to do nothing at all until such time as FreshRSS itself raises its version but in lieu of that switching over to PHP 8.4's HTML parser prematurely is still preferable over other changes imho.

@JamBalaya56562

Copy link
Copy Markdown
Contributor Author

Thanks, understood. I see the rationale for waiting and eventually replacing the parser itself instead of making an interim conversion change.

Even if the conversion change in this PR is not adopted, the regression tests added here might still be useful as a starting point for validating the future PHP 8.4 HTML parser migration. They could potentially be kept or reused separately when that work happens.

@Frenzie Frenzie merged commit d823b9f into FreshRSS:main Jul 12, 2026
1 check passed
@Frenzie

Frenzie commented Jul 12, 2026

Copy link
Copy Markdown
Member

I did some testing and I think this should be fine.

@JamBalaya56562 JamBalaya56562 deleted the fix/image-proxy-html-entities-deprecation branch July 12, 2026 13:59
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.

[ImageProxy] PHP Deprecated: mb_convert_encoding()

2 participants