Skip to content

Conversation

@zachgarwood
Copy link
Member

@zachgarwood zachgarwood commented Jul 17, 2025

The short description is that getUft8Slug() was guessing the wrong encoding and erroneously transcoding the given string.

For the long description, see this Twill PR: area17/twill#2771.

@github-actions
Copy link

github-actions bot commented Jul 17, 2025

Automated PR Summary

The PR modifies the getUtf8Slug function within StringHelpers.php. It adjusts the handling of character encoding conversion by allowing an optional 'from_encoding' parameter to be specified in the options array. If 'from_encoding' is not provided, it defaults to converting based on all supported encodings, similar to the original behavior.

Potential Bugs

  1. Default 'from_encoding' behavior ambiguity: The original implementation passed all available encodings to mb_convert_encoding by default. The new implementation defaults to null if 'from_encoding' is not specified, which could potentially change the behavior if mb_convert_encoding interprets null differently than an explicit list of encodings.
  2. Handling of unsupported encodings: Since 'from_encoding' is now configurable, there's a risk that an unsupported encoding could be passed, leading to runtime errors if not properly handled or validated.
  3. Inconsistent application of conversion: The refactored code applies the conversion inside the condition that checks if 'from_encoding' is set in options. If 'from_encoding' is mistakenly omitted or set to null, the string conversion might not occur as intended, which could lead to issues with strings that require specific encoding treatments.

@deepsource-io
Copy link

deepsource-io bot commented Jul 17, 2025

Here's the code health analysis summary for commits 438da10..19c3e06. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource PHP LogoPHP❌ Failure
❗ 18 occurences introduced
View Check ↗
DeepSource JavaScript LogoJavaScript✅ SuccessView Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

@sonarqubecloud
Copy link

@zachgarwood zachgarwood marked this pull request as ready for review July 17, 2025 16:53
Copy link
Member

@nikhiltri nikhiltri left a comment

Choose a reason for hiding this comment

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

Looks good! ⭐

@zachgarwood zachgarwood merged commit e6828ef into develop Jul 18, 2025
13 of 14 checks passed
@zachgarwood zachgarwood deleted the fix/slug-encoding branch July 18, 2025 16:34
web-dev-trev pushed a commit that referenced this pull request Jul 23, 2025
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.

3 participants