Skip to content

fix(portalblocks): handle null/unset rows in sparse layout arrays #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ralflang merged 1 commit into from
Mar 17, 2026
Merged

fix(portalblocks): handle null/unset rows in sparse layout arrays #69

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lib/Horde/Core/Block/Layout/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ public function __construct(Horde_Core_Block_Collection $collection)
$emptyrows = [];

Copy link
Contributor

@amulet1 amulet1 Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop $emptyrows (looks like it is assigned but never used).

for ($row = 0; $row < $rows; $row++) {
// Skip null rows (sparse array)
if (!isset($this->_layout[$row]) || !is_array($this->_layout[$row])) {
continue;
}

Copy link
Contributor

@amulet1 amulet1 Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this is the right thing to do.

If I undestand correctly, the _layout array can have non-consecutive indexes. And we want to go over each one that exists.

But the for loop only goes up to count($this->_layout), which could miss some indexes.

Example:

$this->_layout = [ 0 => $value1, 10 => $value2 ];

in this case $rows = 2 and the for loop would only check row 0 and 1, but not row 10.

Possible solutions:

  • use foreach, need to redesign "if (isset($field['height'])) {" block as it refers to other rows (could they also be non-consecutive?)
  • alternatively, avoid sparse arrays or make it non-sparse in the constructor?

$cols = count($this->_layout[$row]);
if (!isset($emptyrows[$row])) {
$emptyrows[$row] = true;
Expand Down Expand Up @@ -760,6 +765,11 @@ public function removeRowIfEmpty($row)
return true;
}

// Check if row is actually set (not just within bounds)
if (!isset($this->_layout[$row]) || !is_array($this->_layout[$row])) {
return true;
}

Copy link
Contributor

@amulet1 amulet1 Mar 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved to rowExists method (it has to be redesigned as it does not work correctly for sparse arrays).

$rows = count($this->_layout[$row]);
for ($i = 0; $i < $rows; $i++) {
if (isset($this->_layout[$row][$i]) && $this->_layout[$row][$i] != 'empty') {
Expand Down
Loading