CRUD icon indicating copy to clipboard operation
CRUD copied to clipboard

[Improvement] Consistent options for checklist with other relationship fields

Open o15a3d4l11s2 opened this issue 4 years ago • 2 comments

There is a slight difference between some of the fields, for example checklist.blade.php and relationship_select.blade.php in the way it works with user-provided options.

Checklist:

  if (!isset($field['options'])) {
      $field['options'] = $field['model']::all()->pluck($identifiable_attribute, $key_attribute)->toArray();
  } else {
      $field['options'] = call_user_func($field['options'], $field['model']::query());
  }

Relationship_select:

  if (!isset($field['options'])) {
    $field['options'] = $connected_entity::all()->pluck($field['attribute'],$connected_entity_key_name);
  } else {
    $field['options'] = call_user_func($field['options'], $field['model']::query())->pluck($field['attribute'],$connected_entity_key_name);
  }

Note the call_user_func behaviour - in the checklist only the user function is executed so the user should additionally pluck the id and value. In the relationship_select it executes the function and plucks based on the attribute.

I know this is a small detail, but since the attribute is required for the checklist field to work it would be better if it is used to pluck inside the field's logic.

Maybe you are already aware of this, but do not want to change it because it is a breaking change?

o15a3d4l11s2 avatar Jan 11 '21 08:01 o15a3d4l11s2

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication channels:

  • Bug Reports, Feature Requests - Github Issues (here);
  • Quick help (How do I do X) - Gitter Chatroom;
  • Long questions (I have done X and Y and it won't do Z wtf) - Stackoverflow, using the backpack-for-laravel tag;
  • Showing off something you've made, asking for opinion on Backpack/Laravel matters - Reddit;

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

-- Justin Case The Backpack Robot

welcome[bot] avatar Jan 11 '21 08:01 welcome[bot]

Hello @o15a3d4l11s2 and thanks for the report.

You are right here. Technically checklist is a relationship. But this concept of relationships was recently introduced and some bits of code slipped us.

Indeed, atm we cannot change it without a breaking change, but I will add this to be fixed in the next version where we could push BC.

Wish you the best, Pedro

pxpm avatar Jan 11 '21 11:01 pxpm

Hey @o15a3d4l11s2 hope you are doing well man 👍

It seems I've missed a detail in my previous assessment of the issue. 😞

We can make it non-breaking, in fact I already did in https://github.com/Laravel-Backpack/CRUD/pull/5562 🤷‍♂️

Thanks for the heads up 🙏 I will be closing this and move the conversation to the PR that should be merged shortly.

If you give it a test, leave your thumbs up in the PR thread 👍

pxpm avatar Jul 14 '24 11:07 pxpm