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

4.0.x Remove name property from Phalcon\Di\Service so that it can be used as a better closure wrapper #13590

Merged
merged 3 commits into from Nov 12, 2018

Conversation

Projects
3 participants
@dschissler
Copy link
Contributor

dschissler commented Nov 11, 2018

The current design of the Service class doesn't make it well suited for acting as a closure wrapper and the name property is unnecessary. There is only one place where the name property is used and that is in the DI and it already has access to the name of the service due to having just having received the name via a method parameter. I added a single exception class that is to be thrown when this one situation occurs. I think that this is a good use of an exception since it is an exceptional event. Otherwise the design of Phalcon suffers only to satisfy this one moment.

Accepting this pull request will allow frameworks to use one service per file designs.

return function() {
    // This is a service closure
}

In addition to the following two options;

    return new Service(function() {

    });

and

    return new Service(function() {

    }, true);

and then later by an additional pull request:

    return new SharedService(function() {

    });

If this pull request is not accepted then it must be done like:

    return new Service("this_is_bullshit", function() {

    });

and then hoping to not create a collision. However for purposes of sanity this entire use of Service is essentially unavailable and for no good reason

This pull request is derived from an old pull request that I pulled down from before when Phalcon was in its neglected state. I relied on the actual PR to run the tests for me and if I missed something here or something changed then I'll fix it and then if the fix or fixes become too messy then I will resubmit with a local copy-paste of the completed passing code. Obviously that isn't an ideal development process and is already being addressed elsewhere by the Team.

@sergeyklay sergeyklay requested a review from niden Nov 12, 2018

@sergeyklay sergeyklay added this to the 4.0.0 milestone Nov 12, 2018

@niden

This comment has been minimized.

Copy link
Member

niden commented Nov 12, 2018

The code looks solid.

We will have to rework this component for PSR implementation later on.

@niden

This comment has been minimized.

Copy link
Member

niden commented Nov 12, 2018

@dschissler Can you add a line to the CHANGELOG please before I merge this?

@dschissler

This comment has been minimized.

Copy link
Contributor Author

dschissler commented Nov 12, 2018

Damn typo in the changelog but its just an extra "s".

@niden

niden approved these changes Nov 12, 2018

@niden

This comment has been minimized.

Copy link
Member

niden commented Nov 12, 2018

@dschissler Thank you!

@niden niden merged commit 138dab6 into phalcon:4.0.x Nov 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@niden niden added this to Done in 4.0 Release Dec 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment