[FIX] Issue Make Laminas Filters callable, and compatible with laminas-filters >=3.5.0 #38610
Conversation
|
Hi @Bashev. Thank you for your contribution! Add the comment under your pull request to deploy test or vanilla Magento instance:
❗ Automated tests can be triggered manually with an appropriate comment:
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
@magento run all tests |
|
@magento give me test instance |
|
Hi @Bashev. Thank you for your request. I'm working on Magento instance for you. |
Bashev
changed the title
|
Hi @Bashev, here is your Magento Instance: https://227c4874fb5254b35d67701b26cecf82.instances-prod.magento-community.engineering |
|
@magento run all tests |
|
Shouldn't we fix this in laminas-filter itself? In my opinion the change introduced in https://github.com/laminas/laminas-filter/pull/124/files#diff-3a6c9663e22963eb296b993e0b7cc8335c9f7f02f8364fc66385ed7175b5639dL89 is not done in a backwards compatible way and should be considered a new bug that needs to be fixed. What do you think @Bashev? |
|
Can we have an upstream test case that highlights the BC boundary, please? If the test passes before and fails afterwards, we can act and prevent a regression too, but it meeds to be done with some structure. |
|
@Ocramius: I lack the time to send in a proper PR at the moment, but after some quick testing locally, here's a way to demonstrate the problem, apply these changes and run phpunit and you'll see it fail: diff --git a/test/FilterChainTest.php b/test/FilterChainTest.php
index 33b3df7f..8d7fc58b 100644
--- a/test/FilterChainTest.php
+++ b/test/FilterChainTest.php
@@ -6,6 +6,7 @@ namespace LaminasTest\Filter;
use ArrayIterator;
use Laminas\Filter\FilterChain;
+use Laminas\Filter\FilterInterface;
use Laminas\Filter\PregReplace;
use Laminas\Filter\StringToLower;
use Laminas\Filter\StringTrim;
@@ -116,7 +117,7 @@ class FilterChainTest extends TestCase
{
return [
'callbacks' => [
- ['callback' => [self::class, 'staticUcaseFilter']],
+ ['callback' => new TestFilter()],
[
'priority' => 10000,
'callback' => static fn(string $value): string => trim($value),
@@ -250,3 +251,11 @@ class FilterChainTest extends TestCase
], $filters);
}
}
+
+class TestFilter implements Filterinterface
+{
+ public function filter($value)
+ {
+ return strtoupper($value);
+ }
+}Following change fixes the bug, but not sure if this is the best way: diff --git a/src/FilterChain.php b/src/FilterChain.php
index 4c092ed2..f776a4d5 100644
--- a/src/FilterChain.php
+++ b/src/FilterChain.php
@@ -87,7 +87,7 @@ class FilterChain extends AbstractFilter implements Countable, IteratorAggregate
foreach ($value as $spec) {
$callback = $spec['callback'] ?? false;
$priority = $spec['priority'] ?? static::DEFAULT_PRIORITY;
- if (is_callable($callback)) {
+ if (is_callable($callback) || $callback instanceof FilterInterface) {
$this->attach($callback, $priority);
}
}
@@ -172,12 +172,6 @@ class FilterChain extends AbstractFilter implements Countable, IteratorAggregate
public function attach($callback, $priority = self::DEFAULT_PRIORITY)
{
if (! is_callable($callback)) {
- if (! $callback instanceof FilterInterface) {
- throw new Exception\InvalidArgumentException(sprintf(
- 'Expected a valid PHP callback; received "%s"',
- get_debug_type($callback)
- ));
- }
$callback = [$callback, 'filter'];
}
$this->filters->insert($callback, $priority); |
|
@hostep it's make sense if can be fixed on laminas side, because this will help to most of the projects which using laminas-filter. My approach is different - if i must wait on additional vendor, to find a way to fix it on my side, but in this case, we must wait magento for fix and laminas for fix ... |
|
Fyi, if there's a patch+fix on our end it takes few seconds to tag a release, so please send a PR 😁 |
|
Here you go: laminas/laminas-filter#140 Let's move the discussion over there. |
|
Issue was fixed in laminas-filter and this PR is not required anymore. To fix it in current magento installations, just run |
|
@Bashev Колега, много често те мяркам тук напоследък, евала за свършената работа! :) |
Affected versions: Magento >=2.4.6-p4, 2.4.7
With 2.4.6-p4 laminas/laminas-filters was updated to version 3.5.1, but there is introduced additional validation of the filter classes, which checks is it callable. https://github.com/laminas/laminas-filter/blob/f98ddcfe5b145d84ccfe3cddd95ba60d09231843/src/FilterChain.php#L90
Because of that filtering from magento not works (skipped).
This PR add AbstractFilter (from laminas) where magic __invoke method is implemented.
This also affects all other date fields (like special price from/to, news from/to date) and etc, but not only...
Description (*)
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
A.1 Go to Reports - Sales - Orders (for example)
A.2 Try to filter some orders select some random dates.
A.3 Generate report.
B.1 Go to Catalog - Products
B.2 Create new product or edit existing product
B.3 Add "News from date" and/or "news to date" with random dates.
B.4 Save
Questions or comments
Contribution checklist (*)