Conversation
|
Although the original reporter confirmed that the patch applies cleanly and fixes the issue in their local environment, I have not been able to reliably reproduce the TypeError myself. Because this depends on specific configuration/upgrade paths (where |
jjroelofs
left a comment
There was a problem hiding this comment.
Thanks for the fix @drale1 — the is_array() guards for hidden_regions and block_design_regions look correct and are clearly needed for PHP 8+ compatibility. Nice catch.
However, there are a couple of issues with this PR:
1. Unrelated menu active trail changes not mentioned in PR description
The diff also removes the $is_front parameter from dxpr_theme_menu_set_active_trail() and drops the front page check ($is_front && $item_path === '/') — but this isn't mentioned anywhere in the PR description, commit messages, or linked issue #781.
Per CONTRIBUTING.md: "do not include changes to files not in this list unless absolutely necessary. When you do this you must explain why."
Can you clarify why these menu changes are included? Are they intentional or did they end up here by accident?
2. These menu changes already exist on 8.x
Looking at the current 8.x branch, the $is_front parameter was already removed in earlier merged PRs. Your branch appears to be based on an older 8.x commit, so these diffs are stale — they'll either conflict on merge or be no-ops.
3. "Fix space" commit should be squashed
The second commit (98f2a89 — "Fix space") should be squashed into the first to keep the history clean and atomic.
Requested action
Please rebase onto current 8.x — that should drop the menu-related diffs automatically and leave only the two is_array() guard changes, which are good to go. Squash the "Fix space" commit while you're at it.
98f2a89 to
8ee8a95
Compare
|
@jjroelofs I investigated this and honestly can’t tell where the I’ve removed those unrelated changes and rebuilt the PR as a clean, single-commit fix. The current drupal-lint/stylelint failures don’t appear to be related to my changes. Would you like me to address them in this PR, or should we keep this PR focused and handle the lint issues separately? |
Linked issues
Fix is related to the Drupal.org issue https://www.drupal.org/project/dxpr_theme/issues/3570233
Solution
This PR adds simple
is_array()guards aroundtheme_get_setting('hidden_regions')andtheme_get_setting('block_design_regions')before passing their values intoarray_filter()indxpr_theme.theme.On PHP 8+,
theme_get_setting()can returnnullfor these settings on some configurations. Passingnulldirectly intoarray_filter()causes aTypeError. With the guards in place, the theme safely skips applying the related logic when the settings are not defined, and pages render without PHP fatals.Checklist