Add interfaces for easier testing

[gnat42]

The DB Finals bypass is not 100% reliable. Providing interfaces instead allows for mocking without additional complex hooks modifying the PHP environment.

This adds interfaces and default service aliases so there are no changes required.

[garak]

Thanks for your contribution. I have a few doubts:

• I don't see any change in tests. Is this supposed to get rid of the bypass-finals lib?
• Are we adapting the code to the tests, rather than the other way around?
• Are these changes breaking BC? It seems so. Then we can't merge this into master. I can create a v3 branch tho

[gnat42]

Hello,

I didn't change any of your tests, as this was mostly so that external consumers could write functional tests mocking the services provided in this bundle.

These changes allows someone to require your bundle, but not require the bypass-finals lib. I think they are BC, at most its a minor version bump because it introduces new interfaces.

I could modify/add tests that used the interfaces or tested for the existence of both services (via interface or service name).

What this allows is so that I as a consumer of your bundle can have functional tests and mock the UploadHandler/DownloadHandlers without breaking the fact that the class is final. I tried getting around this via composition / decorating the the required services. However, it becomes really difficult when also using and testing the forms.

These changes are additive, without disrupting any existing code using this same version.

You could still continue to use bypass-libs internally if you wanted in tests, OR, as I said, I'm completely happy to include removal of that necessity and converting the tests to mock the interfaces instead of using it. You let me know what you'd prefer.

From my perspective, a consumer of this bundle should be able to get the new version and it won't affect them. Here's my rationale, but let me know if I've missed anything.

• Your dev requirements is what requires bypass libs, for tests, currently that can stay if you prefer to use it.
  • If someone already requires this bundle, and includes bypass-libs, they can continue to.
  • If someone already requires this bundle, but has no tests mocking these services this doesn't affect them
• With respect to the bundle configuration, we've only added a new service alias for the interface->service map this means
  • Existing installations continue to function injecting the service via its name or typehint to the service classname
  • New or updated versions can optionally, typehint the interface and will get the same service because of the alias

Thoughts?

[garak]

My thoughts are that you generated your response with AI, and at this point maybe even the original PR
Everything seems logic, but the initial assumption of requiring the bypass-finals lib is completely wrong.

[gnat42]

Hello,

I can assure you none of what was written, or the PR were AI generated or assisted in any way. I really have no way of proving it to you. However you can check my github repos, and https://github.com/NobletSolutions for other non personal repos. I've written and maintained for quite awhile. I've submitted to symfony and other projects.

Regardless of that, the composer.json for this bundle has a dev requirement on bypass-libs as [seen here] (https://github.com/dustin10/VichUploaderBundle/blob/master/composer.json#L34). Like I said, if you want me to, I can make more changes to the PR, removing that requirement and using the interface instead.

Here's the diff where I upgrade the bundle in our project and use the newer bundle,change from annotations to attributes, and the modifications to have unit tests that mock the UploaderHandler and requirements for symfony form tests.

If you'd like, I can show you that a cache:clear works for a service with the UploadHandler directly injected or the UploadHandlerInterface so you'll know there are no BC breaks. However it does seem that for whatever reason you may not trust my contribution, so I'm not sure whether it would be something you value.

I'm hoping you can continue working with me on getting this merged in so that I don't have to maintain a fork that we install from and periodically have to rebase on the latest version of this bundle going forward.

Let me know what you'd like to do.

[garak]

OK, let's try to get it into two topics:

1. Why do we need to get rid of bypass-finals?
2. If we decide to go on, is that possible in v2 (no BC break), or are we forced to go on with v3 (BC break)?

For the first point, no one using this bundle will ever get bypass-finals installed, because it's a dev requirement. The purpose is solely meant to run tests, and the library is installed only in a dev or test environment, and only in a dev/test environment meant to develop this bundle (to be extra-clear: not in your dev environment of a project using this bundle as a dependency). That said, we can agree that this is sub-optimal and having interfaces would be better. And so we arrive at the second point.

Replacing concrete classes with interfaces is a no-go for users extending the classes. For example, if someone extended the VichFileType class, altering the signature of the constructor would cause a break. So, again, I can create a v3 branch and you can point this PR to it, but we still don't have an ETA for a possible v3 release.

[gnat42]

Hello,

I don't think you must get rid of it. If you wanted to, by virtue of the fact that interfaces are provided you could get rid of it.

You are correct in saying that I don't get bypass-finals installed because its a dev requirement for this bundle. However, I have tests in my own project that have the UploadHandler injected into them. Previous to the update to a newer version I could just do $uploadMock = $this->createMock(UploadHandler::class); and configure the methods. To be able to update to a newer version of this bundle, that stops working because UploadHandler is now final. Currently my only choice is to also install bypass-finals. I attempted that, however we had intermittent failures in our CI where sometimes it worked and sometimes it didn't.

You are correct that replacing concrete classes with interfaces breaks users could cause a break. In this case it doesn't because we aren't removing the ability for those classes to be injected. The symfony DI system still allows type hinting the concrete class automatically just like it does now. The fact is though, no one can extend these classes because they are final, PHP will not allow that.

The change as it is now is completely safe and BC with the current bundle. It is strictly additive. This is how it's managed in the main symfony project. New interfaces and ways of doing things are added in point releases and optionally the 'old' way is deprecated. Then in the next major version the deprecated methods/ways are removed. I don't think the other way of handling this must be removed, but it can simplify everything if you agreed.

I would think that if you are ok with the changes, I would have to adjust the PR to include documentation on the alternative way of a) injecting into your own services and b) testing by mocking interfaces vs bypass-finals

If someone is extending VichFileType, they can continue to do so because the UploadHandler implements the interface so would still continue to satisfy the function signature.

Perhaps this is what you are saying though. Are you saying that changing the function signature in any way, regardless of its compatibility is considered a backwards incompatible break? If that's what you're saying then yes, I suppose this is a break. However, since no one can extend the final classes, there is no situation where someone is extending them and type hinting their own implementation and this would break it. I'm pretty sure this is a safe BC thing to do that is used in other projects like symfony.

[garak]

The signature change is a break because the argument contravariance in PHP doesn't allow a child class to use a narrow argument type.
You should still be able to redefine the services you need in your test environment if you need to stick with the current bundle version.

Anyway, I think that your suggestion is still good, and I'd love to get it in a major version. I just created a 3.x branch. You can edit this PR to point there.

[gnat42]

I've targeted the new branch.

The signature change is a break because the argument contravariance in PHP doesn't allow a child class to use a narrow argument type. This is the thing though, there cannot be a child class. So normally yes, what you're saying is correct. However practically in this situation none of the services I've provided interfaces for can have child classes.

Would you like me to update anything else on the PR? The docs or additional tests using the interfaces and/or removal of bypass-libs?

[garak]

VichFileType and VichImageType are not services, and their constructor signatures change.
The docs are OK for now, we can move possible additional work (e.g. tests refactoring) to a second step

[Copilot]

Pull Request Overview

This PR introduces interfaces for core classes to enable easier testing by allowing mock implementations without relying on complex environment modifications. The main change is extracting interfaces from concrete classes to improve testability.

• Adds PropertyMappingFactoryInterface and handler interfaces (UploadHandlerInterface, DownloadHandlerInterface)
• Updates all classes to depend on interfaces rather than concrete implementations
• Configures service aliases to maintain backward compatibility

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/Mapping/PropertyMappingFactoryInterface.php	New interface defining the PropertyMappingFactory contract
src/Handler/UploadHandlerInterface.php	New interface for upload handler operations
src/Handler/DownloadHandlerInterface.php	New interface for download handler operations
src/Mapping/PropertyMappingFactory.php	Implements the new PropertyMappingFactoryInterface
src/Handler/UploadHandler.php	Implements the new UploadHandlerInterface
src/Handler/DownloadHandler.php	Implements the new DownloadHandlerInterface
src/Storage/*.php	Updated to use PropertyMappingFactoryInterface instead of concrete class
src/Form/Type/*.php	Updated to use handler interfaces instead of concrete classes
src/EventListener/Doctrine/BaseListener.php	Updated to use UploadHandlerInterface
src/Handler/AbstractHandler.php	Updated to use PropertyMappingFactoryInterface
config/*.xml	Added service aliases for the new interfaces

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[gnat42]

Oh, right, I was thinking about the services themselves, not the form types that consume them.