Service class API change and addition of helper class #13157

dschissler · 2017-11-12T15:49:47Z

I removed the name parameter from the Service class since the only place where it is used in the Phalcon code is for throwing an exception when there was a problem with the service. I added a new exception class which the DI catches and since the DI knows what the name of the service should be it is able to provide the same error reporting capabilities.

This API change allows for Phalcon\Di\Service classes to more gracefully wrap service closures.

I added a Phalcon\Di\Service\SharedService class which simply offers API sugar to make the second parameter unnecessary when using a shared service.

I added a new helper class Phalcon\Di\Service\ServiceRegistry for registering a directory of service files in either yaml or php. These files can return a php array with a static definition, a closure, a Service object or a string for the class name.

[edit] I'm going to take a week off from this and then I'll take down this pull request and start over with a clean repo with just 2-3 commits. I'm going to redesign the API a bit, modify the tests, etc.

…tion to be caught in a container, which already has its name.

…les.

sergeyklay · 2017-11-12T15:51:19Z

phalcon/di/exception/serviceResolutionException.zep

+ */
+class ServiceResolutionException extends \Phalcon\Di\Exception
+{
+}


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

sergeyklay · 2017-11-12T15:52:16Z

phalcon/di/service.zep

@@ -55,23 +53,15 @@ class Service implements ServiceInterface
 	 * Phalcon\Di\Service
 	 *
 	 * @param mixed definition
+	 * @param boolean shared


Annotation not needed at all. Just use

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

sergeyklay · 2017-11-12T15:53:53Z

phalcon/di/service.zep

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


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

and remove @params and @return

sergeyklay · 2017-11-12T15:55:20Z

phalcon/di/service.zep

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


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

and remove @param and @return

sergeyklay · 2017-11-12T15:55:54Z

phalcon/di/service/serviceregistry.zep

+	/**
+	 *
+	 */
+	protected di;


Why do we need empty comment here?

sergeyklay · 2017-11-12T16:00:02Z

phalcon/di/service/serviceregistry.zep

+	/**
+	 *
+	 */
+	protected function _registerDir(string! dirPath)


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

@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?

sergeyklay · 2017-11-12T16:02:31Z

You have successfully broken all the tests :)

sergeyklay · 2017-11-12T19:54:35Z

phalcon/di/service/serviceregistry.zep

+			case "object":
+				for dirPathItem in iterator(dirPath) {
+					this->_registerDir(dirPathItem);
+				}


if dirPath instanceof Traversable

?

sergeyklay · 2017-11-12T19:56:03Z

phalcon/di/service/serviceregistry.zep

+				}
+				break;
+			default:
+				throw new Exception("Invalid services.");


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

sergeyklay · 2017-11-12T19:58:16Z

phalcon/di/service/serviceregistry.zep

+	{
+		var dirPathItem;
+
+		switch (gettype(dirPath)) {


switch typeof dirPath { // ... }

sergeyklay · 2017-11-12T20:02:02Z

phalcon/di/service/serviceregistry.zep

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


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

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

How about . and .. ?

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

Php already caches stuff like this.

sergeyklay · 2017-11-30T08:42:57Z

@dschissler Could you remove session from factory default? See: #12921

David Schissler added 2 commits November 12, 2017 07:39

Remove service name parameter from Service and instead throw an Excep…

b5d60ec

…tion to be caught in a container, which already has its name.

Add service registry helper for registering directories of service fi…

fedc7ae

…les.

sergeyklay suggested changes Nov 12, 2017

View reviewed changes

sergeyklay reviewed Nov 12, 2017

View reviewed changes

sergeyklay added breaks bc Functionality that breaks Backwards Compatibility New Feature Request labels Nov 12, 2017

David Schissler added 5 commits November 12, 2017 18:40

Code quality improvements.

e395bae

Gettin' there.

c3cee07

Attempt to not break test.

e20607c

Remove service name from Service constructors in factory default DIs.

8e936ba

Fix name argument being sent into Service constructor.

e6cf9a6

This comment was marked as abuse.

Sign in to view

dschissler closed this Dec 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service class API change and addition of helper class #13157

Service class API change and addition of helper class #13157

dschissler commented Nov 12, 2017 •

edited

Loading

sergeyklay Nov 12, 2017

sergeyklay Nov 12, 2017

sergeyklay Nov 12, 2017 •

edited

Loading

sergeyklay Nov 12, 2017

sergeyklay Nov 12, 2017

sergeyklay Nov 12, 2017 •

edited

Loading

This comment was marked as abuse.

sergeyklay Nov 13, 2017

This comment was marked as abuse.

sergeyklay commented Nov 12, 2017

sergeyklay Nov 12, 2017

This comment was marked as abuse.

This comment was marked as abuse.

sergeyklay Nov 12, 2017

sergeyklay Nov 12, 2017

This comment was marked as abuse.

sergeyklay Nov 12, 2017

sergeyklay Nov 12, 2017

This comment was marked as abuse.

sergeyklay Nov 13, 2017

This comment was marked as abuse.

This comment was marked as abuse.

Jurigag Nov 23, 2017

This comment was marked as abuse.

This comment was marked as abuse.

sergeyklay commented Nov 30, 2017

This comment was marked as abuse.

Service class API change and addition of helper class #13157

Service class API change and addition of helper class #13157

Conversation

dschissler commented Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyklay Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyklay Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

This comment was marked as abuse.

Choose a reason for hiding this comment

This comment was marked as abuse.

sergeyklay commented Nov 12, 2017

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as abuse.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as abuse.

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

sergeyklay commented Nov 30, 2017

This comment was marked as abuse.

dschissler commented Nov 12, 2017 •

edited

Loading

sergeyklay Nov 12, 2017 •

edited

Loading

sergeyklay Nov 12, 2017 •

edited

Loading