splitio / php-client

PHP SDK client for Split Software
https://split.io
Other
16 stars 12 forks source link

Return a singleton instead of null when trying to create multiple factories #150

Open xico42 opened 4 years ago

xico42 commented 4 years ago

The current code returns null when a factory was already instantiated. I can see some major problems with this approach:

  1. It is not intuitive for the developer
  2. This violates the method contract as specified in the @return docblock annotation tag. The annotation specifies that the return type is an instance of SplitFactoryInterface

My suggestion would be: Instead of tracking the instantiation of the factories by using a flag (KEY_FACTORY_TRACKER) we would keep a reference to the instantiated object in the container, so that whenever an user of the SDK tries to create multiple factories, we simply return the last created factory

Another approach would be to create a new static method called singleton to make this behavior more explicit.

xico42 commented 4 years ago

As a side note, I've implemented the last proposal at #151 because it seems to better comply with semantic versioning

mmelograno commented 3 years ago

Hi fcoedno,

First of all, thanks for suggestions on this particular behaviour. Secondly, we will apologise for how long it took to give you an answer. The suggestion and the code looks good and accurate. However, we are considering a refactor in our SDK to allow multiple factories instantiation. The feature of allowing multiple factory instantiation is one that is currently supported in almost all of our SDKs. For parity purposes, we will work on it for PHP SDK. We will track it as a feature request and keep a reference to this PR, so once it's prioritised and implemented we will reach out to let you know, but we don't have a target date for it yet.

Again, thank you for your patience. Cheers,

diniz-victor commented 1 year ago

@mmelograno do we have any expectations regarding the due date of it? Thanks in advance.

JohnFlyIII commented 1 year ago

Any updates? We're looking to leverage PHP SDK quite heavily and either a fix or update to the SDK would be appreciated Going to look into the above mentioned singleton #151

mmelograno commented 1 year ago

Hi @fcoedno, @diniz-victor and @boxofnotgoodery!

First of all, sorry for the delay on this feature coverage. Second, we have a candidate version for this which you can test if you want by adding the following line "splitsoftware/split-sdk-php": "dev-task/multipleFactory#86b8f7f9b08967cfa00065ebe7694f4859b3e41b" in your composer.

This version is a breaking change due to we are allowing Multiple Factory Instantiation as well as decided to remove support for PHP 7.3

It would be awesome if you can confirm that this version accomplishes your expectations.

Thanks, Matias

saulius-stasiukaitis-tg commented 9 months ago

@mmelograno Returning a new instance for localhost is what we need to make our unit tests work. Now, we override the SDK factory method with almost exact code. So hopefully you'll release this branch at some point :)