[#30143] - Form add parent for subforms
- Closed
- 21 Aug 2021
- Medium
- Build: staging
- # 30143
- Diff
- sfs-it:patch-6
- Success Download Prebuilt packages are available for download. Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/drone/pr Build is passing Details
- Failure continuous-integration/appveyor/pr AppVeyor build failed Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #
Subforms children cannot access to parent form settings, adding parent info can travel to extension stored info, like global settings.
Summary of Changes
file: libraries\src\Form\Form.php
- edit: __constructor
add options['parent'] for subforms - add: Form::getFormParent()
retrive form options['parent']
file: libraries\src\Form\FormField.php - edit: __constructor
to use setForm instead code duplication - add static FormField::subformsCache[]
to store request for subforms using con formControl like key. - edit setForm
to get form parent, and store to subformsCache forms parent. - add protected $subformParent, and $subformParent
to access to form parent form. - edit __get
to retrive $subformParent and $subformParent
file: libraries\joomla\form\fields\subform.php - edit call of instance of Form, adding parent option
files: libraries\joomla\form\fields\text.php, libraries\joomla\form\fields\number.php - edit getInput()
to use parents top link on com_menu, and retrive field parameters on component with subform prefix.
files: libraries\joomla\form\fields\list.php - edit getOptions()
to use parents top link on com_menu, and retrive field parameters on component with subform prefix.
Testing Instructions
Using com_content like example
add to com_content config ( administrator\components\com_content\config.xml:8 ):
<field type="subform" name="test" label="subform test">
<form>
<field type="text" name="text" label="text" default="text"/>
<field type="number" name="number" label="number" default="10"/>
<field type="list" name="list" label="list" default="1">
<option value="0">VALUE 0</option>
<option value="1">VALUE 1</option>
</field>
</form>
</field>
add to com_content article view ( components\com_content\views\article\tmpl\default.xml:36 ):
<field type="subform" name="test" label="subform test">
<form>
<field type="text" name="text" label="text" useglobal="yes"/>
<field type="number" name="number" label="number" useglobal="yes"/>
<field type="list" name="list" label="list" useglobal="yes">
<option value="0">VALUE 0</option>
<option value="1">VALUE 1</option>
</field>
</form>
</field>
On config settings show this
Actual result BEFORE applying this Pull Request
Expected result AFTER applying this Pull Request
Documentation Changes Required
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
I think a changes here is incorrect, the field should not care where it rendered in regular for or in subfrom.
By looking in example XML, you better use <fields> group instead of subfrom:
<fields name="test">
<field type="text" name="text" label="text" useglobal="yes"/>
<field type="number" name="number" label="number" useglobal="yes"/>
<field type="list" name="list" label="list" useglobal="yes">
<option value="0">VALUE 0</option>
<option value="1">VALUE 1</option>
</field>
</field>
and then:
$value = $params->get($this->fieldname);
should be like
$value = $params->get($this->group.'.'.$this->fieldname);
| Labels |
Added:
?
|
||
PHP code style standard
I've fix on my github all occourence of Code style error. I can send it to the same PR or I must open another one?
(sorry but I'm not very skilled to use this collaboration platform.
@sfs-it 1st of all thank you for your 1st pr
https://github.com/joomla/joomla-cms/pull/30143/files#diff-1a31ec6292bd41ba0bafe545bf8c1262R563
if i'm not wrong php 5.X doesn't allow this syntax
hope it helps
I think a changes here is incorrect, the field should not care where it rendered in regular for or in subfrom.
By looking in example XML, you better use
<fields>group instead of subfrom:<fields name="test"> <field type="text" name="text" label="text" useglobal="yes"/> <field type="number" name="number" label="number" useglobal="yes"/> <field type="list" name="list" label="list" useglobal="yes"> <option value="0">VALUE 0</option> <option value="1">VALUE 1</option> </field> </field>and then:
$value = $params->get($this->fieldname);
should be like
$value = $params->get($this->group.'.'.$this->fieldname);
formfield like list, number or text doesnt process group.
editing test on com_content/config.xml
<field type="text" name="text" label="text" default="text"/>
<field type="number" name="number" label="number" default="10"/>
<field type="list" name="list" label="list" default="1">
<option value="0">VALUE 0</option>
<option value="1">VALUE 1</option>
</field>
</fields>```
add 3 field (not a subform that can be placed on a separated file and used in a second place), I use some stuff on my projects that can be reused. They are real subforms, not 3 parameters like example, but the problem is the same.
No subform comunication with com_menu, no path of forms...
than on default.xml view
<fields name="test" label="subform test">
<field type="text" name="text" label="text" useglobal="yes"/>
<field type="number" name="number" label="number" useglobal="yes"/>
<field type="list" name="list" label="list" useglobal="yes">
<option value="0">VALUE 0</option>
<option value="1">VALUE 1</option>
</field>
</fields>```
no default proposed.
I propose a change for something that I find, your appointment can be right, but now, fields aren processed, i dont find any group path in documentation or debugging the program flow.
PHP code style standard
I've fix on my github all occourence of Code style error. I can send it to the same PR or I must open another one?
(sorry but I'm not very skilled to use this collaboration platform.
@sfs-it All fine. You made the fixes in this PR, that's absolutely right. PHP code style is OK now.
The other failing CI test by Travis is cause by what @alikon mentioned above, the short array syntax [] not working on PHP 5.3. Just change it to array() at the place mentioned by @alikon, and all should be ok.
Note: I try to help here with the formal things. But that doesn't mean that I think the PR is ok if these things are corrected. To me it seems to be the first place where we show the value of a global setting like this, Use Global (value), and so it seems not to be consistent with like we show it elsewhere. But it's not on me to decide if this change would be good elsewhere too (so at the end it will be consistently used everywhere where we have Use Global ). But independently of this display consistency aspect it might make sense to have the parent information in the subform, or it may not make sense. I can't really judge that because I am not an expert on these things. So let's hope for more feedback from others.
PHP code style standard
I've fix on my github all occourence of Code style error. I can send it to the same PR or I must open another one?
(sorry but I'm not very skilled to use this collaboration platform.@sfs-it All fine. You made the fixes in this PR, that's absolutely right. PHP code style is OK now.
The other failing CI test by Travis is cause by what @alikon mentioned above, the short array syntax
[]not working on PHP 5.3. Just change it toarray()at the place mentioned by @alikon, and all should be ok.Note: I try to help here with the formal things. But that doesn't mean that I think the PR is ok if these things are corrected. To me it seems to be the first place where we show the value of a global setting like this,
Use Global (value), and so it seems not to be consistent with like we show it elsewhere. But it's not on me to decide if this change would be good elsewhere too (so at the end it will be consistently used everywhere where we haveUse Global). But independently of this display consistency aspect it might make sense to have the parent information in the subform, or it may not make sense. I can't really judge that because I am not an expert on these things. So let's hope for more feedback from others.
I think that is a good idea to permit to create path for subform, in many conditions with subforms you can create a reusable stuff, without create a lot of code in xml, on my component use a lot of XS/SM/MD/LG settings, I've created a standard subform and loading it i cut a lot of code on xml config files.
I use some subset for different views of articles, and reusing and nesting them I can make my code very clean.
Shall be a good idea to use groups in parameters, but I think that must be another PR.
Thanks for your time.
@richard67 Hi, I fixed a lot of code style problems, but Travis and appveyor continue to give me a build fail, can you help me to understand why?
@sfs-it Appveyor seems to be unrelated, that happens from time to time. Travis log here https://travis-ci.org/github/joomla/joomla-cms/jobs/713218885 shows an error that a unit testfailed for a form input. This could be related to this PR, but I'm not sure:
There was 1 failure:
1) JFormTest::testGetInput
Line:1116 The method should return a select list.
Failed asserting that false is true.
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/form/JFormTest.php:1117
@sfs-it Appveyor seems to be unrelated, that happens from time to time. Travis log here https://travis-ci.org/github/joomla/joomla-cms/jobs/713218885 shows an error that a unit testfailed for a form input. This could be related to this PR, but I'm not sure:
There was 1 failure: 1) JFormTest::testGetInput Line:1116 The method should return a select list. Failed asserting that false is true. /home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/form/JFormTest.php:1117
I can create a new pull request changing all files of this without change the core code for test (some little like adding a empty check or an empty assignment), and check if same that test fail?
Because I cannot understand the problem of mine changes in that workflow.
Or that idea is a awful idea? Charging PR system of a null PR is definitely bad?
Excuse me for stupid questions.
There are quite a few places where the equal signs = are aligned and this PR unalign them. That is incorrect as cs for J afaik
Example (without taking into account the code change for $component)
after patch
$tmp = new stdClass;
$tmp->value = '';
$tmp->text = JText::_('JGLOBAL_USE_GLOBAL');
$component = (string) $this->element['useglobal'];
before:
$tmp = new stdClass;
$tmp->value = '';
$tmp->text = JText::_('JGLOBAL_USE_GLOBAL');
$component = JFactory::getApplication()->input->getCmd('option');
Guys, before continue , it need to clear what the PR tries to fix.
Because hardcoding subform related properties in to every field is wrong.
Guys, before continue , it need to clear what the PR tries to fix.
Because hardcodingsubformrelated properties in to every field is wrong.
If this way is wrong, I'll think about another way. The problem is that subform now doesnt have any way to comunicate to structure of form.
Another way should be to add info related to subform nesting only in form, but that way require a lot of interactions to discover the path of field, for every field in a subform.
The field should not care where it rendered, in regular form or in subfrom.
Each field has a $group property that define the "path" to the field inside a form.
To get a right value from the form data it should be queried with use of $group:
$value = $params->get($this->group ? $this->group . '.' . $this->fieldname : $this->fieldname);However whole #11911 designed in a way that it not allow to pick a "global value" if the field in the group.
Well, looks like it a bit bigger issue than I expected
The problem is that subform now doesnt have any way to comunicate to structure of form.
yeah, that also true, hm
upd: in normal case it is not an issue, because field already have own value that bind by
joomla-cms/libraries/joomla/form/fields/subform.php
Lines 399 to 402 in 4c7eb17
The problem is that subform now doesnt have any way to comunicate to structure of form.
yeah, that also true, hm
upd: in normal case it is not an issue, because field already have own value that bind by
joomla-cms/libraries/joomla/form/fields/subform.php
Lines 399 to 402 in 4c7eb17
The problem start when you have global references, and form item cannot access to main form parameters, with path.
in my opinion, we need 2 parameters, "path" and "top parent form". for solve this.
2.nd problem is that you can have a global setting stored in another component, for this "useglobal" shall be yes/true or compment name, also a global setting can be used for more than one parameter with prefix like 'children_' for children pages of menu item, but the component parameter is the same.
comunication children/parent subform shall be used by form property. now formfields comunicate with.. but cannot dive into, adding a method that return toParent parameters formitem dont need to dive anymore.
(excuse me for my horrible english)
After @Fedik comment I restart the PR context restarting form core code, I try to change global setting path of form.
Next hours send new code.
@richard67 this is a good idea or is better close this PR and send another with new code?
@richard67 this is a good idea or is better close this PR and send another with new code?
Whatever is more comfortable for you.
@richard67 this is a good idea or is better close this PR and send another with new code?
Whatever is more comfortable for you.
I think that leave full PR code shall be useful to a new commiter like me now to understand the process that create a commit, but I think that is improbable that someone start to read this PR to understand and a new one without old code shall be fast for understand how the code work without old code.
The problem start when you have global references, and form item cannot access to main form parameters, with path.
The problem that the field should not read the value from global, the value should be bind by the form class already (by the model).
This confusion introduced by #11911.
in my opinion, we need 2 parameters, "path" and "top parent form".
Yes and no. The field already have a group this is the path inside the form.
Maybe you just need to add parentGroup that may help.
So it will work like $this->parentGroup . '.' . $this->group . '.' . $this->fieldname to get a global value.
But need to test.
Maybe @Bakual also have some idea?
I still not very like it, but well, if people wants it
Showing the global value is already a bit hacky for regular fields, and I think there are already one or two instances where it doesn't work (can't remember which one).
So if it doesn't work for subforms, I wouldn't put to much effort into it, chances are you need to make a deal with the devil (aka write horrible code) to make that work.
That being said, I have no clue how subforms work. I've never used it myself.
Showing the global value is already a bit hacky for regular fields, and I think there are already one or two instances where it doesn't work (can't remember which one).
So if it doesn't work for subforms, I wouldn't put to much effort into it, chances are you need to make a deal with the devil (aka write horrible code) to make that work.That being said, I have no clue how subforms work. I've never used it myself.
For test I add on form constructor
$this->options['parent'] = isset($options['parent']) ? $options['parent'] : false;
$this->options['parent-group'] = isset($options['parent-group']) ? $options['parent-group'] : false;
and 2 functions for retrive it.
on subform where call form constructor
$parent = $this->form->getFormParent();
if ($parent === false)
{
$parent = $this->form;
}
$parentGroup = $this->form->getFormParentGroup();
if (!empty($parentGroup) && !empty($this->group))
{
$parentGroup .= '.';
}
$tmpl = Form::getInstance($formname, $this->formsource, array('control' => $control, 'parent' => $parent, 'parent-group' => $parentGroup . $this->group));
than on list formfield
$formName = $this->form->getName();
if (substr($formName, 0, 8) === 'subform.')
{
$form = $this->form->getFormParent();
$prefix = substr($formName, 8);
$formGroup = $this->form->getFormParentGroup();
if (!empty($formGroup)){
$prefix = substr($prefix, strlen($formGroup)+1);
}
if (!empty($prefix))
{
$prefix .= '.';
}
}
else
{
$form = $this->form;
$prefix = '';
}
// Get correct component for menu items
if ($component == 'com_menus')
{
$link = $form->getData()->get('link');
$uri = new JUri($link);
$component = $uri->getVar('option', 'com_menus');
}
$params = JComponentHelper::getParams($component);
$value = $params->get($prefix . $this->fieldname);
// Try with global configuration
if (is_null($value))
{
$value = JFactory::getConfig()->get($prefix . $this->fieldname);
}
// Try with menu configuration
if (is_null($value) && JFactory::getApplication()->input->getCmd('option') == 'com_menus')
{
$value = JComponentHelper::getParams('com_menus')->get($prefix . $this->fieldname);
}
this is a good deal with devil?
@richard67 excuse me, but I fix all errors... on new code, but this still block me.
--
58 | --------------------------------------------------------------------------------
59 | FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
60 | --------------------------------------------------------------------------------
61 | 372 \| ERROR \| Opening parenthesis of a multi-line function call must be the
62 | \| \| last content on the line
on subforms file, this is the code, array is the last content but error doesn disapear:
$tmpl = Form::getInstance($formname, $this->formsource, array(
'control' => $control,
'parent' => $parent,
'parent-group' => $parentGroup . $this->group
)
);
@Fedik, @Bakual after a lot of code style fix the code is style compilance, can you check if it can be useful?
and another little improvement should be use useglobal="XXX" parameter with component name, and adding 2 other parameters like globaladdprefix="YYY" and globalremoveprefix="ZZZ" to use common parameters defaults for more subsets.
That change prefix of element (or on subforms prefix stored).
another little improvement should be use useglobal="XXX" parameter with component name, and adding 2 other parameters like globaladdprefix="YYY" and globalremoveprefix="ZZZ"
That actually a good idea, with this we even no need to know a parent.
If field value jform[params][foo][bar][beer] then field can be:
<field ... name="beer" useglobal="com_example" useglobal-value-key="foo.bar.beer" />
This allows for developer to read any default value, from any component.
If you can change it to work like this, and without parent hack, that will be good
About globalremoveprefix I not very understood, why it can be need.
another little improvement should be use useglobal="XXX" parameter with component name, and adding 2 other parameters like globaladdprefix="YYY" and globalremoveprefix="ZZZ"
That actually a good idea, with this we even no need to know a parent.
If field valuejform[params][foo][bar][beer]then field can be:<field ... name="beer" useglobal="com_example" useglobal-value-key="foo.bar.beer" />This allows for developer to read any default value, from any component.
If you can change it to work like this, and without parent hack, that will be good
? About
globalremoveprefixI not very understood, why it can be need.
you have a subform named content, in a tab load menu page and in another set settings for children pages of menu item.
in this particoular case you can load subform from xml naming contents and children_contents.
for main content
<field type="subform" name="content" useglobal="true" fromsource="CONTENT.xml" />
and for children
<field type="subform" name="children_content" useglobal="true" globalprefixremove="children_" fromsource="CONTENT.xml" />
This solution can create a lot of reusable stuff.
In another example in content of core, on blog view, leading, intro settings can be move to main settings removing leading_ or intro_ prefix.
Global settings are the same, when you render the page you check if you are on menu item, or on a children, and load menuitem parameters that you set.
If there are a way to remove prefix 'children_' on right place of prefix (or field name) you can load the right global parameter.
same for globaladdprefix (less useful)
@Fedik, @Bakual this morning I try to undestand how to implement the "global" with prefix management.
In my opinion to do need to add another parameter on form form options:
$this->options['global-prefix'] = isset($options['global-prefix']) ? $options['global-prefix'] : false;
and related function
public function getFormGlobalPrefix()
{
return $this->options['global-prefix'];
}
Than on each formfield add management of global on setup function
if ($element['globalkey'])
{
$globalname = (string) $element['globalkey'];
}
else
{
$globalname = $this->fieldname;
}
if ($element['globalremoveprefix'])
{
$globalRemovePrefix = (string) $element['globalremoveprefix'];
if (substr($globalname, 0, strlen($globalRemovePrefix)) === $globalRemovePrefix)
{
$globalname = substr($globalname, strlen($globalRemovePrefix));
}
}
if ($this->form->getFormGlobalPrefix())
{
$this->globalName = $this->form->getFormGlobalPrefix() . '.' . $globalname;
}
else
{
$this->globalName = $globalname;
}
Then on global display (this is on list.php):
$formName = $this->form->getName();
if (substr($formName, 0, 8) === 'subform.')
{
$form = $this->form->getFormParent();
}
else
{
$form = $this->form;
}
if (in_array(strtolower($component), array('', 'true', 'yes')))
{
$component = JFactory::getApplication()->input->getCmd('option');
}
// Get correct component for menu items
if ($component == 'com_menus')
{
$link = $form->getData()->get('link');
$uri = new JUri($link);
$component = $uri->getVar('option', 'com_menus');
}
$params = JComponentHelper::getParams($component);
$value = $params->get($this->globalName);
// Try with global configuration
if (is_null($value))
{
$value = JFactory::getConfig()->get($this->globalName);
}
// Try with menu configuration
if (is_null($value) && JFactory::getApplication()->input->getCmd('option') == 'com_menus')
{
$value = JComponentHelper::getParams('com_menus')->get($this->globalName);
}
If do you think that this is a good deal to the devil, i commit the changes.
| Labels |
Added:
?
Removed: ? |
||
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-08-21 13:01:10 |
| Closed_By | ⇒ | sfs-it |
@sfs-it Regardless if this PR is right or not, it fails with our PHP code style standard documented here: https://developer.joomla.org/coding-standards/php-code.html.
Att the bottom of ypur PR in the section for the tests is one row "continuous-integration/drone/pr". If that fails, the link "Details" at the right side of this row leads to a page with test logs of diverese tests performed by Drone (one of the tools we use for automated testing). In a list at the left hand side you then can select "PHPCS" to see the log of the PHP Code Style tests. For your convenience here the direct link: https://ci.joomla.org/joomla/joomla-cms/34032/1/2.
There you can see following code style errors reported:
This kind of error is related to "if" statements not using curly brackets because containing only one line inside the "if" or the "else" block. But as you can see in our code style docs, we require curly brackets even in those cases, and the curly brackets have to use an own row.
Do you think you can fix this yourself now as you have this information?
Or shall we (some other contributor) make code change proposals to your PR so can apply them just with clicking buttons below the suggested changes?
If the latter, let us know here and someone will do it.
Thanks so far for your contribution.