@diosmosis opened this Pull Request on November 25th 2018 Member

Fixes #13709

@tsteur commented on November 25th 2018 Member

For some reason I can't reproduce the notice... could you? I assume it is fine to merge. Not sure where/if we use $period . '' or something similar in any meaningful context besides debug logs.

@diosmosis commented on November 25th 2018 Member

It's reproducible in tests, I didn't try to reproduce through the UI, I think there must have been some error that occurs w/ a week or month period that might trigger it. Ie, something that actually triggers the $period->toString() method.

@tsteur commented on December 6th 2018 Member

some failing tests but otherwise good to merge 👍

@diosmosis commented on December 8th 2018 Member

@tsteur had to make a bunch of changes to the range period test and noticed that it might be possible for a plugin to use Period::toString() and expect arrays of arrays. Think this might be a BC issue?

@tsteur commented on December 9th 2018 Member

I don't think it is a BC concern but hard to say. But thinking about it again... a toString method should really not return arrays. Not sure how it is used... but ideally it would actually return a string and not arrays.

@diosmosis commented on December 9th 2018 Member

I think currently __toString() returns a string, and toString() returns an array of strings. Could try and change it to return a string, will have to see how much code uses it.

@tsteur commented on December 9th 2018 Member

All good, don't worry about it right now 👍

@tsteur commented on December 9th 2018 Member

Feel free to merge if it fixes the problem. Only usage I could find was SearchEngine feature that used the day period and it doesn't seem to regress there.

This Pull Request was closed on December 9th 2018
Powered by GitHub Issue Mirror