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

FIX Use empty array as a fallback for preg_split within DBText summary #9696

Merged
merged 3 commits into from
Jun 3, 2021

Conversation

michalkleiner
Copy link
Contributor

@michalkleiner michalkleiner commented Sep 16, 2020

If the content is invalid for whatever reason e.g. when instantiating a DBText field to get a summary of text through DBField::create_field('Text', $content)->Summary(10) from a UTF-malformed string, preg_split returns false and the rest of the code expects an array.

This fix ensures an array is always returned even when preg_split fails.

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

All of these are parts that are immediately expecting iterable results, so LGTM. Thanks!

@sminnee
Copy link
Member

sminnee commented Sep 16, 2020

The only thing I'd suggest would be a test that fails without this patch, to ensure we don't regress.

@michalkleiner
Copy link
Contributor Author

Is there anywhere a test already dealing with invalid UTF string that I could look at?

@bergice
Copy link
Contributor

bergice commented May 21, 2021

Is there anywhere a test already dealing with invalid UTF string that I could look at?

\SilverStripe\ORM\Tests\DBTextTest::testValidUtf8 seems like a good option.

Copy link
Contributor

@dhensby dhensby left a comment

Choose a reason for hiding this comment

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

I agree a test would be good, but we shouldn't prevent a merge that is clearly a good enhancement just because existing testing doesn't cover a fairly obvious failure case.

@michalkleiner michalkleiner force-pushed the pulls/safer-dbtext-summary branch from 404f300 to 531140c Compare May 23, 2021 11:27
@michalkleiner michalkleiner changed the base branch from 4.6 to 4.8 May 23, 2021 11:34
@michalkleiner
Copy link
Contributor Author

Rebased on 4.8 and added a test for DBText->Summary, but Travis fails on version resolution ¯\(ツ)/¯ before it can run it.

If the content is invalid for whatever reason e.g. when instantiating
a DBText field to get a summary of text through
`DBField::create_field('Text', $content)->Summary(10)`, preg_split returns
false and the rest of the code expects an array.
This tweak ensures an array is always returned even when preg_split fails.
@michalkleiner michalkleiner force-pushed the pulls/safer-dbtext-summary branch 2 times, most recently from b096452 to 9cc23c9 Compare June 2, 2021 03:53
@dhensby
Copy link
Contributor

dhensby commented Jun 2, 2021

Thanks @michalkleiner - but since of the new tests are failing, are you happy to keep looking into it?

@michalkleiner
Copy link
Contributor Author

michalkleiner commented Jun 2, 2021

Yep @dhensby, will get it through ;-) The tests were not even running since I targetted 4.8, now they are after a rebase so can finally finish resolving this.

@michalkleiner
Copy link
Contributor Author

@dhensby all green now ;-)

@dhensby dhensby merged commit 12cffe0 into silverstripe:4.8 Jun 3, 2021
@michalkleiner michalkleiner deleted the pulls/safer-dbtext-summary branch June 3, 2021 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants