Fix regression that not allow to use a custom filter with subform field by Fedik · Pull Request #27561 · joomla/joomla-cms

Fedik · 2020-01-18T11:57:24Z

Summary of Changes

Fix regression that not allow to use a custom filter with subform field, was introduced by one of previous update.

Testing Instructions

Apply patch, create a subform field , somwhere in CustomHTML module, with custom filter:

<field name="test_fields" type="subform" label="test_fields" 
filter="FooBar::filterTest" multiple="true" min="1">
  <form>
    <field name="test_field" type="text" label="TESTFIELD1"/>
  </form>
</field>

Add custom filter class, somwhere in bottom of plugins/system/debug/debug.php:

class FooBar
{
	static function filterTest($value)
	{
		JFactory::getApplication()
		 ->enqueueMessage('Filter works: ' . json_encode($value));
		return $value;
	}
}

Then create a Custom module in backed, and try save it.
Look for the message you got

Expected result

You should get 2 message:
Filter works: {"test_fields0":{"test_field":""}}
and
Module saved

Actual result

You get 1 message
Module saved
that means the custom filter was ignored

viocassel · 2020-01-19T09:47:57Z

I have tested this item ✅ successfully on da9a1bb

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27561.}

jwaisner · 2020-01-21T01:02:22Z

I have tested this item ✅ successfully on da9a1bb

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27561.}

Quy · 2020-01-21T03:53:10Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27561.}

wilsonge · 2020-01-21T11:47:13Z

@joomla/security please review this as this addressed a specific vulnerability in the past

zero-24 · 2020-01-21T18:43:15Z

libraries/src/Form/Form.php

 			$subForm = $field->loadSubForm();

-			if ($field->multiple)
+			if ($field->multiple && $value)


What is the intention of this change? We might should check whether this is an array right?

It can be an array or an object.

What is the intention of this change

if there a no value you will get an php notice while foreach(null as $key => $val)

OK then let's do is_array here and can we add a comment just explaining it covers an optional subform field

Would probably need is_array() and is_object() in that case. This check also needs to be nested to avoid issues. See #27671.

it does not make much sense, because if you see whole condition, it still will be validated

joomla-cms/libraries/src/Form/Form.php

Lines 2158 to 2176 in da9a1bb

if ($field->multiple && $value)

{

foreach ($value as $key => $val)

{

$val = (array) $val;

$valid = $subForm->validate($val);

if ($valid === false)

{

break;

}

}

}

else

{

$valid = $subForm->validate($value);

}

filter already have the same conditions

joomla-cms/libraries/src/Form/Form.php

Lines 1567 to 1579 in da9a1bb

if ($field->multiple && !empty($value))

{

$return = array();

foreach ($value as $key => $val)

{

$return[$key] = $subForm->filter($val);

}

}

else

{

$return = $subForm->filter($value);

}

The entire concept of validating an empty value seems weird to me :)

yeah, but that what we have ;)

It won't be validated if you follow #27671. Apply same changes to filter.

libraries/src/Form/Form.php

zero-24 · 2020-01-21T18:47:15Z

@joel-lupfer @SniperSister Any additional input here?

richard67 · 2020-02-16T11:22:29Z

@joel-lupfer @SniperSister Any additional input here?

SniperSister · 2020-02-17T09:50:31Z

@richard67 fine for me!

Fedik added 2 commits January 18, 2020 13:45

Do not ignore subform filter

7527778

Do not foreach empty value

da9a1bb

joomla-cms-bot added the PR-staging label Jan 18, 2020

Fedik mentioned this pull request Jan 18, 2020

[4.0] Allow to use a custom filter with subform field #27562

Closed

Quy removed the PR-staging label Jan 21, 2020

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 21, 2020

zero-24 reviewed Jan 21, 2020

View reviewed changes

libraries/src/Form/Form.php Show resolved Hide resolved

HLeithner added this to the Joomla! 3.9.16 milestone Jan 22, 2020

Quy added the PR-staging label Jan 23, 2020

Merge branch 'staging' into subform-f-v

df1093a

wilsonge approved these changes Feb 16, 2020

View reviewed changes

joomla deleted a comment Feb 20, 2020

rdeutz merged commit 914a371 into joomla:staging Feb 20, 2020

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 20, 2020

Fedik deleted the subform-f-v branch February 21, 2020 08:40

wilsonge mentioned this pull request Mar 14, 2020

3.9.16 merge #28335

Merged

	if ($field->multiple && $value)
	{
	foreach ($value as $key => $val)
	{
	$val = (array) $val;

	$valid = $subForm->validate($val);

	if ($valid === false)
	{
	break;
	}
	}
	}
	else
	{
	$valid = $subForm->validate($value);
	}

	if ($field->multiple && !empty($value))
	{
	$return = array();

	foreach ($value as $key => $val)
	{
	$return[$key] = $subForm->filter($val);
	}
	}
	else
	{
	$return = $subForm->filter($value);
	}

Uh oh!

Conversation

Fedik commented Jan 18, 2020

Summary of Changes

Testing Instructions

Expected result

Actual result

Uh oh!

viocassel commented Jan 19, 2020

Uh oh!

jwaisner commented Jan 21, 2020

Uh oh!

Quy commented Jan 21, 2020

Uh oh!

wilsonge commented Jan 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fedik Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zero-24 commented Jan 21, 2020

Uh oh!

richard67 commented Feb 16, 2020

Uh oh!

SniperSister commented Feb 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Fedik Jan 21, 2020 •

edited

Loading