- Issue created by @roderik
- 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
The phpunit failure is not for this change; it's only depecation messages which I'll fix soon in 🐛 PHPUnit failure Active .
So- tests actually 'succeeded' (for markup)
- and for (the changed) JSON output we have no tests yet. I should add these after the code change is approved (either in this issue or in a followup).
About alternative implementation
I just want to mention this for completeness - Disclaimer: there are gaps in my knowledge
It feels to me like, in principle, given the fact that the above 'JSON array' is apparently invalid... the Normalizer is responsible for not producing invalid output.
So, instead of the change in the MR ("bulid/render-into-element phase"). the "render-into-output" phase should have the fix.
That would look something like changing some code in normalizeSlots() to this pseudo code:
if ($element->hasSlotNormalizationStyle($slot_key, CustomElement::NORMALIZE_AS_SINGLE_VALUE) || (ThisSlotOnlyHasASingleValue($slot_key) && MyFormatCannotHandleArrayValues()) ) { $slot_data = reset($slot_data); }
Where MyFormatCannotHandleArrayValues() is either always true, or can be made to check the
$format
parameter for thenormalize(mixed $object, ?string $format = NULL, ...)
call.However,
- it is harder for me to see if that has wider implications
- if we would implement this, I'm starting to wonder if NORMALIZE_AS_SINGLE_VALUE still has an actual use in the future, or whether we should also start to deprecate that
- I have no clue if all consumers (JS frameworks) would have problems with array values. (Hence checking the $format parameter.)
- We are not using the $format parameter for anything yet (even though we soon will, in an upcoming change to ✨ Create a HTML-to-CE parser Active that I or Mustapha should still document).
So, this is too 'unknown' for me to make a MR out of, unless you actually agree that this should be the direction. (The current MR that changes DefaultFieldItemListProcessor, feels more in line with... how the code is currently set up...?)
- 🇦🇹Austria fago Vienna
+1 on changing it like this, also on changing in the way as proposed in the MR. Still I added some remarks!
if ($element->hasSlotNormalizationStyle($slot_key, CustomElement::NORMALIZE_AS_SINGLE_VALUE)
|| (ThisSlotOnlyHasASingleValue($slot_key) && MyFormatCannotHandleArrayValues()) ) {
$slot_data = reset($slot_data);
}This pseudo-code does not make sense to me. It'S not that Vue cannot handle that array, it's that it does not make much sense to render into a single Vue component via a slot. The body is typically rendered using a simple slot, what semantically makes sense, but that's done with something like this pseudo-code:
<component content="$SLOT"></component>
$SLOT must be string then and will fail it's an array. Thus, the ARRAY fails because our frontend-code excpects a string.That said, it seems we had this working before - although it's not so logical - and it makes frontend code more consistent to have it working again. Because of that, I turned around and fixed the frontend, so it IS working now.
Anyway, I think we shall do this chance still, because we want to have a default output which is semantically good. And this is without the array.
--> I re-title this a bit.
Still, before merging - I think we need test coverage here. Ideally we could extend the unit test to cover it, but howsoever, we shall make sure we have this behavior covered by tests. thus setting needs work for that.
- 🇦🇹Austria fago Vienna
after commenting this, I realize that changing this won't help our default output since by default the body field *has* a summary enabled!
Maybe we should go back and instead of doing this simply improve the default-config for our body field? Could we use the "flatten" plugin to flatten the output in two slots, like summary and body/default slot? If not, I'd be fine with simply only output the full-body field by default in the slot and not handling the summary. Folks can still change the config if the summary shall be used somehow.