MaterialIcons::getFormIconField assumes depth of icon field

Created on 27 June 2024, 5 months ago

The getFormIconField function in Drupal\material_icons\Plugin\Field\FieldWidget\MaterialIcons assumes the nested depth of the icon field to be 4 in the line.
return (!is_null($parents)) ? $form[$parents[3]][$parents[2]][$parents[1]]['icon'] : NULL;

This is not always the case an can result in the error
ResponseText: TypeError: Drupal\Core\Render\MainContent\AjaxRenderer::renderResponse(): Argument #1 ($main_content) must be of type array, null given, called in /app/web/core/lib/Drupal/Core/Form/FormAjaxResponseBuilder.php on line 89 in Drupal\Core\Render\MainContent\AjaxRenderer->renderResponse() (line 49 of /app/web/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php).
at paths like
/layout_builder/update/block/overrides/node.9316/0/blb_region_col_1/3c9a37a5-f66c-45ba-80f9-a804ea7108af?destination=/node/9316/layout&_wrapper_format=drupal_ajax&ajax_form=1

The $parents array where I saw this issue looks like this:

πŸ› Bug report
Status

Active

Version

2.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States damondt

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @damondt
  • πŸ‡ΊπŸ‡ΈUnited States damondt
  • πŸ‡ΊπŸ‡ΈUnited States damondt
  • πŸ‡¦πŸ‡ΊAustralia darvanen Sydney, Australia

    Can confirm this is happening to me too, trying to track down the cause, don't hold your breath.

  • Status changed to Needs review 5 months ago
  • πŸ‡¦πŸ‡ΊAustralia darvanen Sydney, Australia

    Well I haven't found out *why* the array keys are not in the positions this module expects them to be on a particular request, but I have found a way to prevent the error, which doesn't affect the widget's behaviour as far as I can tell.

    It seems with the !is_null( there are times when the $parents variable is expected to be unavailable, so I think it's reasonable to assume that maybe in some situations it will be available but not in exactly the expected format. So I've made an MR to use a null coalesce operator instead of only checking the main variable.

    The same may well be applied at line 195 but I wanted to keep the initial MR to the scope defined within the issue.

  • Status changed to Needs work 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States damondt

    In the case I'm looking at the issue is that the icon field is nested within a paragraph field. There may be other issues with the logic in this function that the provided patch would work for, but it does not solve for every case.

  • πŸ‡ΊπŸ‡ΈUnited States damondt
  • πŸ‡¦πŸ‡ΊAustralia darvanen Sydney, Australia

    You're absolutely right, for one thing we need to retain the null check on the parent variable. I've added a commit to that effect.

    This does work for me for a field nested within paragraphs, are you saying it didn't work for you?

  • πŸ‡ΊπŸ‡ΈUnited States damondt

    It doesn't solve the issue I'm seeing, the paragraph is on a block content entity where I'm investigating which could be related

  • πŸ‡ΊπŸ‡ΈUnited States damondt

    Pushed progress on fixing this, problems identified:
    - Assumes the nested depth of form field in form, this is not true if in paragraphs.
    - Tries to return a field value as an ajax response, must be render array for field or ajax command.

    I haven't fully solved the second issue yet, so leaving as "needs work" for now. It no longer errors on style value change, but the icon value is yet not updated successfully.

  • Status changed to Needs review 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States damondt

    Fixed ajax issue, marking as needs review.

  • πŸ‡¦πŸ‡ΊAustralia darvanen Sydney, Australia

    I tested out a patch from the latest diff of the MR, in my situation there was still an error:

    An AJAX HTTP error occurred.
    HTTP Result Code: 200
    Debugging information follows.
    Path: /node/8321/edit?destination=/admin/content&ajax_form=1&_wrapper_format=drupal_ajax
    StatusText: parsererror
    ResponseText: 
    Warning:  Trying to access array offset on value of type null in /var/www/html/docroot/modules/contrib/material_icons/src/Plugin/Field/FieldWidget/MaterialIcons.php on line 211

    A difference might be that https://www.drupal.org/project/paragraphs_browser β†’ is in use on this site.

  • πŸ‡ΊπŸ‡ΈUnited States damondt

    Sorry about that, just pushed a fix.

  • πŸ‡¦πŸ‡ΊAustralia darvanen Sydney, Australia

    All good man, no apologies please - we're all in this together :)

    I've tried out the latest code and at least on that project, it passes manual testing. I don't think that's enough to call it reviewed, but it's a start.

  • πŸ‡¦πŸ‡ΊAustralia VladimirAus Brisbane, Australia

    vladimiraus β†’ changed the visibility of the branch 2.0.x to hidden.

  • πŸ‡¦πŸ‡ΊAustralia VladimirAus Brisbane, Australia

    exit stops the program. To exit the loop, use break.

  • First commit to issue fork.
  • πŸ‡¦πŸ‡ΊAustralia VladimirAus Brisbane, Australia

    vladimiraus β†’ changed the visibility of the branch 3457701-getfontfamily-ajax-error to hidden.

  • πŸ‡¦πŸ‡ΊAustralia VladimirAus Brisbane, Australia

    vladimiraus β†’ changed the visibility of the branch 3457701-getfontfamily-ajax-error to active.

  • πŸ‡¦πŸ‡ΊAustralia prasanth_n

    Fixed PHPCS violations and replaced exit with break.

  • πŸ‡¦πŸ‡ΊAustralia VladimirAus Brisbane, Australia

    Looks good. πŸ‘

Production build 0.71.5 2024