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

Conflicts using __get() method to automatically load models #3137

Closed
OldChapStudio opened this Issue Jul 8, 2014 · 6 comments

Comments

Projects
None yet
4 participants
@OldChapStudio

OldChapStudio commented Jul 8, 2014

Hi,

I was trying to add the __get() method to my main controller, so that I don't need anymore to call the $this->load->model() each time I call for a model, here's the code :

public function __get($property) 
    {
        if(strpos($property,'_model') !== FALSE) 
        {
            $this->load->model($property);
            return $this->$property;
        }
        return NULL;
    }

It works fine but generates some conflicts with Code Igniter internals :

  1. Generates a notice error :
Message: Indirect modification of overloaded property Game_controller::$benchmark has no effect
  1. Generates a fatal error :
Cannot assign by reference to overloaded object in path\system\core\Controller.php on line 64

It seems the __get method don't really like the pass-by reference syntax...
By correcting this two lines in core/Controller.php

$this->$var =& load_class($class);
$this->$var = load_class($class);

$this->load =& load_class('Loader', 'core');
$this->load = load_class('Loader', 'core');

Everything works fine. So my question is :
Do CodeIgniter needs anymore the pass-by reference syntax for objects assignements ? Knowing it is obsolete now with php 5.

Do this part of code could be proposed as a pull request ?

Thanks

@narfbg

This comment has been minimized.

Show comment
Hide comment
@narfbg

narfbg Jul 8, 2014

Contributor

It is not obsolete, just not functionally required for working with objects in the general case. It does however improve memory usage and load_class() returns a reference, so your modifications trigger E_STRICT notices.

Anyway, I doubt that this error can be triggered by __get() alone, you're probably doing something in a constructor too.

Contributor

narfbg commented Jul 8, 2014

It is not obsolete, just not functionally required for working with objects in the general case. It does however improve memory usage and load_class() returns a reference, so your modifications trigger E_STRICT notices.

Anyway, I doubt that this error can be triggered by __get() alone, you're probably doing something in a constructor too.

@narfbg narfbg closed this Jul 8, 2014

@Aphax

This comment has been minimized.

Show comment
Hide comment
@Aphax

Aphax Jul 8, 2014

Well, the only errors occuring comes from Codeigniter core. And that's because of pass-by references to unseted properties. Like this line :

$this->$var =& load_class($class);

It seems to be more a php concern than a Code Igniter one, though.

So my question still remains :
Does this justify a correction ? Removing the pass-by reference who is no longer needed with PHP 5 and maybe a pull request for the auto loading models option ?

Thank you.
(Sorry for miss logging, I'm the author of the first message)

Aphax commented Jul 8, 2014

Well, the only errors occuring comes from Codeigniter core. And that's because of pass-by references to unseted properties. Like this line :

$this->$var =& load_class($class);

It seems to be more a php concern than a Code Igniter one, though.

So my question still remains :
Does this justify a correction ? Removing the pass-by reference who is no longer needed with PHP 5 and maybe a pull request for the auto loading models option ?

Thank you.
(Sorry for miss logging, I'm the author of the first message)

@narfbg

This comment has been minimized.

Show comment
Hide comment
@narfbg

narfbg Jul 8, 2014

Contributor

No, as I said - the problem is most probably not in the reference, but in some piece of code that you're not showing. If the reference was a problem, errors would be triggered with or without your __get() method.

Contributor

narfbg commented Jul 8, 2014

No, as I said - the problem is most probably not in the reference, but in some piece of code that you're not showing. If the reference was a problem, errors would be triggered with or without your __get() method.

@Aphax

This comment has been minimized.

Show comment
Hide comment
@Aphax

Aphax Jul 8, 2014

If you think so, I won't insist, thanks for your time !

Aphax commented Jul 8, 2014

If you think so, I won't insist, thanks for your time !

@terrylinooo

This comment has been minimized.

Show comment
Hide comment
@terrylinooo

terrylinooo Jun 11, 2016

Same problem here. I do the same thing as OP did.

Do you solve this issue?

terrylinooo commented Jun 11, 2016

Same problem here. I do the same thing as OP did.

Do you solve this issue?

@bcit-ci bcit-ci locked and limited conversation to collaborators Jun 11, 2016

@narfbg

This comment has been minimized.

Show comment
Hide comment
@narfbg

narfbg Jun 11, 2016

Contributor

This is a bug tracker, not a forum board.

Contributor

narfbg commented Jun 11, 2016

This is a bug tracker, not a forum board.

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