Refactor Turba_View_List by amulet1 · Pull Request #27 · horde/turba

amulet1 · 2026-03-17T18:22:26Z

Pre-initialize column variables via createVariable(), eliminating per-row overhead

Move Horde_Form_Variable initialization from row.inc into constructor,
leveraging Horde_Form::createVariable() instead of direct getType()
calls, eliminating per-row per-column isset() checks and throwaway
Horde_Form instantiations
Normalize email column type to 'html' at initialization time
Replace index-based for loop in row.inc with foreach over columns,
using parallel index into pre-built variables array
Remove now-unused $form property (was always null) and pass null
directly to renderer->render()
Restrict $variables and $columns to protected visibility
Mark unused variables in display() for removal ($hasDelete, $hasEdit,
$hasExport, $start, $end)

This fixes #25 (inability to view address books with customized columns).

IMPORTANT: This code relies on the recent change ( see horde/Form@971e5ee) in Horde_Form class.

…Variable(), eliminating per-row overhead - Move Horde_Form_Variable initialization from row.inc into constructor, leveraging Horde_Form::createVariable() instead of direct getType() calls, eliminating per-row per-column isset() checks and throwaway Horde_Form instantiations - Normalize email column type to 'html' at initialization time - Replace index-based for loop in row.inc with foreach over columns, using parallel index into pre-built variables array - Remove now-unused $form property (was always null) and pass null directly to renderer->render() - Restrict $variables and $columns to protected visibility - Mark unused variables in display() for removal ($hasDelete, $hasEdit, $hasExport, $start, $end)

ralflang · 2026-03-17T18:33:47Z

Please double check if this is still needed - today's release cannot reproduce the issue reported.

amulet1 · 2026-03-17T19:00:54Z

Yes, it fixes the issue, but this PR should still be merged as it also redesigns the class by eliminating bad code.

amulet1 · 2026-03-17T19:04:02Z

It is also important to note that for V3 type is part of variable class, so the approach "create type, then create variable using this type" would not be viable. By using createVariable() now we are avoiding the future redesign.

amulet1 · 2026-03-17T19:57:07Z

Also, I just noticed this in horde/Form@dd8194a:
"External code should use Horde_Form_Type::create() instead for instantiating types."

I am sorry, but The Horde_Form_Type::create() should have never been added. Not only it is a step backward, but it also does not have support for named parameters that getType() does, so if it is used, it will recreate issues that have been fixed already. Horde_Form::createVariable() was added specifically for the purpose of creating variable of specific type (as it is static it can be easily moved elsewhere, if needed).

V3 types merge Horde_Form_Type and Horde_Form_Variable. With V3 we will just update createVariable() internally and migrate to a different class, but its logic and parameters will stay.

ralflang · 2026-03-18T07:26:31Z

Also, I just noticed this in horde/Form@dd8194a: "External code should use Horde_Form_Type::create() instead for instantiating types."

I am sorry, but The Horde_Form_Type::create() should have never been added. Not only it is a step backward, but it also does not have support for named parameters that getType() does, so if it is used, it will recreate issues that have been fixed already. Horde_Form::createVariable() was added specifically for the purpose of creating variable of specific type (as it is static it can be easily moved elsewhere, if needed).

V3 types merge Horde_Form_Type and Horde_Form_Variable. With V3 we will just update createVariable() internally and migrate to a different class, but its logic and parameters will stay.

createVariable is a good step forward but it has a major caveat: It tries to wrap concerns of constructing a variable-of-type into an operation USING that type. That's OK for now but I would much prefer that new thing eating a typed interface.

´´´
createVariable(string humanName, string varName, TypeInterface $type, bool required, bool readonly, string description)
´´´
or, if you really want it (i'd rather break this newly introduced interface and get it right now rather than in 3 years)

´´´
createVariable(string humanName, string varName, TypeInterface $type, array $bagOfTypeAttributes, bool required, bool readonly, string description)
´´´

In the current form I'd rather not propagate the bag of attributes pattern in new places.

Going forward, I'd like to support this style:

$Form->buildVariable(optionally support createVariable arguments here but don't require)->withType($)->withHumanName($)->withVarName($)->...->commit()->buildVariable()->withVarName...

In the current form I veto this PR.

ralflang

Please untangle the "better handling" aspect from the createVariable aspect.
Please explain permission handling in the new model and then remove rather than comment out code.

ralflang · 2026-03-18T07:28:40Z

lib/View/List.php

+
+        $this->columns = $columns ?? [];
+        foreach ($this->columns as $column) {
+            $type = $GLOBALS['attributes'][$column]['type'];


Avoid new access to globals. Feed the global into the view constructor and use it as a member variable inside.

ralflang · 2026-03-18T07:28:48Z

lib/View/List.php

+                $type = 'html';
+                $params = [];
+            } else {
+                $params = $GLOBALS['attributes'][$column]['params'] ?? [];


Avoid new access to globals. Feed the global into the view constructor and use it as a member variable inside.

ralflang · 2026-03-18T07:29:32Z

lib/View/List.php

-        $hasDelete = $driver->hasPermission(Horde_Perms::DELETE);
-        $hasEdit = $driver->hasPermission(Horde_Perms::EDIT);
-        $hasExport = ($GLOBALS['conf']['menu']['import_export'] && !empty($GLOBALS['cfgSources'][Turba::$source]['export']));
+        //$hasDelete = $driver->hasPermission(Horde_Perms::DELETE);


No - please explain how permissions are now handled and then remove the code, don't comment out.

ralflang · 2026-03-18T07:29:44Z

lib/View/List.php

            $max = $min + $perpage;
-            $start = ($page * $perpage) + 1;
-            $end = min($numitem, $start + $perpage - 1);
+            //$start = ($page * $perpage) + 1;


Explain and remove

ralflang · 2026-03-18T07:31:19Z

lib/View/List.php

+            } else {
+                $params = $GLOBALS['attributes'][$column]['params'] ?? [];
+            }
+            $this->variables[] = Horde_Form::createVariable('', $column, $type, $params);


See generic discussion on createVariable - keep the current solution until we have finalized the createVariable discussion. We should separate both aspects to unblock this current PR.

amulet1 requested a review from ralflang March 17, 2026 18:22

amulet1 requested a review from TDannhauer March 17, 2026 19:58

ralflang requested changes Mar 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Turba_View_List#27

Refactor Turba_View_List#27
amulet1 wants to merge 1 commit intohorde:FRAMEWORK_6_0from
amulet1:fix_list_view

amulet1 commented Mar 17, 2026 •

edited

Loading

Uh oh!

ralflang commented Mar 17, 2026

Uh oh!

amulet1 commented Mar 17, 2026

Uh oh!

amulet1 commented Mar 17, 2026 •

edited

Loading

Uh oh!

amulet1 commented Mar 17, 2026 •

edited

Loading

Uh oh!

ralflang commented Mar 18, 2026 •

edited

Loading

Uh oh!

ralflang left a comment

Uh oh!

ralflang Mar 18, 2026

Uh oh!

ralflang Mar 18, 2026

Uh oh!

ralflang Mar 18, 2026

Uh oh!

ralflang Mar 18, 2026

Uh oh!

ralflang Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

amulet1 commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ralflang commented Mar 17, 2026

Uh oh!

amulet1 commented Mar 17, 2026

Uh oh!

amulet1 commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amulet1 commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ralflang commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ralflang left a comment

Choose a reason for hiding this comment

Uh oh!

ralflang Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

ralflang Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

ralflang Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

ralflang Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

ralflang Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

amulet1 commented Mar 17, 2026 •

edited

Loading

amulet1 commented Mar 17, 2026 •

edited

Loading

amulet1 commented Mar 17, 2026 •

edited

Loading

ralflang commented Mar 18, 2026 •

edited

Loading