Avoid empty properties like id='' which is invalid

[AIC-BV]

Credits to @mjauvin

[what-the-diff]

• The function array_filter() is used to filter out the null values in an array.
• In this case, it filters out all of the empty items from $options and merges them with $merge into a new variable called options.

[AIC-BV]

Errors? 😬
Accidentaly placed it as draft, don't know if it changes anything? It looked like Ben asked it at the top right... 😅

[mjauvin]

Errors? grimacing Accidentaly placed it as draft, don't know if it changes anything? It looked like Ben asked it at the top right... sweat_smile

You can ignore this error, it's not related to the PR itself.

[mjauvin]

@AIC-BV can you add example of the generated markup before/after this PR is applied?

[AIC-BV]

@mjauvin
BEFORE: <input name="_session_key" type="hidden" value="EwO55HJQFdZVG1ZYRwY7XSCCWgoZdehoLeo17YcD" id="">
AFTER: <input name="_session_key" type="hidden" value="EwO55HJQFdZVG1ZYRwY7XSCCWgoZdehoLeo17YcD">

Where empty ID is invalid and throwing w3c errors.
ID with empty string is also marked as duplicate by w3c if you have multiple empty IDs in one page

[LukeTowers]

@AIC-BV this could cause issues with attribute values that are considered "empty" but would actually be valid, i.e. 'false' or 0. Could you add some unit test coverage for this?

[AIC-BV]

@LukeTowers
I don't know how to do that

[LukeTowers]

@AIC-BV excellent learning opportunity then :)

You'll want to clone this repository locally (and run composer install) and then add a file to tests/Html/FormBuilderTest.php. If you review https://github.com/wintercms/storm/blob/develop/tests/Html/HtmlBuilderTest.php you should get a general sense of how the tests are structured, but basically every method is run as a separate test, so this file you'd create the class and then a testInput() method to test the FormBuilder->input() method.

In that method, you'd then think of every input / output combination that you can think of that would address the different code branching paths available in the current implementation of the input() method and then use the PHPStorm assert methods to ensure that for a given set of inputs the desired output is generated.

Once you've written the test you can run it by running ./vendor/bin/phpunit --filter=FormBuilderTest in the repo's directory.

If you have access to it, github copilot or chatgpt can be pretty helpful with generating tests for you, but give it a shot yourself first.

Unit testing can be a bit intimidating to get into, but once you've done a couple you'll see that it's not really that complicated and it can be incredibly helpful to your development process.

[mjauvin]

@LukeTowers we changed to !isset() should be safe.

[LukeTowers]

@mjauvin but that doesn't actually solve the original issue does it?

[mjauvin]

Why not?

[LukeTowers]

Because the value would still be null, not void wouldn't it?

[mjauvin]

It happens when the id attribute is not provided, so isset is appropriate (and was tested as a working solution)

[LukeTowers]

If we can get a test case that demonstrates that I'm fine with merging :) Just don't have the time to reproduce locally at the moment.

[mjauvin]

Replaced by #143