Fix filter compatibility's by ryo-morimoto · Pull Request #236 · kalimatas/php-liquid

ryo-morimoto · 2025-12-01T20:30:11Z

I've run the tests with vendor/bin/phpunit
None of the tests were found failing
I've seen the coverage report at build/coverage/index.html
Not a single line left uncovered by tests
Any coding standards issues were fixed with vendor/bin/php-cs-fixer fix

Fix #233 , #234

Changes

capitalize

Before: "hello WORLD" → "Hello WORLD"
After: "hello WORLD" → "Hello world"

plus, minus, times, modulo, divided_by

Before: always returns float
After: returns int when both operands are integers

map

Single hash and nested arrays are now handled correctly

What I'd like reviewed

src/Liquid/Liquid.php::isHash,isInteger
I'm currently adding utility functions directly to Liquid/Liquid.
I did consider making a separate Liquid/Util module, but I generally avoid creating modules with such broad, generic purposes.
Given the small scope right now, I think keeping them in Liquid/Liquid is fine for the time being.

However, if this section grows significantly, it will definitely require more maintenance. So, I'd appreciate your decision on how to proceed.

ryo-morimoto · 2025-12-01T21:12:33Z

It seems difficult to replicate Ruby's hash behavior in PHP. I will spend a little more time on it, but I think I will separate the PR for associative array handling.

sanmai · 2025-12-01T22:20:20Z

Thanks for the PR! LMK when you're ready for a review.

Copilot

Pull request overview

This PR fixes compatibility issues with Ruby's Liquid implementation by updating several filters to match their Ruby counterparts. It addresses issues #233 and #234 by modifying the behavior of the capitalize filter, arithmetic filters (plus, minus, times, modulo, divided_by), and the map filter. The changes include adding utility functions isHash and isInteger to the Liquid class to support proper type detection.

Key Changes:

Modified capitalize to only capitalize the first character and lowercase the rest (instead of capitalizing each word)
Updated arithmetic filters to return integers when both operands are integers, and floats otherwise
Fixed map filter to properly handle single hashes and nested arrays by adding hash detection and selective flattening

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
src/Liquid/StandardFilters.php	Updated `capitalize`, arithmetic filters (`plus`, `minus`, `times`, `modulo`, `divided_by`), and `map` filter for Ruby Liquid compatibility
src/Liquid/Liquid.php	Added utility methods `isHash`, `isInteger`, and modified `arrayFlatten` to support hash-aware flattening
tests/Liquid/LiquidTest.php	Added comprehensive tests for new `isHash` and `isInteger` utility methods, and `arrayFlatten` with skipHash parameter
tests/Liquid/StandardFiltersTest.php	Updated tests for `capitalize` behavior and added extensive test coverage for arithmetic filters with both integer and float operands

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-01T22:25:26Z

src/Liquid/StandardFilters.php

 	{
-		return (float)$input / (float)$operand;
+		if (Liquid::isInteger($input) && Liquid::isInteger($operand)) {
+			return (int) floor($input / $operand);


Using floor() for integer division produces incorrect results for negative numbers. For example, floor(-14 / 3) returns -5 instead of the expected -4 (which PHP's integer division operator intdiv() or truncation would return). Consider using intdiv($input, $operand) for PHP 7.0+, or (int)($input / $operand) for consistent truncation behavior that matches most programming languages' integer division.

Suggested change

return (int) floor($input / $operand);

return intdiv($input, $operand);

Copilot · 2025-12-01T22:25:26Z

src/Liquid/Liquid.php

+			$trimmed = ltrim($value, '-');
+			return $trimmed !== '' && ctype_digit($trimmed);


The ltrim($value, '-') approach is unsafe for edge cases. For the string "-", ltrim removes the minus sign leaving an empty string, but the check $trimmed !== '' correctly returns false. However, consider strings like "--5" or "-+5" which would pass the empty check but fail ctype_digit(). While ctype_digit() will catch these, a cleaner approach would be to use a regex like /^-?\d+$/ which is more explicit and handles all cases in one check.

Suggested change

$trimmed = ltrim($value, '-');

return $trimmed !== '' && ctype_digit($trimmed);

return preg_match('/^-?\d+$/', $value) === 1;

Copilot · 2025-12-01T22:25:27Z

src/Liquid/Liquid.php

+		if (empty($array)) {
+			return false;
+		}
+		return array_keys($array) !== range(0, count($array) - 1);


[nitpick] Using !== for array comparison may be inefficient for large arrays. While this works correctly, array_keys($array) !== range(0, count($array) - 1) creates and compares two potentially large arrays. For PHP 8.1+, consider using the native !array_is_list($array) function with a version check fallback. Alternatively, iterate through keys and return early on the first non-sequential key for better performance.

Suggested change

return array_keys($array) !== range(0, count($array) - 1);

// Use native array_is_list() if available (PHP 8.1+)

if (function_exists('array_is_list')) {

return !array_is_list($array);

}

// Manual check for sequential integer keys starting from 0

$i = 0;

foreach ($array as $key => $_) {

if ($key !== $i++) {

return true;

}

}

return false;

Copilot · 2025-12-01T22:25:27Z

src/Liquid/StandardFilters.php

 	public static function divided_by($input, $operand)
 	{
-		return (float)$input / (float)$operand;
+		if (Liquid::isInteger($input) && Liquid::isInteger($operand)) {
+			return (int) floor($input / $operand);
+		}
+		return (float) $input / (float) $operand;
 	}


No division by zero handling in divided_by filter. When $operand is 0 (or evaluates to 0), this will result in a PHP warning/error and potentially INF or NAN. Consider adding a check and returning a sensible default (like 0 or null) or throwing a more descriptive exception, depending on how Ruby's Liquid handles this case.

Copilot · 2025-12-01T22:25:27Z

src/Liquid/StandardFilters.php

 	public static function modulo($input, $operand)
 	{
+		if (Liquid::isInteger($input) && Liquid::isInteger($operand)) {
+			return (int)$input % (int)$operand;
+		}
 		return fmod((float)$input, (float)$operand);
 	}


No modulo by zero handling in modulo filter. When $operand is 0 (or evaluates to 0), this will result in a PHP warning "Division by zero" for the integer path (line 379) or undefined behavior for fmod(). Consider adding a check and handling this edge case appropriately, consistent with Ruby's Liquid behavior.

Copilot · 2025-12-01T22:25:28Z

src/Liquid/StandardFilters.php

+
+		// Flatten nested arrays while preserving hashes
+		// [[['attr' => 1]]] => [['attr' => 1]]
+		$input = Liquid::arrayFlatten($input, true);


map now flattens nested arrays via Liquid::arrayFlatten($input, true) before iterating, which increases the chance that elements evaluated below as callables (is_callable($elem)) will be surfaced and executed. An attacker controlling input can place nested PHP callables (e.g., "phpinfo", [ClassName, "method"], or a closure) so they are flattened and then invoked, enabling arbitrary function execution from templates. Remove callable invocation or strictly whitelist safe callables; e.g., delete the is_callable branch or change it to only allow vetted closures you create, and avoid flattening untrusted arrays before iteration:

// Either remove callable execution entirely return array_map(function ($elem) use ($property) { // no is_callable branch here // ... only property extraction logic }, $input); // Or, if callable support is strictly needed, restrict: if ($elem instanceof \Closure && $this->isTrustedClosure($elem)) { return $elem(); }

sanmai · 2025-12-02T23:42:42Z

Will it help if I'd merge #237?

ryo-morimoto · 2025-12-04T06:00:22Z

@sanmai That would be greatly appreciated, thank you!

sanmai · 2025-12-04T06:41:55Z

Done! Feel free to merge with the main branch.

ryo-morimoto · 2025-12-17T21:14:33Z

My apologies for the silence. I'll get a PR ready this weekend, organizing what can be merged first.

ryo-morimoto added 6 commits November 30, 2025 23:56

Add test for capitalize first character only

ecf3c51

Add test for divided_by integer division

68ff565

Add test for map single hash and nested array flatten

5f9dd01

Fix map filter to handle single hash and nested arrays

3175b1b

Fix math filters to preserve integer type for integer operands

7c5d4c3

Fix capitalize filter to uppercase first character only

d6ca7fb

sanmai requested a review from Copilot December 1, 2025 22:19

Copilot started reviewing on behalf of sanmai December 1, 2025 22:20 View session

Copilot finished reviewing on behalf of sanmai December 1, 2025 22:22

Copilot AI reviewed Dec 1, 2025

View reviewed changes

	return (int) floor($input / $operand);
	return intdiv($input, $operand);

		$trimmed = ltrim($value, '-');
		return $trimmed !== '' && ctype_digit($trimmed);

	$trimmed = ltrim($value, '-');
	return $trimmed !== '' && ctype_digit($trimmed);
	return preg_match('/^-?\d+$/', $value) === 1;

-		return array_keys($array) !== range(0, count($array) - 1);
+		// Use native array_is_list() if available (PHP 8.1+)
+		if (function_exists('array_is_list')) {
+			return !array_is_list($array);
+		}
+		// Manual check for sequential integer keys starting from 0
+		$i = 0;
+		foreach ($array as $key => $_) {
+			if ($key !== $i++) {
+				return true;
+			}
+		}
+		return false;

Conversation

ryo-morimoto commented Dec 1, 2025

Changes

What I'd like reviewed

Uh oh!

ryo-morimoto commented Dec 1, 2025

Uh oh!

sanmai commented Dec 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

sanmai commented Dec 2, 2025

Uh oh!

ryo-morimoto commented Dec 4, 2025

Uh oh!

sanmai commented Dec 4, 2025

Uh oh!

ryo-morimoto commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants