Problem/Motivation

Having both core Navigation module and contrib Modifiers module enabled causes an error.

The change in #3443810: Custom Navigation logo is disconnected from new Layout template introduced a 'settings' array without a preceding '#'. When the Modifiers module attempts to process this array in a template_preprocess_layout() function it causes an error in Element::children() as settings is not a valid render array.

Steps to reproduce

  1. Enable Modifiers module
  2. Enable Navigation module
  3. Observe error: InvalidArgumentException: "hide_logo" is an invalid render array key. Value should be an array but got a boolean. in Drupal\Core\Render\Element::children() (line 102 of core/lib/Drupal/Core/Render/Element.php).

Proposed resolution

I think the output of doBuildNavigation() should be a valid render array so properties such as 'settings' should have a '#' to indicate they are not intended to be rendered directly.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 default_logo.png627.14 KBm4olivei
#9 hide_logo.png621.09 KBm4olivei
#9 custom_logo.png983.23 KBm4olivei

Issue fork drupal-3516558

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

cb_govcms created an issue. See original summary.

tarawij’s picture

tarawij commented

I can reproduce this on Drupal core 10.4.5, 11.0.13, and 11.1.5 with Modifiers 8.x-1.6.

plopesc made their first commit to this issue’s fork.

plopesc opened merge request !11696

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Active » Needs review

The suggestion looks valid to me.

Changed settings to #settings and I didn't detect any regression afterwards.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems straight forward, been told since navigation is experimental then a CR isn't needed. Also believe this qualifies for not needing test coverage

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Reviewed & tested by the community » Needs work

As #settings is defined by Layout Builder for layout templates, we can actually simplify its use in the template. See MR for details.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Thank you for your review @m4olivei. Great catch!

Addressed your comments in the last commit.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new custom_logo.png983.23 KB
new hide_logo.png621.09 KB
new default_logo.png627.14 KB

Looks great! Thanks for this @plopesc.

I'm seeing everything look good including adjusting the logo settings, eg.

Hide Logo:

Screenshot with logo hidden

Default logo:

Screenshot with default logo

Custom logo

Screenshot with custom logo

  • pdureau committed 05057f1f on 11.x 
    Issue #3516558 by plopesc, m4olivei, cb_govcms, tarawij: Settings is not...
pdureau’s picture

pdureau commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 05057f1 and pushed to 11.x. Thanks!

Need to be ported to 11.1.x, 10.5.x & 10.4.x (careful, 10.5.x has a merge conflict)

pdureau closed merge request !11696

  • nod_ committed 8a5dd952 on 11.1.x authored by pdureau 
    Issue #3516558 by plopesc, m4olivei, cb_govcms, tarawij: Settings is not...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.5.x-dev
Status: Patch (to be ported) » Needs work

doesn't cherry pick to 10.5.x, need a MR for that one

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Patch (to be ported)

smustgrave opened merge request !12181

smustgrave’s picture

smustgrave commented
Status: Patch (to be ported) » Reviewed & tested by the community

Seems to be a clean backport

  • nod_ committed 7d1886da on 10.5.x 
    Issue #3516558 by plopesc, smustgrave, m4olivei, nod_, cb_govcms,...

  • nod_ committed 34f55547 on 10.6.x 
    Issue #3516558 by plopesc, smustgrave, m4olivei, nod_, cb_govcms,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 34f55547a4e to 10.6.x and 7d1886dab79 to 10.5.x. Thanks!

nod_ closed merge request !12181

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.