Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set specific flags for html_entity_decode #2771

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

mikkamp
Copy link
Contributor

@mikkamp mikkamp commented Jan 16, 2025

Changes proposed in this Pull Request:

This PR resolves the following warning notices:

The default value of the $flags parameter for html_entity_decode() was changed from ENT_COMPAT to ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML401 in PHP 8.1. For cross-version compatibility, the $flags parameter should be explicitly set.

We only use the function html_entity_decode in two locations:

  1. To decode currency symbols (generally we don't expect this to contain quotes, so the behaviour should remain the same regardless of PHP version)
  2. For product names that show up in the issues table. We are already escaping these strings before inserting in the DB as well as decoding before displaying, so the change doesn't affect any behaviour

I was considering adding a unit test, but because the behaviour is identical, and what we are really concerned about is that the end display shows the product names without encoded entities, so instead I went with a manual visual test to confirm the behaviour remains consistent.

Closes #2770

Detailed test instructions:

  1. Add a product with a name containing single and double quotes, ex: Product "name" which shouldn't cause problems
  2. Make sure to leave some requirements out like adding a description or image so it ends up in the product issues table
  3. Go to the connection test page and "Clear Status Cache"
  4. Go to the Product Feed page and wait for the statuses to refresh
  5. Check the DB table wp_gla_merchant_issues and confirm the product name column contains the quote characters as expected
  6. View the product issues table on the product feed page and confirm the name contains the quotes as expected (can drop some rows from the DB table if there are too many to display)

image

Changelog entry

  • Tweak - Set specific flags for html_entity_decode.

@mikkamp mikkamp self-assigned this Jan 16, 2025
@github-actions github-actions bot added type: bug The issue is a confirmed bug. changelog: fix Took care of something that wasn't working. labels Jan 16, 2025
@mikkamp mikkamp requested a review from a team January 16, 2025 14:13
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.2%. Comparing base (f88648c) to head (c92acd5).
Report is 2 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #2771      +/-   ##
============================================
+ Coverage       66.7%   67.2%    +0.6%     
- Complexity         0    4677    +4677     
============================================
  Files            316     480     +164     
  Lines           4922   19571   +14649     
  Branches        1204       0    -1204     
============================================
+ Hits            3282   13159    +9877     
- Misses          1503    6412    +4909     
+ Partials         137       0     -137     
Flag Coverage Δ
js-unit-tests ?
php-unit-tests 67.2% <100.0%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/Ads/AccountService.php 98.7% <100.0%> (ø)
src/MerchantCenter/MerchantStatuses.php 76.2% <100.0%> (ø)

... and 794 files with indirect coverage changes

Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! The code works as expected. LGTM.

@mikkamp mikkamp merged commit a80bf4a into develop Jan 17, 2025
14 checks passed
@mikkamp mikkamp deleted the fix/2770-html-entity-decode branch January 17, 2025 08:22
@ianlin ianlin mentioned this pull request Jan 21, 2025
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP Compatibility warnings from QIT code scan
2 participants