[#13225] - [com_fields] New methods in JFormField
- Closed
- 1 Jan 2017
- Medium
- Build: master
- # 13225
For the custom fields feature, 3 new methods have been added to JFormField. As I wrote in #13224, their names are not descriptive and need to be changed. However specifically looking at JFormField::appendXMLFieldTag() (whatever that really does), I don't see why we need JFormField::postProcessDomNode(). A class that extends from JFormField can simply override JFormFIeld::appendXMLFieldTag(), call the parent method and then modify the node when the parent method returns, instead of providing yet another (undocumented) method.
Regarding JFormField::getFormParameters(): Please use the directory lookup methods from JFormHelper (or rather add a addFieldFormPath() method to it) to do the lookup of the fields forms. If I simply want to provide my version of the XML file, I now have to copy both the field and the XML. It is also inconsistent with our other JForm code.
This is part of a code review of the fields feature. See #13222, #13223, #13224 and other connected issues.
joomla-cms-bot
- | Labels |
Added:
?
|
||
Honestly, "hooks" like this really feel like anti-pattern. You really should be either extending the parent method or using an event system for hooks.
I did not say that any form include paths should be changed. I'm saying that we are using JFormHelper to look up rules, forms and fields and all of a sudden a part of JForm uses a completely different method of finding a file. If we don't give a crap about consistency, we might as well go the Virtuemart 1.x route and reimplement str_pad() 3 times in 3 different broken ways. Besides that, I agree with @mbabker
| Title |
|
||||||
| Title |
|
||||||
Closing since we have a PR to remove those functions.
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-01-01 17:02:32 |
| Closed_By | ⇒ | Bakual |
This is just a helper function which produces an XML string to be loaded into a form. It should not manipulate the form include paths.
I thought it would be more clear to have a function for classes to override than overriding the full function.