Optimize MutableCreationOptionsInterface capability #4408

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

noopable commented May 3, 2013

This enables Not only the factories but also the abstract factories implementing MutableCreationOptionsInterface to be able to get creation options.

library/Zend/ServiceManager/AbstractPluginManager.php
+ protected function createServiceViaCallback($callable, $cName, $rName)
+ {
+ do {
+ if (!is_array($callable)) {
@bakura10

bakura10 May 4, 2013

Contributor

Please factorize all those if into one single if (or at least group them in two if if you prefer) as you break in all cases.

Contributor

noopable commented May 4, 2013

I'm not good at English, so I'm sorry if I misunderstood you.
The method (createServiceViaCallback) is a protected method. It is only called from 'createFromFactory' and 'createFromAbstractFactory'. And it is a common gateway to create an object by the *factory. These call it by a callback array. And the creationOptions is only set at AbstractPluginManager::get if a client needs to set constructor options.
Thus I think this patch(or hotfix) will not cover all cases but covering the known cases.
My first commit is a step-by-step approach getting ability to get the creationOptions with a determinate minimum side effect.
My second commit enables a callable object to get creationOptions.
Are there any other necessary things?

Contributor

bakura10 commented May 5, 2013

What I told you is that all your if break, so you should factorize the if. For instance, replace this:

if (A) {
    break;
}

if (B) {
    break;
}

by:

if (A || B) {
    break;
}

it's more efficient and more compact :).

Contributor

noopable commented May 5, 2013

Oh I'm sorry for misunderstanding you.
But is it a code standard problem or a project rule that you pointed out ?
Or too ugly?
I usually chunk code-blocks with the aim of preserving the condition intent.
Each if-line is

  • Is it factory object or not?
  • Does the interface want to get creationOptions?
  • The client want to use creation option or not ?
  • Is it valid the specified creationOptions ?

The habit of these is efficient to debug with step-in tools .And more easy to check lines, easy to edit.

if (!isset($factory) || !is_object($factory)) {
    break;
}
if (!$factory instanceof MutableCreationOptionsInterface) {
    break;
}
if (null === $this->creationOptions) {
    break;
}
if (is_array($this->creationOptions) && empty($this->creationOptions)) {
    break;
}

But My habit is trivial matter.
I can remake these if-clause to the one-line.
Is that better?

if (is_object($callable)) {
    $factory = $callable;
} elseif (is_array($callable)) {
    $factory = reset($callable);
}
if (isset($factory) && is_object($factory) && ($factory instanceof MutableCreationOptionsInterface)
    && (null !== $this->creationOptions) && is_array($this->creationOptions) && !empty($this->creationOptions)){
    $factory->setCreationOptions($this->creationOptions); 
}
Contributor

bakura10 commented May 5, 2013

これわすごいね!

Service manager is a component that need to be efficient as it's used nearly everywhere, so if we can avoid some if... it's always good ;-)

Contributor

sasezaki commented May 5, 2013

@bakura10 You did typo! s/これわすごいね!/これはすごいね!/ (´・ω・`)

Contributor

bakura10 commented May 5, 2013

Oh yeah... How did I make this mistake, this is the only thing irregular in japanese :D.

library/Zend/ServiceManager/AbstractPluginManager.php
+ }
+
+ if (isset($factory) && is_object($factory) && ($factory instanceof MutableCreationOptionsInterface)
+ && (null !== $this->creationOptions) && is_array($this->creationOptions) && !empty($this->creationOptions)){
@iquabius

iquabius May 5, 2013

Contributor

If you're testing $factory against an interface, you can remove is_object(), and you don't need to check if null !== $this->creationOptions because is_array() will return false if it is null.

Contributor

noopable commented May 5, 2013

Thanks all.

I used to check by is_object in php old version but now not necessary. I removed it.

(null !== $this->creationOptions) is used in current
https://github.com/zendframework/zf2/blob/master/library/Zend/ServiceManager/AbstractPluginManager.php#L192
Can I rewrite it on the way?

And so, should I remake the 6 commits to 2 ?

Contributor

iquabius commented May 6, 2013

(null !== $this->creationOptions) is used in current
https://github.com/zendframework/zf2/blob/master/library/Zend/ServiceManager/AbstractPluginManager.php#L192
Can I rewrite it on the way?

That one line is slightly different and I think it is right the way it is.

And so, should I remake the 6 commits to 2 ?

That will be good since it will make the git history cleaner.

Contributor

noopable commented May 6, 2013

I have no idea about this.
Travis saids:

BUILD FAILED

/home/travis/build/zendframework/zf2/build.xml:15: Can't get http://cs.sensiolabs.org/get/php-cs-fixer.phar to
/home/travis/build/zendframework/zf2/php-cs-fixer.phar

Contributor

iquabius commented May 6, 2013

That have been happening for some time, I'm not sure why but you don't need to worry about that, it is just a Travis issue.

weierophinney added a commit that referenced this pull request May 6, 2013

Merge pull request #4408 from noopable/pr-FactoriesWithMutableCreatio…
…nOptions

Optimize MutableCreationOptionsInterface capability

weierophinney added a commit that referenced this pull request May 6, 2013

[#4408] CS fixes
- re-flow conditionals, one condition per line; brace onto next line for
  multi-line conditionals.
- add comments where needed
- add imports where needed

@ghost ghost assigned weierophinney May 6, 2013

weierophinney added a commit that referenced this pull request May 6, 2013

+ && is_array($this->creationOptions) && !empty($this->creationOptions)){
+ $factory->setCreationOptions($this->creationOptions);
+ }
+
@noopable

noopable Jun 1, 2013

Contributor

Memo: if $factory is not object, 'instanceof' raises fatal error. To do fix it?

@iquabius

iquabius Jun 2, 2013

Contributor

That actually does not happen, at least with php 5.3 and up!

@noopable

noopable Jun 2, 2013

Contributor

Is that really?
Document saids,

Constants, however, are not allowed. 

http://php.net/manual/en/language.operators.type.php#example-130

I tested it.

> php -v
PHP 5.4.13 (cli) (built: Mar 15 2013 02:07:14)
Copyright (c) 1997-2013 The PHP Group
Zend Engine v2.4.0, Copyright (c) 1998-2013 Zend Technologies
    with Xdebug v2.2.2, Copyright (c) 2002-2013, by Derick Rethans

<?php
var_dump((false instanceof \StdClass));

raises
PHP Fatal error: instanceof expects an object instance, constant given in - on line 2

Please try it.

@iquabius

iquabius Jun 2, 2013

Contributor

you are right, but the code is not using a constant, it is using a variable ($factory), so there's no way of this to happen.

@noopable

noopable Jun 2, 2013

Contributor

Many thanks.
I was anxious about it . Now I see.

@noopable noopable deleted the noopable:pr-FactoriesWithMutableCreationOptions branch Dec 13, 2013

weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#4408 from noopable/pr-…
…FactoriesWithMutableCreationOptions

Optimize MutableCreationOptionsInterface capability

weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015

[zendframework/zendframework#4408] CS fixes
- re-flow conditionals, one condition per line; brace onto next line for
  multi-line conditionals.
- add comments where needed
- add imports where needed

weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015

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