Proposal to improve/fix block template loading and package overrides
Permalink
I've just been implementing a theme and wanted to provide template overrides for another package (in this case problog).
This plainly didn't work and the only option was to put them in `<root>/blocks/`. This is horrible for both the theme vendor and user so I decided to see if it was fixable.
Much pain and suffering ensued.
The good news is that my theme could override blocks from other packages if only BlockViewTemplate->createView() searched the package of the current theme last rather than in an arbitrary order. This indeed fixed my issue and my theme can now provide overrides for custom templates provided by problog.
Rather than stop there I tried to get my head around the rest of the code and I think it will be possible to for themes/packages to override the default view template as well (rather than just custom templates). It could also be made considerably DRYer.
I've created a gist (https://gist.github.com/pietschy/7241034) with my thoughts, 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: '.
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???
For those who didn't read the gist here's a summary of the comments.
Summary of functional issues:
- Currently only custom templates can be overridden by packages, it's not possible for a package to override /<block_handle>/view.php.
- While packages can override custom templates, the search order is arbitrary so the current theme may or may not win.
Summary of implementation issues:
- It's quite hard to follow.
- The package search code is a bit weird and I think there's logical errors. See comments below in the packages loop.
- There's heaps of roll your own paths that should be delegated to the things that know where they live..
E.g. The package search code could should use `$pkg->getPackagePath()` and should probably also use something like `$pkg->getAssetPath`.
- The custom template override if/else structure seems to be wrong.
Functional Goals:
- Allow packages to override the default views as well as custom templates.
- Give the current theme (if it's in a package) the second highest priority (below root level blocks dir) so it gets a chance to override templates from other packages (i.e. so my them can customise problog!).
PLEASE NOTE: the code below is already temporarily doing this with the new method getPackagesInTemplateSearchPriority().
Implementation Goals:
- Improve the readability of the code so the algorithm is more visible at a conceptual level i.e. search these locations for these styles. i.e.
- get the code for handling the different template styles out of the main flow (e.g. block_handle.php or block_handle/view.php)
- create the notion of template search locations (root, package of the current theme, other packages, core, block).
- Ideally I'd like to separate the notion of the thing the finds the template from the template itself.
This plainly didn't work and the only option was to put them in `<root>/blocks/`. This is horrible for both the theme vendor and user so I decided to see if it was fixable.
Much pain and suffering ensued.
The good news is that my theme could override blocks from other packages if only BlockViewTemplate->createView() searched the package of the current theme last rather than in an arbitrary order. This indeed fixed my issue and my theme can now provide overrides for custom templates provided by problog.
Rather than stop there I tried to get my head around the rest of the code and I think it will be possible to for themes/packages to override the default view template as well (rather than just custom templates). It could also be made considerably DRYer.
I've created a gist (https://gist.github.com/pietschy/7241034) with my thoughts, 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: '.
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???
For those who didn't read the gist here's a summary of the comments.
Summary of functional issues:
- Currently only custom templates can be overridden by packages, it's not possible for a package to override /<block_handle>/view.php.
- While packages can override custom templates, the search order is arbitrary so the current theme may or may not win.
Summary of implementation issues:
- It's quite hard to follow.
- The package search code is a bit weird and I think there's logical errors. See comments below in the packages loop.
- There's heaps of roll your own paths that should be delegated to the things that know where they live..
E.g. The package search code could should use `$pkg->getPackagePath()` and should probably also use something like `$pkg->getAssetPath`.
- The custom template override if/else structure seems to be wrong.
Functional Goals:
- Allow packages to override the default views as well as custom templates.
- Give the current theme (if it's in a package) the second highest priority (below root level blocks dir) so it gets a chance to override templates from other packages (i.e. so my them can customise problog!).
PLEASE NOTE: the code below is already temporarily doing this with the new method getPackagesInTemplateSearchPriority().
Implementation Goals:
- Improve the readability of the code so the algorithm is more visible at a conceptual level i.e. search these locations for these styles. i.e.
- get the code for handling the different template styles out of the main flow (e.g. block_handle.php or block_handle/view.php)
- create the notion of template search locations (root, package of the current theme, other packages, core, block).
- Ideally I'd like to separate the notion of the thing the finds the template from the template itself.
