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

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

Open
xico42 opened this issue May 5, 2020 · 6 comments

Comments

@xico42
Copy link

xico42 commented May 5, 2020

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
Copy link
Author

xico42 commented May 5, 2020

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

@mmelograno
Copy link
Contributor

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
Copy link

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

@JohnFlyIII
Copy link

JohnFlyIII commented Nov 5, 2022

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
Copy link
Contributor

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

@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 :)

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

No branches or pull requests

5 participants