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

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

dschissler
Copy link
Contributor

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.

@niden
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
niden previously approved these changes Nov 12, 2018
@niden
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 was marked as abuse.

@niden
Copy link
Member

niden commented Nov 12, 2018

@dschissler Thank you!

@niden niden merged commit 138dab6 into phalcon:4.0.x Nov 12, 2018
@sergeyklay sergeyklay added the breaks bc Functionality that breaks Backwards Compatibility label Nov 12, 2018
@niden niden added the documentation Documentation required label Apr 9, 2019
@niden niden added 4.0 and removed documentation Documentation required labels Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks bc Functionality that breaks Backwards Compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants