[#12960] - Allow 'default' in path list to allow fallback for overrides.
- Closed
- 25 Nov 2016
- Medium
- Build: staging
- # 12960
- Diff
- Spudley:staging
- Success continuous-integration/drone the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Summary of Changes
Consider the following custom form field class:
class JFormFieldMytype extends JFormFieldSubform
{
protected $type = 'Mytype';
protected $renderLayout = 'mytypefields';
protected function getLayoutPaths()
{
return array(JPATH_ROOT . '/modules/mod_mymodule/layout/');
}
public function getInput()
{
// load a bunch of field definitions as XML, and set them into $this->formsource.
return parent::getInput();
}
}
This class extends the Subform field type to allow me to dynamically generate a group of fields in my module config and render the subform using a custom layout.
However, an unwanted side-effect is that the fields within the subform also inherit the custom layout path. I don't have custom layouts for all the standard field types, so they fail to render, because when you override the layout, JLayoutFile does not retain the defaults as a fall-back.
An interim solution would be to force the defaults back into the path list. The code above would be changed to look like this:
protected function getLayoutPaths()
{
$dummyRenderer = new JLayoutFile('default');
$defaultPaths = $dummyRenderer->getDefaultIncludePaths();
$layoutPaths = array(JPATH_ROOT.'/modules/mod_mymodule/layout/');
return array_merge($layoutPaths, $defaultPaths);
}
This is not ideal because (1) it's ugly code, (2) it would need to be repeated if you create multiple classes, and (3) it creates a dummy instance of JLayoutFile just for the purposes of getting the default paths, which are then promptly passed back into an existing instance of the same class. However, it is the solution I've gone with in my module code for now, as I need to use my module now, ahead of this PR being accepted and released.
The longer term solution is submitted here. In this PR, I have modified JLayoutFile such that you may include 'default' in your layout paths array. This will be treated as a special case and replaced with the default paths, thus allowing a fall-back.
We can now write our getLayoutPaths() method like this:
protected function getLayoutPaths()
{
return array(JPATH_ROOT . '/modules/mod_mymodule/layout/', 'default');
}
This will be particularly useful in field types that extend subform, as noted above, but will be available to any field type, or indeed anything that uses JLayoutFile. But because 'default' has to be added explicitly to the paths array it will not affect any existing code, nor are any APIs or method signatures changed.
Testing Instructions
- Create a custom field type overriding subform as detailed above. Have it load some standard fields types in the subform.
- Create a custom layout file for the custom subform field type, but not for any of the standard types within it.
- Without this PR, the fields within the subform will not be rendered.
- After the PR, the fields will be rendered correctly.
Documentation Changes Required
Documentation for creating custom field types and custom layouts is currently very weak.
The Custom field types page doesn't even mention being able to use custom layouts. I know this is a niche feature, but it would be good to at least mention it, and include the fact that you can use 'default' as a fallback.
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
|
||
joomla-cms-bot
- | Category | ⇒ | Libraries |
@phproberto parent::getLayoutPaths() just returns an empty array, so doesn't solve the problem.
That would be a correct patch: force JFormField::getLayoutPaths() to return the default list layout paths.
Meanwhile you can do what I did for the stats plugin fields:
https://github.com/joomla/joomla-cms/blob/staging/plugins/system/stats/field/base.php#L26
Okay. I did consider patching JFormField::getLayoutPaths() instead to return the default paths; that was one of the options I tried out. But that seemed to me like it could change behavior unexpectedly. Also, it didn't address the problem for any other consumers of JLayoutFile.
That was the reason I put the patch where I did -- so that (a) it doesn't change behavior for any code that doesn't explicitly expect it, and (b) it is available for any class that uses JLayoutFile.
I looked at a few other options as well:
Add a config or something similar to
JLayoutFilethat tells it to include the default paths if the config is set. Unfortunately, this would still be inaccessible toJFormField::getLayoutPaths(), so doesn't really help.Change
JFormField::getRenderer()such that the instance ofJLayoutFileis passed intogetLayoutPaths()as an argument. This seemed like quite a clean option to me. It allows me to write my original code without having to instantiate a dummy instance ofJLayoutFile. It does mean a change in method signature, but it's an additional argument, so won't affect anyone not using it. The downside is that again it doesn't help other consumers ofJLayoutFile, but if I accept that's not necessary then perhaps this is the better option?
Okay. I did consider patching JFormField::getLayoutPaths() instead to return the default paths; that was one of the options I tried out. But that seemed to me like it could change behavior unexpectedly. Also, it didn't address the problem for any other consumers of JLayoutFile.
- If doesn't change current behavior because JLayoutFile will load the default paths if no paths are specified so it's fully B/C. Also it's not only ok but also recommended to have a renderer with default paths specified.
- There are only a few places where this can happen. We don't need 100 options to configure JLayoutFile. Just use it right and document its usage. Preconfigured renderers (what you are trying to fix) should only be used in modules, plugins, view & fields. And you can already override the getRenderer() method or the getLayoutPaths(). I don't care about other consumers of
JLayoutFile. We aren't talking about something that isn't possible (load default include paths) but something that you think is too complex.
Adding an array with a value that is magically converted to another array is not the solution. If you need 4 lines to configure something but is easy to understand it's better than 1 line of magic that you cannot get.
You have a lot of options and you are picking the wrong one:
- Create your own renderer (extending JLayoutFile) with its own default include paths that will be used everywhere.
- Override getRenderer() to add your include path after parent class has returned the renderer.
- Send a PR that forces parent class to return always default paths (what I suggested that could be an acceptable global solution).
Fair enough. Point taken. :)
By the way, I didn't intend to come across as argumentative or disagreeing with you; I was just trying to explain my reasoning process. I'll update the PR as suggested when I have a bit of time available to do so.
@phproberto -- okay, I have now updated the PR as discussed.
JFormField::getLayoutPaths() is now given the JLayoutFile object by getRenderer(), and uses it to set the paths to the default paths as defined by the instance of JLayoutFile provided.
I've set the argument as optional and it only gets the defaults if its passed. This is to avoid breaking any existing code that might override the method and then call the parent without any arguments.
My previous change has been removed.
Ah damn. I've screwed up this PR now.... I'm going to withdraw it and start again.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-11-25 15:56:13 |
| Closed_By | ⇒ | Spudley |
No problem. I do that all the time
@phproberto - would you be able to have a look at the replacement PR I raised? It would be great to get some feedback on it. Thank you :) Also thank you for the feedback on this original PR; it was really helpful.
You should use:
BTW you should provide a way to override the layout in your module too.