Skip to content
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.

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

ensure table column works with associative arrays too #3387

Merged
merged 6 commits into from
Feb 4, 2022

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Dec 9, 2020

refs: #2635

Problem: Using table column to display a single array of values.

Solution: As is expected by table column, we ensure to provide a multi-dimensional array when developer provide a single array.

If I understood code correctly, only when the field is not casted as array we endup with an object as value because of the json_decode, forcing true as a second parameter we ensure an array output instead of object.

So we migth be able to remove the conditional check for is_object().

@pxpm pxpm requested a review from promatik December 9, 2020 14:59
@pxpm pxpm self-assigned this Dec 9, 2020
@pxpm pxpm added the Feature label Dec 10, 2020
@promatik
Copy link
Contributor

@pxpm I tested this, and it's braking the current table.
Maybe here it should be a different sign(?) count($value) !== count($value, COUNT_RECURSIVE)

Also, can you give me an example where we can use a field to create an associate array so I can test that part?

image

image

@iMokhles
Copy link
Contributor

iMokhles commented Dec 13, 2020

@promatik i tested it and it works

normal table 2 json
Capture d’écran 2020-12-13 à 04 25 28

normal table 1 json
Capture d’écran 2020-12-13 à 04 28 09

before changes
Capture d’écran 2020-12-13 à 04 30 48

AFTER changes

associative array
Capture d’écran 2020-12-13 à 04 26 11

how did you save your array and which php version you have !

My PHP version 7.3.9

…ith-single-dimension-arrays

# Conflicts:
#	src/resources/views/crud/columns/table.blade.php
@pxpm pxpm changed the base branch from master to 4.2 December 23, 2021 12:42
@pxpm pxpm assigned promatik and unassigned pxpm Dec 23, 2021
@pxpm pxpm changed the title ensure table works with associative arrays too ensure table column works with associative arrays too Dec 23, 2021
@pxpm
Copy link
Contributor Author

pxpm commented Dec 23, 2021

ping @tabacitu @promatik

Merged 4.2 here, fixed conflicts.

This is a NON-BC I think, let me know if I am wrong.

Pedro

@tabacitu
Copy link
Member

If it's non-BC and a SHOULD (not a MUST) then it's a problem for future us. Moving to 4.2.x. Let's focus on the 4.2 launch please, we have more important things to finish.

@scrutinizer-notifier
Copy link

The inspection completed: 544 Issues, 80 Patches

@tabacitu
Copy link
Member

@pxpm this column will soon be moved to backpack/pro, so please take another look at this, test it and tell me if this is 100% a non-breaking change and I can merge it. If not, we leave it for future us (after we launch 4.2).

Thanks!

@pxpm
Copy link
Contributor Author

pxpm commented Feb 3, 2022

@tabacitu I've done the required changes to make this non-BC and solve the problem we are trying to solve here.

So before table would work if:

  • You gave it a json string
  • You gave it a multidimensional associative array

After it works:

  • You gave it a json string
  • You gave it a multidimensional associative array
  • You gave it a single associative array

I've removed the need to check for is_object in html by adding the second parameter to json_decode($var, true) that forces to decode nested objects too, and by casting to array (array) $obj in case developer passed an object.

AFAIK nothing breaks, a good and easy way to test is by adding the features column in Product crud, that has a table field.

   CRUD::addColumn([
       'name' => 'features',
       'type' => 'table',
       'columns' => ['name' => 'name', 'desc' => 'desc']
   ]);

If you want to test the other scenarios too you can pass them in column 'value' => function($entry) {}

@pxpm pxpm assigned tabacitu and unassigned promatik and pxpm Feb 3, 2022
Copy link
Contributor

@promatik promatik left a comment

Choose a reason for hiding this comment

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

Tested, everything is working and I think this is a non breaking change 🙌

There was only one situation that was not covered by the validations, it was the case the user sent an array of objects, something like;

'value' => function ($entry) {
    return [
        (object) ['name' => '1', 'desc' => 'a'],
        (object) ['name' => '2', 'desc' => 'b'],
    ];
},

But this can't happen by the normal usage of the field (save/retrieve).

@promatik promatik requested review from tabacitu and removed request for tabacitu February 4, 2022 00:13
@tabacitu tabacitu merged commit 04871ee into 4.2 Feb 4, 2022
@tabacitu tabacitu deleted the column-table-to-work-with-single-dimension-arrays branch February 4, 2022 07:38
This was referenced Feb 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants