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

Instrument usage of bug with iteration of String with offset or 0 limit #1667

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

dylanahsmith
Copy link
Contributor

Problem

Looks like 01dea94 introduced a bugs from trying to preserve ruby 1.8.7 behaviour. One that I think seems worth trying to fix is the special case of the limit and offset not applying to a String, which seems like enough of an edge case that it may not be used in practice.

The other regression from that commit demonstrates how little usage there was of String iteration at the time, since in ruby 1.8.7 liquid/ruby would iterate over lines of a String (i.e. String#each would split on the default separator $/).

Solution

Use Liquid::Usage.instrument to see if any code iterates of a String with a positive offset or 0 limit. That way we can decide whether we can fix this buggy behaviour or need to preserve it for backwards compatibility.

@dylanahsmith dylanahsmith merged commit c743936 into master Jan 11, 2023
@dylanahsmith dylanahsmith deleted the instrument-string-slice-bug-usage branch January 11, 2023 19:06
dylanahsmith added a commit that referenced this pull request Jan 17, 2023
@dylanahsmith
Copy link
Contributor Author

dylanahsmith commented Jan 17, 2023

The instrumentation shows there are a couple hundred uses of per-hour, so it looks like existing code already depends on this bug.

Unfortunately, it is hard to dig into the usage (with Liquid::Usage.instrument) to get a concrete example and see if the scope of the bug can be reduced (E.g. Is it the from / to parameter that needs to trigger the bug? Is the bug needed for both the for and tablerow tags? How many themes actually depend on this bug?). Perhaps we should have a different Liquid::Usage method that takes more arguments to we can more easily extract the concrete usage.

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.

2 participants