Add column default attribute & make that default consistent across column types

[promatik]

This PR combines #3903 + #3437.

Questions from #3903;

Before this PR, we just can't change the default - that appears in some fields. This PR solves it by adding the default everywhere, BUT, I think this is more like a placeholder and not a default 😕

In some cases like the date field, we can't set the default to -, because the default should be a date or null. But one may want the default date to be kept as null and a placeholder of - or '' (empty) when date is null. That is not possible.

@tabacitu I think you need to step in ✌

Questions from #3437;

Should the - be a config?

Since we are talking about it, should [...] be a config also? I would like to use the Unicode Character … (U+2026) only 😅

[promatik]

Another note on this, in some places the default is replacing the value, so the default behavior gets done, adding the prefix and suffix. Other don't. This is in my opinion another reason to either add the placeholder or make the - configurable.

[tabacitu]

@promatik - I think placeholder is a much better name for this than default... Unfortunately, for consistency with fields, I think we should keep value and default - just like we have in fields. We may merge fields and columns in v5/v12 so let's not add stuff to the nomenclature.

In the end, I propose we do this:

• refactor all the columns to store the value inside $column['value'] instead of $value
• allow the dev to overwrite $column['value'] like they overwrite any other attribute - we could just do smth like $column['value'] = $column['value'] ?? data_get($entry, $column['name']);
• use $column['default'] as the placeholder text, when $column['value'] is null, instead of $column['prefix'].$column['value'].$column['suffix']
• have - as the default $column['default'] for all columns (since it won't overwrite the value, just what is shown if value is null);

While we're at it, since we're now adding a customizable value attribute, we should probably also:

• allow $column['value'] to be a closure (function ($entry) {}); this would render the closure column obsolete so... we can deprecate it, by
• add a note to the closure column in the 4.2 docs, that it will be removed in a future version of Backpack, because the same thing can now be achieved using any column (including the text column) and the value attribute - just pass the same closure to the value attribute of any column type;

This way:

• columns will behave similarly to fields;
• dev will be able to define a certain value if they want, which will be echoed prefix-value-suffix, instead of whatever is in the db;
• dev will be able to define a certain default (aka placeholder) if the want, which will be echoed directly, without prefix and suffix, if the value is null;

Sounds reasonable? @zachweix what do you think?

[scrutinizer-notifier]

The inspection completed: No new issues

[promatik]

This was a lot of work and a lot of testing, but I think the result is good 👌

Columns are now much more standard. 🙌

The PR for the deprecation of closure column Laravel-Backpack/docs#278.

[tabacitu]

I love it ❤️ Very clean and complete PR, good job @promatik ! This will make it easier to make $column an object in the future too, because we'll just need to do CrudColumn implements ArrayAccess, and all its attributes will make sense.

I will test & merge it as it is now. No point in making any more stylistic changes. Perfect is the enemy of good.

But... for future reference, it seems to me like most columns now have this order in the php logic:

@php
// calculate value
// calculate others (escaped, prefix, suffix etc.)
// finish calculating value
@endphp

I think it'd been cleaner if the value got calculated in just one spot, either:

@php
// calculate value
// calculate others (escaped, prefix, suffix etc.)
@endphp

or

@php
// calculate others (escaped, prefix, suffix etc.)
// calculate value
@endphp

For example in the array column, this would look cleaner to me:

-   $column['value'] = $column['value'] ?? data_get($entry, $column['name']);
    $column['escaped'] = $column['escaped'] ?? false;
    $column['prefix'] = $column['prefix'] ?? '';
    $column['suffix'] = $column['suffix'] ?? '';

+   $column['value'] = $column['value'] ?? data_get($entry, $column['name']);

    if(is_callable($column['value'])) {
        $column['value'] = $column['value']($entry);
    }
    // the value should be an array whether or not attribute casting is used
    if (!is_array($column['value'])) {
        $column['value'] = json_decode($column['value'], true);
    }

BUUT. It wouldn't have been as surgical. That's why I think we should NOT do the above right now. That's a task for future-us 😀 Again, perfect is the enemy of good - we should remember that more often 😅

---

Of course, if we wanted to be fancy, we could go as far as doing something like:

-   $column['value'] = $column['value'] ?? data_get($entry, $column['name']);
+   $column['value'] = (function($entry) uses ($column) {
+        // if the value is a closure, run it; if it's defined, use it; if null, get the attribute on the entry;
+        $value = is_callable($column['value']) ? $column['value']() : $column['value'] ?? data_get($entry, $column['name']);
+        
+        // the value should be an array whether or not attribute casting is used
+        if (!is_array(value)) {
+            return json_decode($value, true);
+        }
+
+        return $value;
+     })();
    $column['escaped'] = $column['escaped'] ?? false;
    $column['prefix'] = $column['prefix'] ?? '';
    $column['suffix'] = $column['suffix'] ?? '';

-    if(is_callable($column['value'])) {
-        $column['value'] = $column['value']($entry);
-    }

But again. We're NOT doing that right now. No point - we'll be moving this logic into PHP classes in a few months.
I'm telling myself that, not just you @promatik 🤣

Again, good job on this!

[tabacitu]

Found a few minor inconsistencies, but overall... looks to be VERY backwards-compatible. Good job @promatik .

[tabacitu]

This changes behaviour when the relationship_count value is zero. Before it was showing an empty cell. Now it's showing "0 items" which I don't think makes sense (especially if you add a link to it).

Why use !empty() here @promatik ?

I would see the best case here as showing the $column['default'] like all other columns.

It's arguable though. More consistent would be to show 0 items like it does now... I don't know...

[tabacitu]

Ok scratch that, I changed my mind. It's more consistent to show 0 items - more important to be consistent with itself than with other columns. I'm ok with how this is now, showing 0 items.

[promatik]

No! 😅 I think you are right ...
array_count works the same way, I think we should keep the cell empty ...

[promatik]

But this raises an issue

If we have a wrapper with a link, the link will be there with the dash, should it?

[promatik]

And this may happen in other fields where we just added the dash - 😬
Maybe I should refactor this a little bit more so that;

@if(!empty($column['value']))

    @includeWhen(!empty($column['wrapper']), 'crud::columns.inc.wrapper_start')
        {{ $column['prefix'] }}

        @if($column['escaped'])
            {{ $column['value'] }}
        @else
            {!! $column['value'] !!}
        @endif

        {{ $column['suffix'] }}
    @includeWhen(!empty($column['wrapper']), 'crud::columns.inc.wrapper_end')

@else
    {{ $column['default'] ?? '-' }}
@endif

This way, the empty dash will always be outside the wrapper ...

Let me know @tabacitu ✌, I know in advance – we shouldn't be worrying that much with perfection – but let's hope this is the last change ✌😅

[tabacitu]

I've just re-tested this and decided it's good enough as-is. We can of course find more quirks and inconsistencies and gradually do them, but for now... this will do.

@promatik please add issues or make separate PRs in 4.2.x if you think there are more things to do in regard to this. But for now, merging 💪 🎉🎉🎉