Corrected "Required" attribute display

[tungdt-90]

the $requiredFields will now return a correct HTML (example <input> tag) with attribute required if this field is required.

[dhensby]

Why does it need the keys? what's the format of that array currently?

Would you mind writing a test for this?

[tungdt-90]

@dhensby This problem came up to me 2 months ago. I use bootstrap form in my project, but the field which i marked as requireddid not render the requiredattribute in the HTML tag. Then, i made a debug, print out the array, and i found out that: The current array is : key => value, but the required attribute lies in the key of array instead of the value.

I'm sorry if my explaination is too complicate.

[dhensby]

Ah, so it's something like:

array(
    'required' => true,
    ...
);

[tungdt-90]

@dhensby Yes, like that.

[dhensby]

OK - I'm cautious about this change. it makes sense from a brief overview but I'm not sure what the deeper implications of this are.

@tractorcow ?

@tungdt-90 can you show me any other modules/core code that does this?

[dhensby]

ah, examining the file as a whole makes this fairly obvious ;)

If you see https://github.com/tungdt-90/silverstripe-newsletter/blob/master/code/pagetypes/SubscriptionPage.php#L323-L324

The implication is the value stored against the key has influence on whether it's actually required or not, so you could hit some false positives with this change. I think you need to do an array_filter on the array first...

 $required = new RequiredFields(array_keys(array_filter($requiredFields)));

[tungdt-90]

Before that, in my project i had to extend the file, then applied my fix, and it worked just fine.

So now i will do a test again. Maybe it works fine my my case, but maybe it will not in some other cases.

[dhensby]

@tungdt-90 exactly - you're likely testing where all the keys => true but there is the chance a value stored against a key is false and therefore it should not be required.

I liked to the lines in the file where you can see the array is a map of field to boolean.

If you put my suggested fix in and confirm it's working, then this will get merged - otherwise it has a flaw and won't be merged

[tungdt-90]

@dhensby Yes, although i seem to have delay the test a little bit, but i will soon give you a confirm, or a better fix if i found something. Thank you.

[dhensby]

@tungdt-90 any update on this?

[tungdt-90]

@dhensby Hi there

Sorry for my late reply, it's been a crazy time for me, and i forgot to stop by to confirm this.

In fact, with the code like this:

if (!empty($requiredFields)) {
       $required = new RequiredFields(array_keys(array_filter($requiredFields)));
} else {
      $required = null;
}

It already checks on whether $required is true or not, by defining if (!empty($requiredFields))

As you said before to me:

you're likely testing where all the keys => true but there is the chance a value stored against a key is false and therefore it should not be required.

So in here, if i don't check the Required check box when i create the form, then the $requiredFields will be false, and $required will be set to null.

[dhensby]

I don't think you're understanding me.

If the value associated with a key in the array is false, then the field should not be required. At the moment any key is required regardless of the value. That has to be resolved.

[tungdt-90]

I was wrong in saying about the array. There is nothing like key => true or key => false here.

I will try to make it simple. For example, we got 2 Required Fields : Email and FirstName.

So, the $requiredFields will be an array like this:

array {
  "Email" => 1,
  "FirstName"=> 1
}

but not something like:

array(
  'required' => true,
    ...
);

// I was wrong in here, there is no such thing like this. 
// The KEY in array is the NAME of the Fields, it's not some true false boolean.

And then, as my above comment, the required field will be render like this:

$required = new RequiredFields(array_keys($requiredFields));

// Or more precisely

$required = new RequiredFields("Email", "FirstName");

// Then, the next line in code will create the form, with $required is the RequiredFields 

$form = new Form($this, "Form", $formFields, $actions, $required);

[dhensby]

1 == true

If you want this PR merged then you'll have to follow my recommendation, I'm afraid.

The code is quite clear here: https://github.com/tungdt-90/silverstripe-newsletter/blob/38285daccbd2ea8a715c1fed46c0f58d4fd25311/code/pagetypes/SubscriptionPage.php#L323-L324

It checks if the value is truthey or not. You're not doing this same check and instead you're just taking all the keys regardless of the value, this is not the right behaviour.

Please amend your PR as per my recommendation

[tractorcow]

Sorry to be late to this party.

I can confirm that @dhensby 's suggested alteration is the correct one.

If you have a

array(
    'FieldName' => false
);

Then it should NOT appear in the required fields. What @dhensby is saying is that your code doesn't do this check, but adding the array_filter will do it for us.

@tungdt-90 please amend the code as @dhensby suggested and we can look at merging.

If you need help, this is what I would put if I were doing this PR. :)

$requiredFieldNames = array_keys(array_filter($requiredFields));
if ($requiredFieldNames) {
    $required = new RequiredFields($requiredFieldNames);
} else {
    $required = null;
}

Best of luck, and just a reminder that I'm not supporting this module so I'm out of here unless @dhensby invokes me again :D

[tungdt-90]

Hi, Sorry for my late reply.

Yeah, i saw where i were wrong, and i did miss FieldName => false. But I have to delay this for a bit, because i'm having some other tasks to finish in the next two weeks. Then I will back to this.

This is a good chance for me to practice, since i'm still a junior dev. Thanks for your support.

[tractorcow]

That's ok @tungdt-90 , your effort and enthusiasm is appreciated too. ;D

[wilr]

Dup of #83