Useless Mage::getModel() in Observer.php?

[markvds]

First of all: thanks for the module! It saves me a lot of time.

The thing I don't get is why this line is in place: https://github.com/magento-hackathon/Sandfox_GeoIP/blob/master/app/code/community/Sandfox/GeoIP/Model/Observer.php#L7

There are two things about it:

• I'm not really sure why this needs to be done on every controller_front_init_before. I can imagine a situation where you determine the country of the visitor one time and then store the country in the session. In that case, we don't need the GeoIP anymore after the first call.
• Why isn't it loaded as a singleton, like you suggested yourself in the docs?

Please note that geoip/country is initialized at each Magento load. In order to save system resources please always use it as a singleton.

[Schrank]

I think you are right, my impression is too, that this is a bug. Your PR is welcome :)

[markvds]

And what is a bug? The whole observer (which I think is the bug), or only the use of Mage::getModel()?

[Schrank]

Imho the bug is, that it is not a singleton. I think it is intended, that the geoip is initialized.

[schmengler]

It doesn't matter, getModel() is called once per request and singletons are not persisted between requests.

(this means, using getSingleton() would actually produce more overhead, although it's negligible)

Forget it, the object is intended to be used later on in custom blocks or templates, so a singleton would make sense.

[Schrank]

I disagree. Using getSingleton makes the geoIp and country available through the request. At the moment, the init is done but no consequence is produces, or I'm wrong? If you want to know which country the customer is from, you are doing the init again before getting the country

[schmengler]

Yes, I just updated my comment. It really should be changed.

[Schrank]

Maybe better in the registry? Just an idea :)

[markvds]

But then again: what should be changed? The model gets initialized anyway the first time you reference the singleton. There is no need to do so on every front controller init.

[markvds]

@Schrank Singletons are stored in the registry, so no need to store it explicitly in the registry. As long as you always use a singleton.

[Schrank]

I tend to remove the observer completly

[seansan]

+1