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

Allow applying further filters after DoctrineProxyFilter #175

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

fsevestre
Copy link
Contributor

Backport PR #101 and #133 (based issue #98) on version 1, after discussions on #173.

I continue to think that being unable to apply 2 filters with Doctrine Proxy is a bug.
If you think it's a BC break, I can create a new DoctrineProxyFilter class and depreciate the existing class, but I'm not sure how to name it.

@fsevestre
Copy link
Contributor Author

Hello, sorry to ping you @mnapoli, I have errors on prod env while copying Doctrine entities because of that issue :(
Can you please review it ?

@mnapoli
Copy link
Member

mnapoli commented Oct 15, 2022

Hi! To be honest, I am not sure about the impact of this change, and whether it could cause backward compatibility issues.

Introducing a new (copied) class sounds like a big addition too. Is there any other solution we can do to increase confidence here? What is your own degree of confidence on this change?

Usually I'm fine trying things out, but this package is used very very widely, I'd like to avoid taking too much risk.

@fsevestre
Copy link
Contributor Author

Hello :) As you stated, this package is very used and even if I'm confident with the fix on itself, it might indeed have side effects.

Working on the 2.0 version might be a solution (the fix is already there) but I'm totally fine with what you said here #173 (comment)

What I can propose is to only add the ChainableFilter interface in this PR and I revert the DoctrineProxyFilter to his initial Filter interface.
On my side, as a user, I would be OK to add on my projects my own DoctrineProxyFilter implementation (implementing the new interface). It's not perfect, but I would be able to fix my issue.

What do you think ?

@fsevestre
Copy link
Contributor Author

Hello again @mnapoli

I have finally some time to work on this PR and I have 2 proposals:

  • From my previous comment: I only add the ChainableFilter interface but no existing filter will implement it. To fix my issue, I will have to create my own DoctrineProxyFilter implementing this interface.
  • Instead of an interface, ChainableFilter will be a decorator class. For example:
$deepCopy = new DeepCopy();
$deepCopy->addFilter(new ChainableFilter(new DoctrineProxyFilter()), new DoctrineProxyMatcher());

What do you think?

@mnapoli
Copy link
Member

mnapoli commented Mar 3, 2023

I think the second option sounds good 👍

@fsevestre
Copy link
Contributor Author

Updated the PR with the second option.
The CI failed, but the errors seems unrelated to the changes.

@mnapoli
Copy link
Member

mnapoli commented Mar 8, 2023

Thanks!

@mnapoli mnapoli merged commit 7284c22 into myclabs:1.x Mar 8, 2023
@fsevestre fsevestre deleted the 173-chainable-filter branch March 8, 2023 13:32
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