Removing unnecessary file inclusion and using Magento's built in meth…

[edward-simpson]

…ods to load required models and blocks

Magento isn't meant to be filled with include_once, I have gone through the module, fixed class name declarations to be compatible with Magento, and modified functions to use Magento's Mage::getMethod(), Mage::getBlockSingleton() where necessary

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[dmitridr]

Hello,

Thanks for the pull request, but I'm not sure we could accept it. Would you mind going into detail why these includes you have removed are not necessary? We found in the past that the plugin breaks and causes server 500 if compilation mode is enabled without these statements that you have removed being present.

[dmitridr]

I took a second look and it seems reasonable that we can accept this if we test it thoroughly, though I'm still worried it will break compilation mode. Please accept the CLA and we can test this ourselves, though it would be helpful to know if you've tried testing it on compilation mode also.

[edward-simpson]

Hi Dmitri,

Magento autoloads all classes declared in the etc/config.xml file (even if you just declare Mage_Core_Model it will autoload everything else under Mage_Core_Model_*, then using Mage::getModel, Mage::getSingleton, Mage::getBlockSingleton, it will load the desired class.

I've done a bit of testing my end but let me know if you run into any issues and I'll fix what I can!

I've already completed the CLA so my account should be on your system

Kind regards,

Edward

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

@edward-simpson has updated the pull request. Re-import the pull request

[dmitridr]

Hi Edward,

I wanted to give you an update : we are looking at this change and testing it locally to make sure everything is fine on different versions, we will probably merge it sometime next week, but we will send out v2.6.1 with more critical fixes first, including the fix in your other PR.

Thanks for sending us this! I think this makes the code a lot nicer, and we weren't aware of this kind of approach, which is why we used includes.

If you see further problems similar to this, we'd love to hear about it (either via PR or otherwise).

Cheers, Dmitri

[edward-simpson]

Hi Dmitiri,

No problem at all, happy to help, send me a message any time if you want me to take a look at anything!

Kind regards,

Edward

[facebook-github-bot]

@edward-simpson has updated the pull request. Re-import the pull request

[edward-simpson]

Hi Dmitri,

What's the latest timeline on this being merged?

Kind regards,

Edward

[dmitridr]

Hi Edward,

Sorry for the delay, we're aiming to merge this for the next version of the plugin, which should be released by next week.

Dmitri

[facebook-github-bot]

@edward-simpson has updated the pull request. Re-import the pull request

[dmitridr]

Rebased.

[dmitridr]

Thanks for your patience @edward-simpson, we have merged this and we'll finally add this in the next version. I did discover another bug, common.php needed to be renamed to Common.php for the changes to work properly.

Thanks again for the pull request, I think this is a good step to make our code cleaner.

Cheers, Dmitri.