inpsyde / modularity

A PSR-11 implementation for WordPress Plugins, Themes or Libraries.
https://inpsyde.github.io/modularity/
GNU General Public License v2.0
44 stars 4 forks source link

Remove unnecessary and expensive hasService check #36

Closed tfrommen closed 1 year ago

tfrommen commented 1 year ago

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

This PR removes the ContainerConfigurator::hasService check inside ContainerConfigurator::addService.

This is unnecessary and potentially expensive (especially with large and/or multiple and/or composite containers).

Previously, there was an exception thrown in case someone wanted to register a module with an existing ID. In https://github.com/inpsyde/modularity/commit/74cf1a5bba4913a4ecb1f1db714fdbd047d13e3b, the exception has been removed (for the reasons mentioned in #17).

This means, previously, we needed to know if there already is a service with the given ID. Now, we don't care, and we can simply unset the potentially-existing service as unset($foo) does not care whether $foo exists.

What is the current behavior? (You can also link to an open issue here)

Calling ContainerConfigurator::addService will internally call ContainerConfigurator::hasService.

What is the new behavior (if this is a feature change)?

Calling ContainerConfigurator::addService will attempt to unset the entry in the factory lookup table without checking if it exists first.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Other information:

Biont commented 1 year ago

The explanation makes sense to me. LGTM, thanks