Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: Attribute 'checked' in Check element #15959

Closed
bakos opened this issue May 13, 2022 · 19 comments
Closed

[BUG]: Attribute 'checked' in Check element #15959

bakos opened this issue May 13, 2022 · 19 comments
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report documentation Documentation required status: low Low

Comments

@bakos
Copy link

bakos commented May 13, 2022

Describe the bug

In Phalcon 5.0.x Check field clears 'checked' attribute on render.

To Reproduce

$checkField = new \Phalcon\Forms\Element\Check('checkField');
$checkField->setAttribute('checked', 'checked');
var_dump($checkField->getAttributes());
echo $checkField;

Expected behavior

It should render html with attribute 'checked'. It works in phalcon 4.x, 3.x

Screenshots

Phalcon 4

image

Phalcon 5

image

Details

  • Phalcon version: 5.0.0beta3
  • PHP Version: 7.4.28
  • Operating System: Debian
  • Installation type: pecl
  • Server: Apache

Additional context

I tried this on attribute 'disabled' and it was working, so the bug is not about setting any attribute

@bakos bakos added bug A bug report status: unverified Unverified labels May 13, 2022
@niden
Copy link
Sponsor Member

niden commented May 23, 2022

Can you try setting the checked with the same value as value?

$checkField = new \Phalcon\Forms\Element\Check('checkField');
$checkField
    ->setAttribute('value', 'one-two-three')
    ->setAttribute('checked', 'one-two-three')
;

echo $checkField;

@bakos
Copy link
Author

bakos commented May 24, 2022

It works in that configuration

@niden
Copy link
Sponsor Member

niden commented May 24, 2022

I am closing this but will tag it as documentation so as to update the documents accordingly

@niden niden closed this as completed May 24, 2022
@niden niden added not a bug Reported issue is not a bug documentation Documentation required 5.0 The issues we want to solve in the 5.0 release and removed bug A bug report status: unverified Unverified labels May 24, 2022
@niden niden self-assigned this May 24, 2022
@niden niden added this to Implemented in Phalcon Roadmap May 24, 2022
@bakos
Copy link
Author

bakos commented Jul 21, 2022

@niden this solution seems to be workaround and not real solution. There can be checkboxes without value created just to represent actual state.

@niden niden reopened this Jul 21, 2022
@niden
Copy link
Sponsor Member

niden commented Jul 21, 2022

@niden this solution seems to be workaround and not real solution. There can be checkboxes without value created just to represent actual state.

Can you give me an example please?

@bakos
Copy link
Author

bakos commented Jul 22, 2022

@niden I have a big project with many forms where i have checkboxes. Now in every checkbox that is only used for preview and that worked on v2, v3 and v4 (yes its old project mvp was building on v1) i have to modify code like this:

$checkField = new \Phalcon\Forms\Element\Check('checkField');
$checkField->setAttribute('checked', 'checked');

into this:

$checkField->setAttribute('value', 'workaround');
$checkField->setAttribute('checked', 'workaround');

because this value has no other meaning than to make "checked" worked. I thought maybe this could work:

$checkField->setAttribute('checked',$checkField->getAttribute('value'));

but it is not. I have to add value to checkboxes just to make it work. I can and probably do that but it`s workaround not a solution and potencialy a source of problems couse its easy to forget that $checkField->setAttribute('xyz', 'xyz'); will work and set attribute on field, but $checkField->setAttribute('checked', 'checked') will not work and will not set attribute on the field becouse in 'checked' you have to add value before and set this value to checked. Sounds like a bug.
.

@punneke
Copy link

punneke commented Aug 19, 2022

Yes, setAttribute() is definately not the solution. In Phalcon 3/4 the checked attribute was true of false when the Entities property value was the same as the default value attribute.

For example:

$status = new Check('status ', [
    'value' => '1',
]);
$status ->setLabel('Status');
$this->add($status );

Worked out of the box when $entity->status = '1'

$form->render('status');
<input type="checkbox" id="status" name="status" value="1" checked="checked">

In Phalcon 5 the checked attribute is not automatically set anymore.

<input type="checkbox" id="status" name="status" value="1">

This is my current workaround:

$status = new Check('status', [
    'value' => 1,
]);
if (!is_null($entity)) {
    $status->setAttribute('checked', $entity->status);
}
$status->setLabel('Status');
$this->add($status);

@niden niden added bug A bug report status: low Low and removed not a bug Reported issue is not a bug labels Aug 19, 2022
@niden niden moved this from Implemented to Working on it in Phalcon Roadmap Aug 19, 2022
@punneke
Copy link

punneke commented Aug 19, 2022

@niden I was looking further into the issue.

I think the issue lies in the fact that the value in $entity->active is 1 as an integer and not '1' a string.
This probably changed in Phalcon 5 because all database integers used to be returned as a string and now they are integers.

So the check on line 127 in https://github.com/phalcon/cphalcon/blob/master/phalcon/Html/Helper/Input/Checkbox.zep#L127 fails because is strict with ===.

@mikx33
Copy link

mikx33 commented Oct 25, 2022

I also have a similar problem, because of which I can not upgrade to version 5

@sitchi
Copy link
Sponsor

sitchi commented Nov 14, 2022

Temporary solution sitchi@00e0015

@amujib
Copy link

amujib commented Nov 17, 2022

I found another bug related to checkbox when using '0' as unchecked value.
To Reproduce

$checkField = new \Phalcon\Forms\Element\Check('checkField');
$checkField
    ->setAttribute('value', '1')
    ->setAttribute('unchecked', '0')
;

echo $checkField;

Expected Result
<hidden name="checkField" value="0"><input type="checkbox" id="checkField" name="checkField" value="1" />

Actual Result
0<input type="checkbox" id="checkField" name="checkField" value="1" />

@SliceOfLife
Copy link

I got another vector for this issue.
The "checked" attribute is gone after the form validation fails when we re-render Form with _POST values.
This behavour works with the legacy Tag renderer, but might be relevant for the new TagFactory.

For now I use this temporary fix:

$this->tag->set('inputCheckbox', CheckboxRendererFix::class);
class CheckboxRendererFix extends \Phalcon\Html\Helper\Input\Checkbox
{
	private function processChecked(): void
	{
		$attributes = $this->attributes;

		if (empty($attributes['checked'])) {
			if (!empty($attributes['value'])) {
				$attributes['checked'] = 'checked';
			}
		}

		$this->attributes = $attributes;
	}
}

@dugwood
Copy link
Contributor

dugwood commented Jun 15, 2023

The main issue here is the test of checked value. It should use empty() for the test:

As @amujib stated, testing 0 or (empty string) should be a valid option. It was working with Phalcon 4.

So instead of empty() there should only be a isset(). And we can assume that null is not a valid value.

Can you fix this? Or should I open another bug report @niden?

@dugwood
Copy link
Contributor

dugwood commented Jun 15, 2023

@niden something like:

    /**
     * Processes the checked value
     */
    private function processChecked() -> void
    {
        var checked, value;
        array attributes;

        let attributes = this->attributes;
        if !fetch checked, attributes["checked"] {
            let checked = null;
        }

        unset attributes["checked"];

        if checked !== null {
            if !fetch value, attributes["value"] {
                let value = null;
            }
            if checked === value {
                let attributes["checked"] = "checked";
            }
        }

        let this->attributes = attributes;
    }

@niden
Copy link
Sponsor Member

niden commented Jun 15, 2023

The main issue here is the test of checked value. It should use empty() for the test:

As @amujib stated, testing 0 or (empty string) should be a valid option. It was working with Phalcon 4.

So instead of empty() there should only be a isset(). And we can assume that null is not a valid value.

Can you fix this? Or should I open another bug report @niden?

If you don't mind please add an issue and reference this discussion/tag me.

Sorry guys, too busy at work to remember all these discussions :/

@dugwood
Copy link
Contributor

dugwood commented Jun 15, 2023

@niden I've done a very simple PR: #16357

I've compiled it with Zephir and it works as Phalcon 4 (with setDefault) used to work :-)

Thanks!

@niden
Copy link
Sponsor Member

niden commented Jun 15, 2023

Resolved in #16358

Thank you again @dugwood

@niden niden closed this as completed Jun 15, 2023
@dugwood
Copy link
Contributor

dugwood commented Jun 16, 2023

@bakos regarding your initial report: checkboxes always have a value. If you POST a checkbox, you'll see that PHP considers its value to be on when checked. So it has a default value of on.

Radio fields are different as it should be considered as a <select> (I mean you have to choose one value among all the values). So there must be a value too. If empty, it should be set as empty with value="".

@niden there's another issue with this, as radio_field() in Volt doesn't work like this at all. I've kept my old Phalcon v4 code (with set_default()) and it works, but setting checked doesn't work as expected, it checks every radio field. I think it would be good to fix it one day, but that's a lot of changes for Phalcon users.

@HalitYesil
Copy link

What!

Is selected if the value property value and the checked property value are the same.

`
/**
* Processes the checked value
*/
private function processChecked() -> void
{
var checked, value;
array attributes;

    let attributes = this->attributes;
    if !fetch checked, attributes["checked"] {
        let checked = null;
    }

    unset attributes["checked"];

    if checked !== null {
        if !fetch value, attributes["value"] {
            let value = null;
        }
        if checked === value { // Is selected if the value property value and the checked property value are the same. . <<<<======
            let attributes["checked"] = "checked"; 
        }
    }

    let this->attributes = attributes;
}

`

I did it as follows and it worked.

$input = new \Phalcon\Forms\Element\Radio('radio1', [ 'name' => 'radiogroup', 'value' => 'one-two-three' ]); $input->setDefault('one-two-three'); $input->setAttribute('checked', $input->getValue()); // <<<< Set the input value property value for the checked property.

I think the team was a little confused while writing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report documentation Documentation required status: low Low
Projects
Status: Released
Phalcon Roadmap
  
Working on it
Development

No branches or pull requests

9 participants