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

handle/handleize filter doesn't return a Liquid-consistent value #424

Closed
deanebarker opened this issue Dec 10, 2021 · 7 comments · Fixed by #426
Closed

handle/handleize filter doesn't return a Liquid-consistent value #424

deanebarker opened this issue Dec 10, 2021 · 7 comments · Fixed by #426
Assignees

Comments

@deanebarker
Copy link
Contributor

The example on this page of the Liquid doc claims this --

{{ '100% M & Ms!!!' | handle }}

-- should output this:

100-m-ms

In Fluid v2.2.8, it does not. It outputs:

100% m & ms!!!

The Fluid implementation seems to be missing:

  1. Removal of punctuation
  2. Replacement of spaces with dashes
@deanebarker
Copy link
Contributor Author

I'm happy to write a PR for this, for the record, but I don't understand the regex magic that's happening in there. If I write this, I'm just gonna iterate the characters via a Select or something.

@hishamco
Copy link
Collaborator

Please submit a PR, then we can review it

Thanks

@deanebarker
Copy link
Contributor Author

deanebarker commented Dec 11, 2021

Okay, this is not the PR or the actual function, but this is my logic. Before I go any further, let me know if this is fine.

(It's over-commented for clarity.)

string Handle(string input)
{
	const char SPACE = ' ';
	const char DASH = '-';
	
	var validChars = new List<char>();
	foreach(var character in input)
	{	
		// Only allow letters, numbers, spaces, or dashes
		if(!Char.IsLetterOrDigit(character) && character != SPACE && character != DASH)
		{
			continue;
		}
		
		// Don't allow consecutive spaces
		if(character == SPACE && validChars.LastOrDefault() == SPACE)
		{
			continue;
		}
		
		// Convert any whitespace (tabs, newlines, whatever) to a single space
		if(Char.IsWhiteSpace(character))
		{
			validChars.Add(SPACE);
			continue;
		}
		
		// Allow the character
		validChars.Add(character);
	}
	
	var newString = new String(validChars.ToArray());
	newString = newString.Trim(); // Clean whitespace
	newString = newString.Replace(SPACE, DASH); // Spaces become dashs
	newString = newString.ToLower(); // Everthing is lower-case
	
	return newString;
}

@deanebarker
Copy link
Contributor Author

deanebarker commented Dec 11, 2021

I fuzzed it and found a bug, which I corrected above (Last() needed to be LastOrDefault())

Incidentally, on my desktop machine I ran it on 10,000 random 30-character strings and it took 5ms total.

@hishamco hishamco self-assigned this Dec 11, 2021
@hishamco
Copy link
Collaborator

Just a quick not StringBuilder is a better option for string manipulation, If you don't plan to create a PR, I will create a unit test first that fails, then I can start the PR

@deanebarker
Copy link
Contributor Author

deanebarker commented Dec 11, 2021

I did that at first. I can't check the last character with a StringBuilder, can I?

@deanebarker
Copy link
Contributor Author

Here, I re-wrote it with a StringBuilder.

string Handle(string input)
{
	const char SPACE = ' ';
	const char DASH = '-';
	
	var validChars = new StringBuilder();
	var lastCharacter = '\0';
	foreach(var character in input)
	{	
		// Only allow letters, numbers, spaces, or dashes
		if(!Char.IsLetterOrDigit(character) && character != SPACE && character != DASH)
		{
			continue;
		}
		
		// Don't allow consecutive spaces
		if(character == SPACE && lastCharacter == SPACE)
		{
			continue;
		}
		
		// Convert any whitespace (tabs, newlines, whatever) to a single space
		if(Char.IsWhiteSpace(character))
		{
			lastCharacter = character;
			validChars.Append(SPACE);
			continue;
		}
		
		// Allow the character
		validChars.Append(character);
		lastCharacter = character;
	}
	
	var newString = validChars.ToString();
	newString = newString.Trim(); // Clean whitespace
	newString = newString.Replace(SPACE, DASH); // Spaces become dashs
	newString = newString.ToLower(); // Everthing is lower-case
	
	return newString;
}

@hishamco hishamco changed the title handle/handlesize filter doesn't return a Liquid-consistent value handle/handleize filter doesn't return a Liquid-consistent value Dec 11, 2021
sebastienros pushed a commit that referenced this issue Jan 2, 2022
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 a pull request may close this issue.

2 participants