FormHelper _groupTemplate

[nifa-dev]

Changes in CakePHP https://github.com/cakephp/cakephp/pull/10100 created an instance where the input index of $options might not exist.

Description

Created a shorthand statement to reference the input index of the $options array if it is set, if not use an empty array.

Summarize

Only one change, outlined above.

Benefits

Fixed notice error that was present after updating to CakePHP 3.3.13

Related Issues

fixes #179.

[ADmad]

The fix looks good, could you please also add a test case to avoid regression in future.

[nifa-dev]

Good point. And since my inexperience has caught up to me, I'm going to have to ask for help with that.

The way I see it is that this test case (already included) could actually do the trick:

public function testBasicCheckboxInput()
{
    $this->Form->create($this->article);

    $result = $this->Form->input('published');
    $expected = [
        'div' => ['class' => 'checkbox'],
        'input' => [
            'type' => 'hidden',
            'name' => 'published',
            'value' => 0,
        ],
        'label' => ['for' => 'published'],
        ['input' => [
            'type' => 'checkbox',
            'name' => 'published',
            'id' => 'published',
            'value' => 1
        ]],
        'Published',
        '/label',
        '/div'
    ];
    $this->assertHtml($expected, $result);
}

Am I wrong in assuming that PHPUnit should automatically convert the Notice Error to an exception? If that in fact occurred the assertHtml method would fail resulting in the test to fail. Most of the items I've read in researching this indicate that functionality is supposedly default, but this test does not seem to do that. What am I missing?

Adjusting the testCase method to include a custom error handler that throws an exception essentially does the trick, but this doesn't seem right to me.

public function testBasicCheckboxInput()
{
    set_error_handler(function($errno, $errstr, $errfile, $errline) {
        throw new \RuntimeException($errstr . " on  line " . $errline . " in file " . $errfile);
    });

    $this->Form->create($this->article);

    $result = $this->Form->input('published');
    $expected = [
        'div' => ['class' => 'checkbox'],
        'input' => [
            'type' => 'hidden',
            'name' => 'published',
            'value' => 0,
        ],
        'label' => ['for' => 'published'],
        ['input' => [
            'type' => 'checkbox',
            'name' => 'published',
            'id' => 'published',
            'value' => 1
        ]],
        'Published',
        '/label',
        '/div'
    ];
    $this->assertHtml($expected, $result);
}

If anyone can educate me as to where my ways are in error, I'd appreciate it.

[ADmad]

Yeah if the code generates notice then the test should be already failing.

[nifa-dev]

Thanks. That's what I was thinking, but it doesn't seem to be happening. The first test case included above passes, even before the changes I introduced in this PR are inserted. Well, it passes but also shows the notice error. I'm mixed up as to why that's passing and not failing.

[ADmad]

I'll look into it and see if i can figure out why existing test doesn't fail.

[thinkingmedia]

This is the patch that I made. My thinking is that this future proofs it better, but it could also introduce unexpected bugs.

    protected function _groupTemplate($options)
    {
        $groupTemplate = $options['options']['type'] . 'FormGroup';
        if (!$this->templater()->get($groupTemplate)) {
            $groupTemplate = 'formGroup';
        }

        return $this->templater()->format($groupTemplate, $options + $options['options']);
    }

Anyway, that patch got me running again.

[ADmad]

@nifa-dev I tested locally and current tests are failing and this patch fixes it.

@thinkingmedia Simply merging options could potentially cause problems if extra options sneak in, so I prefer this fix instead.

[thinkingmedia]

@ADmad yeah, my patch is bad. I don't know why I recommended it. lol