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

Service class API change and addition of helper class #13157

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions phalcon/di.zep
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use Phalcon\Config;
use Phalcon\Di\Service;
use Phalcon\DiInterface;
use Phalcon\Di\Exception;
use Phalcon\Di\Exception\ServiceResolutionException;
use Phalcon\Config\Adapter\Php;
use Phalcon\Config\Adapter\Yaml;
use Phalcon\Di\ServiceInterface;
Expand Down Expand Up @@ -127,7 +128,7 @@ class Di implements DiInterface
public function set(string! name, var definition, boolean shared = false) -> <ServiceInterface>
{
var service;
let service = new Service(name, definition, shared),
let service = new Service(definition, shared),
this->_services[name] = service;
return service;
}
Expand Down Expand Up @@ -160,7 +161,7 @@ class Di implements DiInterface
var service;

if !isset this->_services[name] {
let service = new Service(name, definition, shared),
let service = new Service(definition, shared),
this->_services[name] = service;
return service;
}
Expand Down Expand Up @@ -210,7 +211,7 @@ class Di implements DiInterface
*/
public function get(string! name, parameters = null) -> var
{
var service, eventsManager, instance = null;
var service, eventsManager, instance = null, e = null;

let eventsManager = <ManagerInterface> this->_eventsManager;

Expand All @@ -227,7 +228,11 @@ class Di implements DiInterface
/**
* The service is registered in the DI
*/
let instance = service->resolve(parameters, this);
try {
let instance = service->resolve(parameters, this);
} catch ServiceResolutionException, e {
throw new Exception("Service '" . name . "' cannot be resolved");
}
} else {
/**
* The DI also acts as builder for any class even if it isn't defined in the DI
Expand Down
30 changes: 30 additions & 0 deletions phalcon/di/exception/serviceResolutionException.zep
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@

/*
+------------------------------------------------------------------------+
| Phalcon Framework |
+------------------------------------------------------------------------+
| Copyright (c) 2011-2017 Phalcon Team (https://phalconphp.com) |
+------------------------------------------------------------------------+
| This source file is subject to the New BSD License that is bundled |
| with this package in the file LICENSE.txt. |
| |
| If you did not receive a copy of the license and are unable to |
| obtain it through the world-wide-web, please send an email |
| to [email protected] so we can send you a copy immediately. |
+------------------------------------------------------------------------+
| Authors: Andres Gutierrez <[email protected]> |
| Eduar Carvajal <[email protected]> |
+------------------------------------------------------------------------+
*/

namespace Phalcon\Di\Exception;

use Phalcon\Di\Exception\ServiceResolutionException;

/**
* Phalcon\Di\Exception\ServiceResolutionException
*
*/
class ServiceResolutionException extends \Phalcon\Di\Exception
{
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this file phalcon/di/exception/serviceResolutionException.zep => phalcon/di/exception/serviceresolutionexception.zep to follow common style

26 changes: 7 additions & 19 deletions phalcon/di/service.zep
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ namespace Phalcon\Di;

use Phalcon\DiInterface;
use Phalcon\Di\Exception;
use Phalcon\Di\Exception\ServiceResolutionException;
use Phalcon\Di\ServiceInterface;
use Phalcon\Di\Service\Builder;

Expand All @@ -40,9 +41,6 @@ use Phalcon\Di\Service\Builder;
*/
class Service implements ServiceInterface
{

protected _name;

protected _definition;

protected _shared = false;
Expand All @@ -55,22 +53,14 @@ class Service implements ServiceInterface
* Phalcon\Di\Service
*
* @param mixed definition
* @param boolean shared
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annotation not needed at all. Just use

public function __construct(var definition, boolean shared = false)

*/
public final function __construct(string! name, definition, boolean shared = false)
public function __construct(definition, boolean shared = false)
{
let this->_name = name,
this->_definition = definition,
let this->_definition = definition,
this->_shared = shared;
}

/**
* Returns the service's name
*/
public function getName() -> string
{
return this->_name;
}

/**
* Sets if the service is shared or not
*/
Expand Down Expand Up @@ -121,6 +111,7 @@ class Service implements ServiceInterface
* Resolves the service
*
* @param array parameters
* @param \Phalcon\DiInterface dependencyInjector
* @return mixed
*/
public function resolve(parameters = null, <DiInterface> dependencyInjector = null)
Copy link
Contributor

@sergeyklay sergeyklay Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public function resolve(array parameters = null, <DiInterface> dependencyInjector = null) -> var

and remove @params and @return

Expand Down Expand Up @@ -202,7 +193,7 @@ class Service implements ServiceInterface
* If the service can't be built, we must throw an exception
*/
if found === false {
throw new Exception("Service '" . this->_name . "' cannot be resolved");
throw new ServiceResolutionException();
}

/**
Expand Down Expand Up @@ -254,6 +245,7 @@ class Service implements ServiceInterface
/**
* Returns a parameter in a specific position
*
* @param int position
* @return array
*/
public function getParameter(int position)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public function getParameter(int position) -> array | null

and remove @param and @return

Expand Down Expand Up @@ -292,10 +284,6 @@ class Service implements ServiceInterface
{
var name, definition, shared;

if !fetch name, attributes["_name"] {
throw new Exception("The attribute '_name' is required");
}

if !fetch definition, attributes["_definition"] {
throw new Exception("The attribute '_definition' is required");
}
Expand Down
115 changes: 115 additions & 0 deletions phalcon/di/service/serviceregistry.zep
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
namespace Phalcon\Di\Service;

use Phalcon\DiInterface;
use Phalcon\Di\Exception;
use Phalcon\Di\ServiceInterface;
use Phalcon\Config\Adapter\Yaml;

/**
* Phalcon\Di\Service\ServiceRegistry
*
**/
class ServiceRegistry
{

/**
*
*/
protected di;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need empty comment here?


/**
*
*/
public function __construct(<DiInterface> di)
{
let this->di = di;
}

/**
*
*/
public function registerDir(dirPath)
{
var dirPathItem;

switch (gettype(dirPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch typeof dirPath {
    // ...
}

This comment was marked as abuse.

case "string":
this->_registerDir(dirPath);
break;
case "array":
case "object":
for dirPathItem in iterator(dirPath) {
this->_registerDir(dirPathItem);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if dirPath instanceof Traversable

?

This comment was marked as abuse.

This comment was marked as abuse.

break;
default:
throw new Exception("Invalid services.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see here a error message with an explanation of what happened and the reasons for this

break;
}
}

/**
*
*/
protected function _registerDir(string! dirPath)
Copy link
Contributor

@sergeyklay sergeyklay Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method names should not be prefixed with a single underscore to indicate protected nature. Actually when you extend this class in PHP your code will not pass the code style tests.

I propose to get rid of underscores in method names for 4.x

This comment was marked as abuse.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dschissler Frankly speaking I'm a little confused with Dir. Probably it's because I'm not a native speaker (like many of our users). Dir it is: DIRection. DIRectory, Do It Right, ...? Why do we need abbreviations for methods that aren't part of the public API?

This comment was marked as abuse.

{
var dir, fileinfo, extension, serviceNameSection, forceShared, serviceName;

let dir = new \DirectoryIterator(dirPath);
for fileinfo in iterator(dir) {
if (fileinfo->isDir()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if unlikely fileinfo->isDir() === true {
    // ...
}

Also I'm pretty sure we don't need to scan this dir in each request

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about . and .. ?

This comment was marked as abuse.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I propose consider such a design with which user can easily inject a caching mechanism. For example Decorator Pattern

This comment was marked as abuse.

This comment was marked as abuse.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Php already caches stuff like this.

continue;
}

let extension = fileinfo->getExtension();
let serviceNameSection = fileinfo->getBasename("." . extension);

let forceShared = substr(serviceNameSection, -7) === "_shared";
let serviceName = forceShared ? substr(serviceNameSection, 0, -7) : serviceNameSection;

this->_registerFile(fileinfo->getPathname(), serviceName, extension, forceShared);
}
}

/**
*
*/
protected function _registerFile(string! path, string! serviceName, string! extension, boolean forceShared)
{
var serviceDef;

switch (extension) {
case "php":
let serviceDef = require path;

switch (gettype(serviceDef)) {
case "object":
if (serviceDef instanceof ServiceInterface) {
if (forceShared) {
serviceDef->setShared(true);
}
this->di->setRaw(serviceName, serviceDef);
} elseif (is_callable(serviceDef)) {
this->di->set(serviceName, serviceDef, forceShared);
} else {
throw new Exception("Invalid service object definiton.");
}
break;
case "array":
case "string":
this->di->set(serviceName, serviceDef, forceShared);
break;
default:
throw new Exception("Invalid service definiton.");
}
break;
case "yml":
let serviceDef = new Yaml(path);
this->di->set(serviceName, serviceDef->toArray(), forceShared);
break;
default:
throw new Exception("Invalid service definition type.");
break;
}
}
}
48 changes: 48 additions & 0 deletions phalcon/di/service/sharedservice.zep
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@

/*
+------------------------------------------------------------------------+
| Phalcon Framework |
+------------------------------------------------------------------------+
| Copyright (c) 2011-2017 Phalcon Team (https://phalconphp.com) |
+------------------------------------------------------------------------+
| This source file is subject to the New BSD License that is bundled |
| with this package in the file LICENSE.txt. |
| |
| If you did not receive a copy of the license and are unable to |
| obtain it through the world-wide-web, please send an email |
| to [email protected] so we can send you a copy immediately. |
+------------------------------------------------------------------------+
| Authors: Andres Gutierrez <[email protected]> |
| Eduar Carvajal <[email protected]> |
+------------------------------------------------------------------------+
*/

namespace Phalcon\Di\Service;

use Phalcon\Di\Service;

/**
* Phalcon\Di\Service\SharedService
*
*
*<code>
* $service = new \Phalcon\Di\Service\SharedService(
* "request",
* "Phalcon\\Http\\Request"
* );
*
* $request = service->resolve();
*</code>
*/
class SharedService extends Service
{
/**
* Phalcon\Di\Service\SharedService
*
* @param mixed definition
*/
public final function __construct(definition)
{
parent::__construct(definition, true);
}
}
5 changes: 0 additions & 5 deletions phalcon/di/serviceinterface.zep
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,6 @@ use Phalcon\DiInterface;
*/
interface ServiceInterface
{
/**
* Returns the service's name
*/
public function getName() -> string;

/**
* Sets if the service is shared or not
*/
Expand Down