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

Wip separate injector contexts rebase #196

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Danack
Copy link
Collaborator

@Danack Danack commented Apr 20, 2023

Although the current use of the extra args in Injector::execute() and Injector::make() are powerful, they are also dangerous.

They can lead to 'surprising' behaviour where data is shared inappropriately:

    public function testWhySeparationIsNeeded()
    {
        $injector = new Injector();
        $message = "shared instance has one off message";

        // Declare a class as shared
        $injector->share(SharedClassInInjector::class);
        // Create an instance with one-off variables. The object is created.
        $obj1 = $injector->make(SharedClassInInjector::class, [':message' => $message]);

        // Create another instance... but as it is shared, the previous
        // 'one-off' message is used.
        $obj2 = $injector->make(SharedClassInInjector::class, [':message' => "Surprise!"]);

        $this->assertSame($message, $obj1->getMessage());
        $this->assertSame($message, $obj2->getMessage());
    }

by allowing a separate context/injector to be created, the correct behaviour can be done by defining things that should't be shared, on the separated context.

Note, this doesn't prevent users from accidentally sharing data inappropriately.

    $separate_injector = $injector->separateContext();
    $injector->defineParam('message', $message);

The $separate_injector would still be able to access that message. It only makes it possible to not do it.

API and names are not super great currently. Open to improvement on them.

Also, if someone is separating a context, they are probably going to do something with it very soon, so we could make it possible to pass in shares, defines etc.

@Danack Danack added this to the 2.0 milestone Apr 20, 2023
@Danack Danack changed the base branch from master to dev April 20, 2023 12:34
@Danack Danack mentioned this pull request Apr 20, 2023
@Danack
Copy link
Collaborator Author

Danack commented Apr 21, 2023

Forgot to say, this could lead to the deprecation and eventually removal of the args for Injector:execute() + Injector:make(), which would give a small performance benefit due to the fewer cases of how to make stuff.

But there's no rush on that...

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

Successfully merging this pull request may close these issues.

1 participant