search

concretecms / concrete5-legacy

Legacy repository for concrete5
http://www.concrete5.org
560 stars 323 forks source link

Proposal to improve/fix block template loading and support package overrides #1482

Closed pietschy closed 10 years ago

pietschy commented 11 years ago

Hi all,

I've just been implementing a theme and wanted to provide template overrides for another package (in this case problog). This didn't work and the only option was to put them in <root>/blocks/. This situation is pretty painful from the perspective of both the theme developer and end user so I decided to see if it was fixable.

In looking at the code I found that themes (in packages) could override custom block templates from other packages, but it's random chance and only happens if the theme is the last package searched by BlockViewTemplate->createView().

I implemented a quick fix to always search the package of the current theme last. This fixed the issue for custom templates.

Rather than stop there I tried to get my head around the rest of the code and I think it might be possible to for themes/packages to override the default view template as well (rather than just custom templates). It think it could also be made considerably DRYer with the use of a couple of standard design patterns.

I've created a gist (https://gist.github.com/pietschy/7241034) with my thoughts along, comments and findings before I go changing too much (I've already made the theme the last package to search). My code comments/notes are prefixed with 'AP: '.

Please Note: the gist is based on 5.6 but 5.7 looks the same except for the addition of the new asset handling (which would continue work as is in the approach I'm proposing).

I'd be happy to have a crack at this. Does anyone see any gotchas, snake holes or other reasons why I should run away???

herent commented 11 years ago

I tend to use custom templates, like customer_blog or theme_handle_blog in my theme packages. Then you can just do $a->setCustomTemplate('problog_list', 'customer_blog'); before rendering. Seems to work OK, but it's not quite what you need.

I would worry about how this affects package upgrades. If PB changes something in how that template is created, your override might not work any more. Then who gets the support request? It's your addon causing the problem, but I'm pretty sure that RadiantWeb is going to get the call...

pietschy commented 11 years ago

Yeah, I tried the $a->setCustomTemplate(..) method but it has a couple of big drawbacks. It hits the db and actually updates settings, it doesn't work when the add-on uses the a default template rather than a custom one (as pro blog does) and it also doesn't work when the add-on is already using a custom template (as pro blog also does). In the end I was left with a fairly ugly mix if custom templates & root level block overrides.

I'm not sure on the upgrades thing since I don't have marketplace experience. I guess we'd have to get an idea if add-on developers would prefer easy theme-ability over and above any support calls. Theme developers would certainly need to indicate which versions of the add-ons they work with if they were providing overrides.

aembler commented 10 years ago

I'm going to move this over into the 5.7 branch because I think better discussion about it should happen over there, since it's a fairly significant enhancement, and we've overhauled a lot of the underlying code (so there should be some flexibility to make stuff like this happen in 5.7.)

aembler commented 10 years ago

https://github.com/concrete5/concrete5-5.7.0/issues/9