Skip to content

Fix for subform field: fix default value for "max" and "groupByFieldset"#11158

Merged
rdeutz merged 1 commit intojoomla:stagingfrom
Fedik:field-subform-groupByFieldset
Aug 19, 2016
Merged

Fix for subform field: fix default value for "max" and "groupByFieldset"#11158
rdeutz merged 1 commit intojoomla:stagingfrom
Fedik:field-subform-groupByFieldset

Conversation

@Fedik
Copy link
Copy Markdown
Member

@Fedik Fedik commented Jul 16, 2016

Summary of Changes

This fix a default values for "max" and "groupByFieldset" properties, for subform field.

Testing Instructions

Use "Custom HTML" module and add subform field into the module xml:

<field name="field-name" type="subform" formsource="modules/mod_custom/test1.xml" 
   multiple="true"  label="Subform Field" description="Subform Field Description" />

and create simple form xml in modules/mod_custom/test1.xml:

<?xml version="1.0" encoding="UTF-8"?>
<form>
    <field name="test_text" type="text"
        label="test_text" description="test_text" />

    <field name="test_textarea" type="textarea"
        label="textarea"   />
</form>

expected result
You should see the subform inputs, with possibility to add new row.

actuall result
the subform is empty, and you can add only one row

@Fedik Fedik changed the title Subform field, fix default value for "max" and "groupByFieldset" Fix for subform field: fix default value for "max" and "groupByFieldset" Jul 17, 2016
@toniedw
Copy link
Copy Markdown

toniedw commented Jul 17, 2016

I have tested this item ✅ successfully on c599253


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11158.
Thanks for creating this field, already using it in an extension.

@brianteeman
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on c599253

Thanks


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11158.

@brianteeman
Copy link
Copy Markdown
Contributor

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11158.

@brianteeman brianteeman added RTC This Pull Request is Ready To Commit PR-staging labels Aug 3, 2016
@brianteeman brianteeman added this to the Joomla 3.6.2 milestone Aug 3, 2016

case 'max':
$this->max = max(1, (int) $value);
if ($value)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this check, $value is set and if it is nothing that will be cast to false it will be used for the max(1, whatever) so that doesn't allow to set max = 0 but that was the same before with the difference that max isn't now set at all. I also don't understand why this only for max and not for min.

Copy link
Copy Markdown
Member Author

@Fedik Fedik Aug 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the problem here when attribute max="" is not defined (in field xml), and so it always null here,
so in result we got $this->max = 1;

.. if it is nothing that will be cast to false ..

will be cast to zero, and max(1, 0) will return 1 😉

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can max have negativ values? If not the the max(1, $value) doesn't makes sense, because value is always 1 or greater.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in case someone do max="-100"

@SiteOp
Copy link
Copy Markdown

SiteOp commented Aug 19, 2016

Successfully tested. Perfect.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11158.

@rdeutz rdeutz merged commit 2cb53bc into joomla:staging Aug 19, 2016
@Fedik Fedik deleted the field-subform-groupByFieldset branch August 19, 2016 18:40
ggppdk pushed a commit to ggppdk/joomla-cms that referenced this pull request Aug 19, 2016
@brianteeman brianteeman removed the RTC This Pull Request is Ready To Commit label Aug 20, 2016
roland-d pushed a commit to roland-d/joomla-cms that referenced this pull request Sep 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants