merged branch ArabCoders/master (PR #66)
merged branch ArabCoders/master (PR #66)

This PR was squashed before being merged into the master branch (closes #66).

Commits
-------

afece3c Change  Visibility to be protected

Discussion
----------

Change $values Visibility to be protected

This change allow other classes to extend Pimple and override some core class function.

EDIT: change public to protected

---------------------------------------------------------------------------

by stof at 2013-02-14T08:26:41Z

Why public ? It make no sense to have it public as some work needs to be done when accessing the values.

---------------------------------------------------------------------------

by ArabCoders at 2013-02-14T09:35:35Z

Hi
@stof,

it does make sense when you want to override(ie: overload) function take the following example

````php
class diDebug extends \Pimple
{
    public function offsetGet($id)
    {
        if (!array_key_exists($id, $this->values)) {
            throw new InvalidArgumentException(sprintf('Identifier "%s" is not defined.<BR />%s', $id, $this->get_backtrace() ));
        }

        $isFactory = is_object($this->values[$id]) && method_exists($this->values[$id], '__invoke');

        return $isFactory ? $this->values[$id]($this) : $this->values[$id];
    }

    public function raw($id)
    {
        if (!array_key_exists($id, $this->values)) {
            throw new InvalidArgumentException(sprintf('Identifier "%s" is not defined.<BR /> %s', $id, $this->get_backtrace() ));
        }

        return $this->values[$id];
    }
}
````

this example will not work because pimple::$values is set to private, and i dont think its reasonable to expect every call to the dependency injection class to be in try {} catch{} block

---------------------------------------------------------------------------

by fabpot at 2013-02-14T09:37:51Z

protected would be enough.

---------------------------------------------------------------------------

by stof at 2013-02-14T09:40:02Z

@ArabCoders thus, why do you need to include the backtrace in the exception message ? Enable XDebug and the trace will be displayed when an exception occurs.

---------------------------------------------------------------------------

by ArabCoders at 2013-02-14T09:43:22Z

@fabpot, indeed that will works too, i can send another pull request if you want.

@stof that's because most shared hosts dont have xdebug enabled or even compiled, and I have to debug manually.

---------------------------------------------------------------------------

by stof at 2013-02-14T09:45:06Z

are you developing directly on your hosting ?

---------------------------------------------------------------------------

by whatthejeff at 2013-02-14T09:48:27Z

Why not just do this:

```php
class diDebug extends \Pimple
{
    public function offsetGet($id)
    {
        try {
           return parent::offsetGet($id);
        } catch (Exception $exception) {
           // do whatever
        }
    }
}

---------------------------------------------------------------------------

by ArabCoders at 2013-02-14T09:53:35Z

@stof No, Im trying to debug client framework, and i run into this issue the other developer didn't follow any logical patterns, or test environment and the website is huge to be copied over to my test server. to replicate the problem so i had to make do with what is other developer had at that time and invent some hacks to make it work.

Anyway its not about me, i just think $values should be either public or protected so that other classes can extend it without any problems. ^^

---------------------------------------------------------------------------

by ArabCoders at 2013-02-14T09:59:10Z

@whatthejeff Yes it can be done that way too, i only showed an example for use case to make $values [public/protected] not necessarily the only example ^^

---------------------------------------------------------------------------

by stof at 2013-02-14T10:00:47Z

@ArabCoders Making it public means that it is part of the public API. But this is wrong. People should not be able to access the internal storage from the outside.

---------------------------------------------------------------------------

by ArabCoders at 2013-02-14T10:05:35Z

@stof yeah i realized that, that's why i asked if @fabpot wanted me to do another pull request with protected or he would do it :) im kinda new to the whole fork/pull thing.

PS: English is not my first tongue, so forgive any grammar mistakes

silexphp/Pimple,fabpot/Pimple,jpauli/Pimple,RustJason/Pimple,arduanov/Pimple,michabbb-backup/Pimple,gomboc/Pimple,mcuadros/pimple-hack,mnapoli/simplex,fabpot/Pimple,jbboehr/Pimple,arduanov/Pimple,jpauli/Pimple,jbboehr/Pimple,RustJason/Pimple,Ocramius/Pimple,gomboc/Pimple,jpauli/Pimple