Problem/Motivation

There are a few problems when creating homeboxes directly:

1.
If we press on the "Add Homebox / Preset" button on "/admin/structure/homebox_type/{homebox_type}/homeboxes" "isPreset" is enabled by default, which is quite confusing. We should differ between "Add Homebox" and "Add Preset" buttons and prebuild the form based on url parameters

2.
When going to the "/admin/homebox/add/{homebox_type}" route, the following error appears:

Warning: Undefined array key 0 in Drupal\homebox\Plugin\Field\FieldWidget\PortletWidget->formElement() (line 84 of modules/custom/homebox/src/Plugin/Field/FieldWidget/PortletWidget.php).

3. When adding a preset, we should manually suffix the preset number to the label. The automatically generated preset on homebox type generation is called "{homebox_type->label} Preset 0". So any other created presets should be named preset 1..2....5 etc..
Meaning we should load all preset homeboxes of the defined homebox_type, count them and add the appropriate number to prepopulate the add form label.

Steps to reproduce

Proposed resolution

Fix the problems.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork homebox-3439626

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Grevil created an issue. See original summary.

Grevil’s picture

Assigned: Unassigned » Anybody
Status: Active » Needs review

All done, please review!

Grevil’s picture

Anybody’s picture

Assigned: Anybody » Grevil
Status: Needs review » Needs work

Nice work, but I'm not really sure the context parameter is the best possible solution.

I'm a bit more in favor of using inheritance and two different form classes inheritng from the base class.
That's typically the better polymorphysm pattern instead of if's that tend to grow by time and code becomes ugly.

Furthermore, in the current implementation you have to ensure the context parameter is always present, which is also a sign of improvable design. In that case two separate same-based routes / URLs could do the job better without edge-cases like

  • unexpected value for context
  • missing context

which for routes the system would handle itself by design (404)

Grevil’s picture

Assigned: Grevil » Unassigned
Status: Needs work » Needs review

All done, final review pls.

Anybody’s picture

Status: Needs review » Reviewed & tested by the community

LGTM. #5 will be documented and solved in a follow-up later. @Grevil will link it in both directions.

  • Grevil committed 63eafc6d on 3.0.x
    Issue #3439626 by Grevil: Issues when creating homebox presets / other...
Grevil’s picture

Thanks!

Grevil’s picture

Status: Reviewed & tested by the community » Fixed
Grevil’s picture

Status: Fixed » Closed (fixed)

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