Design flaw in core URL helper

[witrin]

Hi,

I think the implementation of the method Mage_Core_Helper_Url::getCurrentUrl() should be moved into the core URL model. In early versions you have used the core URL model for its implementation:

return $this->_getUrl('*/*/*', array('_current' => true, '_use_rewrite' => true));

Without knowing why you have changed this, but now it is very laborious to override its behaviour. This is because, the core URL helper is currently a base class for eight other helper classes.

Let's suppose you want to override how URLs are generated (e.g. to integrate Magento with TYPO3). Now you have to override the helper itself respectively its derivation beside the core URL model.

Thus I recommend to make the new implementation also be part of the core URL model. This should make it cleaner and more flexible.

If you agree with me, I would create a pull request for this issue.

Best regards,
Artus

[magento-team]

Hello. Thanks for the feedback. We will analyze this idea and get back to you.

[magento-team]

Artus --
Your contribution is welcome. The changes in Mage_Core_Helper_Url::getCurrentUrl() were done to fix a bug in maintaining port in URL, but should have been done in Mage_Core_Model_Url.

So recommended contribution is to revert the helper back to use URL model, and make sure the test case with port is maintained. The test case:

• setup a Magento instance with port in base URL -- i.e. http://example.com:8080/magento/
• invoke the Mage_Core_Helper_Url::getCurrentUrl()
• expected result: the generated URL has 8080 port

[verklov]

Fixed by #245 . Closing this issue.