Lack of validation of empty array types generates incorrect markup?

[dabd]

This is an issue I have found in a few components that have array parameters.

This example illustrates it.

{% if params.navigation %}
    <button type="button" class="govuk-header__menu-button govuk-js-header-toggle" aria-controls="navigation" aria-label="Show or hide Top Level Navigation">Menu</button>
    <nav>
...

When the array is empty params.navigation is still a truthy value so it will create the button but no navigation. If this is not the intended outcome, should a guard be added to check for an empty array?

[NickColley]

Hello @dabd!

I believe we could do something like:

{% if params.navigation && (params.navigation | length) %}

in Nunjucks to check that there an array is not empty but I would need to double check this.

Is this something that you've run into in development or just an observation based on looking at the code, this will help us prioritise this.

[dabd]

Hi @NickColley, thanks for the quick reply!

This is something that I've found out by running tests on an implementation of the library's components we're working on. Our tests are checking for parity of the generated markup, and for some test cases we were getting different results because we are checking for empty arrays.

This creates a bit of a dilemma between having a faithful implementation or doing validations based on assumptions that might be incorrect. It would be nice to have these checks if that is the correct thing to do. Thanks!

[NickColley]

Thank you for the extra context! I've added the awaiting-triage label this means we'll meet as a team next Wednesday and discuss what we want to do next.

[edwardhorsford]

This seems sensible to me. We recently added support for not outputting markup where something wasn't provided - this feels like a natural extension.

[NickColley]

@dabd we talked about this is a team.

Is this blocking you from completing your implementation? Is there a way you can set up your test suite to avoid sending boolean true values do your templates, is this a Scala nuance?

If it is we can look into this, if you're interested in doing a pull request for this functionality let me know too.

[dabd]

It is not blocking us, but it would be nice to have those issues fixed so we don't have to encode them in our implementation or add validations that would make implementations diverge. I'm not a JavaScript developer but maybe people in our team could contribute with PRs.

…

On Wed, 30 Oct 2019, 14:44 Nick Colley, ***@***.***> wrote: @dabd <https://github.com/dabd> we talked about this is a team. Is this blocking you from completing your implementation? Is there a way you can set up your test suite to avoid sending boolean true values do your templates, is this a Scala nuance? If it is we can look into this, if you're interested in doing a pull request for this functionality let me know too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1618?email_source=notifications&email_token=AABI4JZS7Z5ZAIFNMKI7T43QRGMTFA5CNFSM4JD6FZVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECUOR5Y#issuecomment-547940599>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABI4JZJOMJYM2OW7OKODDDQRGMTFANCNFSM4JD6FZVA> .

[silasl]

A PR has been raised for this: #1638

[hannalaakso]

@dabd I believe that #1638 has fixed this so closing the issue but you can re-open if needed.