Replaced hard coded "new" object creation with Magento friendly way

[woutersamaey]

Replaced hard coded "new" object creation with Magento friendly way, allowing other modules to rewrite these objects.

[Flyingmana]

Please use @deprecated only for classes, which are intended to get removed completely, not as a workaround to mark direct instanciation.

Also for reviewers: check usage is only changed, where magento is already loaded, there are probably uses where this will fail now, because magento is not loaded yet.

[woutersamaey]

So I remove the @deprecated word?

[Flyingmana]

yes, please :)

[woutersamaey]

I removed the 3 @deprecated words

[sreichel]

Also for reviewers: check usage is only changed, where magento is already loaded, there are probably uses where this will fail now, because magento is not loaded yet.

I dont sse problems with changes inside app/core/Mage, but there schould be no calls to Mage:: inside lib/Varien ...

[colinmollenhour]

Thanks for the PR!

Can you explain the use case for this in more detail?

Also, did you consider that you can override any class file by dropping classes with the same name into app/code/* directories (due to ordering of include paths)?

My thinking is that the lib/Varien files are utility libraries (akin to the Zend libs) and not really meant to be extended with new user-specific functionality like the models and blocks are, and if there are bugs or lacking features it is best to just submit fixes/improvements to the core code rather than override them. And if you must override them or can't wait for a PR to be merged there is always the copy/modify option with the include path.

[woutersamaey]

We have created a module that uses Amazon S3 as primary media storage without using the media dir on the disk. Other modules have S3 or other storage options too, but in our case the media dir is completely empty.

To build this, we had to dig deep in Maganto and found these models to be not easily rewriteable.

You’re right about the include path etc, but I would prefer Magento to load models in the same way, as Magento tells us to do. Why the ‘new’ keyword is used at all seams to break Magento’s own rules without valid reason.

So I can understand your workaround, but my thinking was to have an easy and clean solution that does not need any workarounds.

I’d even endeavor to remove all uses of ‘new’ as much as possible. Let devs rewrite any and all
models they like.

I understand Varien is a lib, but Magento always extends lib files and then uses these, while also loading them the Magento way. Some examples here are the Zend_Db classes. These (and others) are all wrapped in ‘Mage_’ classes.

So hope you like this PR. There is always an alternative, but my goal is following best practises even when Magento forgot their own rules.

[colinmollenhour]

Thanks for the additional detail. You make a good case and I think I am more convinced than before so no objections.

As an aside, any chance your S3 module will be open-source? :)

[woutersamaey]

The S3 module actually needs more core edits, but it's very hard for me to post them here and get everyone to agree on them. So, let's first try to get this one in...

[sreichel]

Are the changes to lib/Varien required if you use your added wrapper classes?

[tmotyl]

This kind of change has a significant risk of introducing some side effects.
Maybe we are missing some api or event which would make the whole architecture of your extension clearer and you would not need to override core classes any more?

Can you describe implementation details of your change?

[woutersamaey]

@luigifab I fixed these few nit-picks

[fballiano]

I love how @luigifab is our syntax police (I really do, that's not sarcastic) :-)

[woutersamaey]

We should use PHP CS Fixer for these nit-picks. Devs really have better things to do imo.

[ADDISON74]

From what I understand this PR comes as a proposal to solve a particular situation encountered in Magento. At first glance, the changes are made correctly, but what are the implications in the wrong sense?

If this PR will be merged I would change the phrase bellow is by removing the comma:

Instead of creating instances using "new", use Mage::getModel('core/varien_file_uploader')

[fballiano]

Are the changes to lib/Varien required if you use your added wrapper classes?

I agree with @sreichel that it's very weird to see Mage:: in "lib/", is it absolutely necessary for some reason?

[luigifab]

I tried this:

echo get_class(new Varien_Io_File()),"\n";
echo get_class(Mage::getModel('Varien_Io_File')),"\n";
echo get_class(Mage::getModel('varien/io_file')),"\n";

Results are:

Varien_Io_File
Varien_Io_File
Varien_Io_File

So, Mage_Core_Model_Varien_File_Uploader + Mage_Core_Model_Varien_Image + Mage_Core_Model_Varien_Io_File are not necessary?

[fballiano]

I wouldn't have this in v20 only, it will create too many merging problems.

Also, there are conflicts to be fixed, I'm setting it to draft.

[ADDISON74]

Use Mage::getModel('core/varien_file_uploader') instead of creating instances with "new"

[fballiano]

anyway files in "lib/" should never have references to "Mage::" as Sven pointed out

[ADDISON74]

I only wanted to correct a few sentences, This PR has some important issues that need to be discussed.

[fballiano]

yep for sure, if we merge luigifab PR (which is similar) I'll take this one further but I without the "lib/" part ;-)

[ADDISON74]

Use Mage::getModel('core/varien_image') instead of creating instances with "new"

[ADDISON74]

Use Mage::getModel('core/varien_io_file') instead of creating instances with "new"