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

Fixes regression in FilterChain options, allows callback to be an instance of FilterInterface again. #140

Merged

Conversation

hostep
Copy link

@hostep hostep commented Apr 11, 2024

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

Fixes regression bug introduced by https://github.com/laminas/laminas-filter/pull/124/files#diff-3a6c9663e22963eb296b993e0b7cc8335c9f7f02f8364fc66385ed7175b5639dL89
The attach method can take a callable or a FilterInterface, however, with the added check on only is_callable, this breaks applying filters from classes that implement FilterInterface

See magento/magento2#38610 for how this breaks in Magento

This PR fixes this.

I tried to fix new problems psalm reports, but am unable to fix them, some help would be appreciated! Update: fixed, somehow the psalm cache was corrupt during my tests, so my initial fix did work in the end.

@baldwinagency-pieter baldwinagency-pieter force-pushed the fix-regression-with-filterinterface-235x branch from bfdaf80 to ea501a5 Compare April 11, 2024 07:24
@hostep
Copy link
Author

hostep commented Apr 11, 2024

@Ocramius: all tests are green, can be reviewed, thanks! :)

@Xerkus Xerkus added the Bug Something isn't working label Apr 11, 2024
@Xerkus Xerkus added this to the 2.35.2 milestone Apr 11, 2024
Copy link
Member

@Xerkus Xerkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#124 did introduce a BC break.

In context it is not intuitive but attach() method is accepting FilterInterface.

@Xerkus Xerkus changed the title Fixes regression, Allow callbacks to be a FilterInterface again. Fixes regression in FilterChain options, allows callback to be an instance of FilterInterface again. Apr 11, 2024
@Xerkus Xerkus merged commit 3e821b3 into laminas:2.35.x Apr 11, 2024
11 checks passed
@Xerkus
Copy link
Member

Xerkus commented Apr 11, 2024

@hostep thank you

@hostep
Copy link
Author

hostep commented Apr 11, 2024

Thanks for the quick merge and release!

@gsteel
Copy link
Member

gsteel commented Apr 11, 2024

Thanks for fixing my mistake @hostep 🤦‍♂️

@mortenbirkelund
Copy link

@hostep Nice work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants