Please check if the PR fulfills these requirements
[x] The commit message follows our guidelines
[x] Tests for the changes have been added (for bug fixes/features)
[x] Docs have been added/updated (for bug fixes/features)
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
It removes the Exception thrown when a duplicate service ID is encountered. This made it impossible to inject mocks on the boundaries of the application for functional tests. It also prevented a couple of use-cases and patterns during runtime that required explicit workarounds or potentially wasteful usage of extensions
What is the current behavior? (You can also link to an open issue here)
It is not possible to override a previously registered service from another module. This caused an Exception to be thrown.
What is the new behavior (if this is a feature change)?
Services (and factories respectively) can now be overridden. The factory flag is cleared when replacing the service resulting in consistent behaviour in both cases: A factory would simply re-add the flag afterwards while a service won't.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Depends. If you are extremely picky, then removing an error would break third-party code that somehow depended on the error being there (which admittedly was part of the public API)
On the other hand, this would technically mean there can never be minor releases because code might rely on a new method not being present. Hence, I would not classify this as a breaking change. All code that was working before will continue working.
Other information:
Debugging
I would have liked to trigger some WP action when an override occurs, enabling maintainers to be very defensive about this topic and throw an exception for any non-whitelisted override. That, and of course for regular debugging purposes. However, since Package::hookName() is inaccessible to the ContainerConfigurator, the only solution would be to trigger the action with a generic hook name (like inpsyde.modularity.on-service-override), which I feel is undesired for this scenario.
Stability
There is a concern about introducing potential bugs due to accidental service overrides. These can happen if two modules (from separate sources that do not communicate with or know each other) happen to use the same service ID. This might go unnoticed by the package maintainer (especially because we currently do not have a mechanism to notify anyone about it). This is something should need to evaluate. Future development can help mitigate this if it occurs: We could...
introduce the aforementioned WP hook to be defensive about it
introduce an allowOverride() method to whitelist at Packagelevel
introduce a CollisionResolver interface to do the same - and then some
inspect the list of services (and therefore potential overrides) from a composer plugin - and notify maintainers during composer install
Ultimately, this is the package maintainers' responsibility. In a scenario where an accidental override might occur, it is already possible for similar problems to stem from extensions
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
It removes the Exception thrown when a duplicate service ID is encountered. This made it impossible to inject mocks on the boundaries of the application for functional tests. It also prevented a couple of use-cases and patterns during runtime that required explicit workarounds or potentially wasteful usage of extensions
What is the current behavior? (You can also link to an open issue here)
It is not possible to override a previously registered service from another module. This caused an Exception to be thrown.
What is the new behavior (if this is a feature change)?
Services (and factories respectively) can now be overridden. The factory flag is cleared when replacing the service resulting in consistent behaviour in both cases: A factory would simply re-add the flag afterwards while a service won't.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Depends. If you are extremely picky, then removing an error would break third-party code that somehow depended on the error being there (which admittedly was part of the public API) On the other hand, this would technically mean there can never be minor releases because code might rely on a new method not being present. Hence, I would not classify this as a breaking change. All code that was working before will continue working.
Other information:
Debugging
I would have liked to trigger some WP action when an override occurs, enabling maintainers to be very defensive about this topic and throw an exception for any non-whitelisted override. That, and of course for regular debugging purposes. However, since
Package::hookName()is inaccessible to theContainerConfigurator, the only solution would be to trigger the action with a generic hook name (likeinpsyde.modularity.on-service-override), which I feel is undesired for this scenario.Stability
There is a concern about introducing potential bugs due to accidental service overrides. These can happen if two modules (from separate sources that do not communicate with or know each other) happen to use the same service ID. This might go unnoticed by the package maintainer (especially because we currently do not have a mechanism to notify anyone about it). This is something should need to evaluate. Future development can help mitigate this if it occurs: We could...
allowOverride()method to whitelist atPackagelevelCollisionResolverinterface to do the same - and then somecomposer installUltimately, this is the package maintainers' responsibility. In a scenario where an accidental override might occur, it is already possible for similar problems to stem from extensions