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

[BUG] \Phalcon\DI #1209

Closed
ovr opened this issue Sep 8, 2013 · 20 comments
Closed

[BUG] \Phalcon\DI #1209

ovr opened this issue Sep 8, 2013 · 20 comments
Labels
not a bug Reported issue is not a bug

Comments

@ovr
Copy link
Contributor

ovr commented Sep 8, 2013

Hi all)
Bug with boolean and service consruct in di

/**
* @var boolean *false*
*/
$returnValue = $this->{$method->getName()}();
//work *like hack*
$di->set($name, function() use ($returnValue) {return $returnValue;});
//bad links
$di->set($name, function() use (&$returnValue) {return $returnValue;});
//not work
$di->set($name, $returnValue);

i am n`t c developer
ovr@663624f

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@phalcon
Copy link
Collaborator

phalcon commented Sep 8, 2013

DI is a dependency injection and service resolver component, it does not act as a registry.

@ovr
Copy link
Contributor Author

ovr commented Sep 8, 2013

@phalcon
Okey How can I save my identity in view controllers all inj di classes if it may be \User|false?

@phalcon
Copy link
Collaborator

phalcon commented Sep 9, 2013

You can create a real registry:

class MyRegistry
{

    private $items = array();

    public function get($key)
    {
        return $this->_items[$key];
    }

    public function set($key, $value)
    {
        $this->_items[$key] = $value;
    }
}

Service registration:

$di['registry'] = new MyRegistry();

Usage:

$registry = $di->getRegistry();
$registry->set('my-record', Records::findFirst());
$record = $registry->get('my-record');

@ovr
Copy link
Contributor Author

ovr commented Sep 9, 2013

But now in view I need.

$this->registry->get('identity');

Not beautiful use
please read my first message and see that it may hacked but normal variant not be used and no exceptions for me didn't present on set

@phalcon
Copy link
Collaborator

phalcon commented Sep 9, 2013

But that would allow a developer to give an incorrect usage to the DI, so in the future we will have false "bugs" or NFR with:

Hey, I found a bug:

$di->set('x', 'yyy');

When I try to access 'x' I'm getting the following exception:
"Class yyy was not found"
Why? I want to use the DI as a registry, I don't want to use it to load classes

@ghost
Copy link

ghost commented Sep 9, 2013

Well,

class MyRegistry
{
    private $items = array();

    public function __get($key)
    {
        return $this->_items[$key];
    }

    public function __set($key, $value)
    {
        $this->_items[$key] = $value;
    }
}

and then use

$this->registry->identity

a bit shorter :-)

@ghost
Copy link

ghost commented Sep 9, 2013

I want to use the DI as a registry, I don't want to use it to load classes

Good point — registry and dependency injector are different entities.

@ovr
Copy link
Contributor Author

ovr commented Sep 9, 2013

@phalcon @sjinks
Callable

$di->set('ex1', function() {return 'sdgdsg';});
var_dump($di->get('ex1'));
$di->set('ex2', function() {return false;});
var_dump($di->get('ex2'));

https://github.com/phalcon/cphalcon/blob/1.3.0/ext/di.c#L113
but in realy it string|object|callable?
but callable may be all type

@ghost
Copy link

ghost commented Jan 28, 2014

@phalcon

What if we check in Phalcon\DI\Service::resolve() what the service has been resolved to and throw an exception if it is not an object?

@ghost ghost mentioned this issue Jan 28, 2014
@ovr
Copy link
Contributor Author

ovr commented Jan 29, 2014

after we create this condition for check resolve it broke my app )) it broke back support need to be in 1.3.0 and important fix

@ghost
Copy link

ghost commented Jan 29, 2014

This condition has not been added yet as far as I know.

@ovr
Copy link
Contributor Author

ovr commented Jan 29, 2014

yeap but i only notice about that condition will crash backward compatibility ^_^

@ghost
Copy link

ghost commented Jan 29, 2014

Yes, need to emit E_DEPRECATED there :-)

@ghost
Copy link

ghost commented Feb 2, 2014

@zaets28rus This has been merged, could you please test and close the issue if everything works for you?

@ovr
Copy link
Contributor Author

ovr commented Feb 3, 2014

@sjinks i will test it after phalcon merge #1966

@temuri416
Copy link
Contributor

I've commented on this here: #1935

Can we please have a switch that would allow us to return strings? It's been a disastrous patch, really.

@ghost
Copy link

ghost commented Feb 26, 2014

@phalcon Need your opinion.

@temuri416
Copy link
Contributor

I would like to explain my position once more.

I totally understand why you introduced Phacon\Registry, and I fully support it and will be using it.

Phalcon\Di was a great way to embed custom logic and make sure that's executed just once (when you need it) and returns variety of result types. For example:

$di->set('myObj', function ($returnAsJson = false) {
    $obj = new Obj;
    if ($returnAsJson) {
        return $obj->asJson();
    }
    return $obj;
});

$json = $di->get('myObj', true);

And that was great. In my opinion, implementation of Phalcon\Registry should not have affected the way Phalcon\Di is functioning, because they serve clearly distinct purposes.

Will be happy to hear your opinions.

Thanks!

@ovr
Copy link
Contributor Author

ovr commented Mar 7, 2014

@phalcon ping

@andresgutierrez
Copy link
Contributor

You can use Phalcon\Registry in case you need a registry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not a bug Reported issue is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants