Skip to content
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

core/template block should use a helper or a model #12

Closed
vsushkov opened this issue Apr 19, 2012 · 20 comments
Closed

core/template block should use a helper or a model #12

vsushkov opened this issue Apr 19, 2012 · 20 comments
Assignees

Comments

@vsushkov
Copy link

core/template block should use a helper or a model to render a template. If it will be so than anyone can rewrite this helper or model and implement, for example, HAML templates support. At the moment the only way to do so is to place Mage_Core_Block_Template in app/local folder.

@paulchubatyy
Copy link

Also it would be nice to have each renderrer defined per module basis.

@stalniy
Copy link

stalniy commented Apr 27, 2012

Do you want to rewrite base Magento theme to HAML?

@vsushkov
Copy link
Author

@stalniy I haven't decided yet. Maybe slim :)

@jmspldnl
Copy link

jmspldnl commented May 2, 2012

@vsushkov Why wouldn't you just create a new block that extends from the core/template block? Then you would only need to override the render functions with code that processes your new template type, and then make sure your template files use your new block.

I guess I'm confused on how a helper or model would provide any additional functionality that doesn't already exist.

@vsushkov
Copy link
Author

vsushkov commented May 3, 2012

Why wouldn't you just create a new block that extends from the core/template block?

@jmspldnl Because if I'd like to write all my catalog templates on HAML, I'd have to create a thousand of blocks.

@jmspldnl
Copy link

jmspldnl commented May 4, 2012

@vsushkov I don't really see how a helper would "help" in this situation though. You would only really move the changes into the helper, but you would still have to rewrite the helper to make it work.

If you are trying to do this in your magento-haml repo, then I'm not sure you are going about it the best way. It's bad practice to copy core files directly into your local path. You really should "rewrite" the core/template block with a block that extends from it and only override the functions you need to change. I would think that you could do some kind of if/else logic so that a template that ends with .haml would call the haml parser.

@magento-team
Copy link
Contributor

Hi @vsushkov,

thank you for the proposal. We're working on it. Please stay tuned.

@ghost ghost assigned magento-team May 5, 2012
@vsushkov
Copy link
Author

vsushkov commented May 5, 2012

@mage2-team thank you! :)

@jmspldnl

You really should "rewrite" the core/template block with a block that extends from it and only override the functions you need to change.

It won't help as soon as I cannot (=I don't want) to rewrite all standard Magento blocks and extend them from my own template class. Rewrites are used only in createBlock factory methods. It's long to explain, just google about it.

but you would still have to rewrite the helper to make it work.

No, it's not necessary if this helper will iterate through all declared template processors (like Magento iterates through total models, payment methods etc.)

@stalniy
Copy link

stalniy commented May 5, 2012

Something like this i think will be nice

Mage_Core_Block_Template extends Mage_Core_Block_Abstract {
protected static $_defaultRenderer;
protected $_renderer;

public static function setDefaultRenderer(Mage_Core_Block_Renderer_Interface $renderer) {
    self::$_defaultRenderer = $renderer;
}

public function setRenderer(Mage_Core_Block_Renderer_Interface $renderer) {
    $this->_renderer = $renderer;
    return $this;
}

public function getRenderer() {
    return $this->_renderer ? $this->_renderer : self::$_defaultRenderer;
}

public function renderView()
{
    $this->setScriptPath(Mage::getBaseDir('design'));
    $html = $this->getRenderer()->render($this->getTemplateFile());
    return $html;
}
............................................................................................
}

How to use:

  • using "core_block_abstract_to_html_before" you can change renderer for each block before rendering
  • somewhere in the code (e.g. in index.php, but i'm not sure if this is the best place) Mage_Core_Block_Template::setDefaultRenderer(new HamlRenderer($someConfigurations));

@jmspldnl
Copy link

jmspldnl commented May 7, 2012

@vsushkov that makes a little more sense now. So you're saying you would essentially define template parsers/processors in config, and then the helper would iterate through the config to determine which parser to call. Let's see what the @mage2-team comes up with, but I think it's a little more clear for me now.

@magento-team
Copy link
Contributor

After careful examination of the subject by the team and product management it was considered that implementing such feature can have negative impact on Magento community.
Supporting different template types in one system makes its support more complicated. Since every module may contain templates in different formats, a developer should be familiar with all kind of template engines in order to work with a system.
In case of discovering a bug in such a system it becomes not clear whether it's caused by the Magento core or custom template renderer functionality.

Magento 2 roadmap contains stories, which require changes of view layer design. If proposed feature would be implemented, it may appear to contradict with the upcoming view layer refactoring.

Thank you for the proposition.

@LeeSaferite
Copy link

So basically you don't want a flexible system, you just want us to do it your way or not at all? Why make anything extendable at all then?

@stalniy
Copy link

stalniy commented Jun 8, 2012

I agree with comment above. You just need to implement interface and PHPRenderer. You dont need to support all types of templates! You dont need to change module structure. Just give interface for devs thats' all. You can check how Symfony2 team did this type of work. I think they did it pretty well!

@colinmollenhour
Copy link

You can always override Mage_Core_Block_Abstract in app/code/community or app/code/local if you really must use a different templating system. You can even release it as a module on Magento Connect. For me, PHP is fine and I agree that this is not a good feature to be supported in the core but that doesn't stop anyone from using another templating system, it just means it isn't officially supported.

@LeeSaferite
Copy link

You seem to miss the point.

He said he would like the option of writing all the templates for his site using a different view renderer. If he had to extend Mage_Core_Block_Abstract to make this happen, then he would have to re-implement every block in the system basically.

There is no valid reason this couldn't or shouldn't be done. Hard coding these things when the whole premise of the framework is modularity is foolish to say the least.

ETA: I didn't catch that you meant to use the include path to hide the real abstract template. I would say this violates the spirit of the modular system as you are not adding on, you are hiding a portion of the core code. So, while it would work, I would find this method highly sub-optimal.

@colinmollenhour
Copy link

Not that it matters anyway because the Magento 2 team has already respectfully rejected this idea, but while the idea sounds great, I think it is much more complicated to add support for and rewrite all of the templates in HAML or Smarty or some other templating system than you think and you'd probably just give up in the end. If you really want to, override MCBT in a different code pool. Why is that so wrong? That's exactly what the code pool hierarchy is for! If you really can't bring yourself to override this one file that almost nobody else would override, then change all of your .phtml templates to simply load the haml templates!

<?php Mage::helper('xxx')->renderHaml($this);

@stalniy
Copy link

stalniy commented Jun 9, 2012

Extending from Mage_Core_Block_Template will cause a lot of copy-paste it's bad because you will need to support each block file and rewrite it after each Magento release. Rewriting of Mage_Core_Block_Template using Magento rewrite system will cause a conflict with other extensions if somebody make rewrite for another purpose. If you use code pool local it's also bad, because it's illogically to have in module structure 2 folders My_Haml and Mage. And if somebody have rewrite for Mage_Core_Block_Template using code pool (for example some programmer did it on some site) and customer found a very cool extension on MagentoConnect that also implements rewrite for this block in code pool and then he install it and will get a lot of problem. Because if you system is not under CVS it will override file in local code pool and all changes will be missed.

If implementing of such feature is really complicated it will be nice to read why, because for now mage2-team said only that it's complicated to support. It would be nice to read some examples of test case. If they did "careful examination of the subject", they must have more logical explanation than we see in the comment above. It's my opinion.

@antonmakarenko
Copy link

UPD: recently Magento team implemented support of twig templates along with phtml-templates. It resulted in factoring out the template renderer out of the Mage_Core_Block_Template. The update for now is in internal repository, not rolled out yet (what you may see in 04446b6 is an experimental prototype which will be replaced with a new update).

Thanks everyone for your passion regarding this issue and sorry for confusion.

@colinmollenhour
Copy link

Sorry if this is the wrong place to ask this question, but I searched and there is no related issue.. Why abolish the use of code pools? If I must modify a core class then I must. Removing the feature won't deter me, it will just make it more annoying.

@sshymko
Copy link

sshymko commented Oct 22, 2013

UPD: Template engine abstraction, which allows a module/extension to introduce new template engine into the system, has been introduced. See the discussion #370 (comment).

@FabXav FabXav mentioned this issue Oct 11, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants