Skip to content

fix(Helpers): prevent TypeError in excerpt() when phrase is not found or radius is odd - #10756

Open
banghamid wants to merge 2 commits into
codeigniter4:developfrom
banghamid:fix/excerpt-type-error
Open

banghamid wants to merge 2 commits into
codeigniter4:developfrom
banghamid:fix/excerpt-type-error

Conversation

@banghamid

@banghamid banghamid commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Description
Because system/Helpers/text_helper.php uses declare(strict_types=1), excerpt() previously threw an unhandled TypeError in two scenarios:

  1. When $phrase was passed but did not exist in $text, mb_stripos($text, $phrase) returned false, causing mb_substr($text, 0, $phrasePosition) to fail with TypeError: mb_substr(): Argument #3 ($length) must be of type ?int, bool given.
  2. When $phrase was null and $radius was an odd integer (e.g., 101), $radius / 2 evaluated to float (50.5), causing mb_substr($text, 0, $phrasePosition) to fail with TypeError: mb_substr(): Argument #3 ($length) must be of type ?int, float given.

This PR:

  • Falls back to excerpt($text, null, $radius, $ellipsis) (extracting from the beginning of $text) when $phrasePosition === false.
  • Uses intdiv($radius, 2) so $phrasePosition is always an integer.
  • Updates docs/helpers/text_helper.md to document the fallback behavior when $phrase is not found.
  • Adds regression tests in TextHelperTest (testExcerptPhraseNotFound() and testExcerptOddRadius()).

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@carson-codeigniter4 carson-codeigniter4 Bot added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 10, 2026

@neznaika0 neznaika0 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.

If you are approved to change excerpt() by running the command again, this is considered a change. It should be written in docs.

Comment thread system/Helpers/text_helper.php Resolved
Comment thread tests/system/Helpers/TextHelperTest.php Resolved
@banghamid
banghamid requested a review from neznaika0 November 10, 2026 17:43
@gr8man

gr8man commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Overall, this is a solid and much-needed fix. Resolving the TypeError caused by strict_types=1 is critical for production stability.

Here is feedback addressing the review comments and the PR:

1. Fallback vs. Throwing an Exception

Falling back to extracting from the beginning (excerpt($text, null, $radius, $ellipsis)) is the right approach here:

  • Target Branch (develop): Throwing an exception (e.g. InvalidArgumentException) when a phrase is missing would be a breaking change for existing applications and view templates on a patch branch.
  • Helper Purpose: Text helpers are presentation utilities commonly used in views and parser filters ({ value|excerpt(...) }). If a dynamic search term is not found in the body text, gracefully falling back to a leading excerpt prevents a 500 error / page crash for end users.

2. The Missing Character Bug (It sad So)

The dropped character and the leading space when $phrase === null is indeed a longstanding algorithmic flaw (it dates back to treating the midpoint as a 1-char "phrase" and slicing it out). However:

  • Fixing that algorithm would change the output contract and invalidate existing assertions in testExcerpt().
  • Following CI4's single-responsibility guidelines for PRs, that bug should be tracked and fixed in a separate, dedicated PR so this bugfix can land cleanly.

3. Suggestions

  1. Use intdiv(): In system/Helpers/text_helper.php, consider using intdiv() instead of casting a float:
    $phrasePosition = intdiv($radius, 2);

@banghamid

Copy link
Copy Markdown
Contributor Author

Thanks for the review @neznaika0 and @gr8man!

I pushed a small update:

  • Switched (int) ($radius / 2) to intdiv($radius, 2) as suggested.
  • Updated docs/helpers/text_helper.md (and the docblock) to note the fallback behavior when $phrase is not found in $text.

Agreed on keeping the missing-character quirk ($phrase === null) out of this PR so we don't break existing test expectations on develop. Let me know if anything else looks off.

@banghamid
banghamid requested a review from gr8man November 11, 2026 17:53

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants