- Issue created by @Dan.Ashdown
- Status changed to Needs review
over 1 year ago 11:52am 20 October 2023 - Issue was unassigned.
- 🇦🇹Austria fago Vienna
Looks all good, needs some testing though still since we have no automated tests for this.
- Status changed to Needs work
9 months ago 3:50pm 16 July 2024 - 🇸🇮Slovenia useernamee Ljubljana
I gave this issue another look, because I came to the similar solution in 📌 Support for Search Index View and Rendered entity row Active . Here I focused on support for entity reference fields:
if ($fields_plugin) { $custom_element = new CustomElement(); foreach ($this->view->field as $name => $value) { // @todo Field row plugin is not yet supported and is broken for // multi value entity reference fields. // Actually, we should use this method for all entity reference fields not just multi value ones. if ($value->options['type'] === 'entity_reference_entity_view' && $value->multiple) { $entity = $value->getEntity($row); $targets = $entity->get($name)->referencedEntities(); $referenced_custom_elements = []; $view_mode = $value->options['settings']['view_mode'] ?? 'default'; foreach ($targets as $target) { $referenced_custom_elements[] = $this->getCustomElementGenerator()->generate($target, $view_mode); } $custom_element->addSlotFromNestedElements($name, $referenced_custom_elements); } else { $custom_element->setAttribute($name, $value->render($row)); } } }
- 🇸🇮Slovenia useernamee Ljubljana
Since this branch was kind of stalled I took the liberty and add my proposal to support referenced entities in it.
- 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
That looks good. But
1) I see that there are a few differences between this and 📌 Support for Search Index View and Rendered entity row Active code, while they are solving the same thing.
The code in this MR contains one more separate path than the #3493780 MR, because this has
if ($value->options['type'] === 'entity_reference_entity_view') {
The #3493780 MR contains some more code abstractions and updates to new syntax.
This MR has phpcs errors which are new. (I think phpstan errors are preexisting...)
I assume we are going to merge this MR first? I don't have an opinion on whether the " code abstractions and updates to new syntax" should be included here, do whatever you want in that respect. As long as phpcs is solved.
2) This needs tests. I was a bit hesitant whether I would want to set this ticket to "Needs work" for that, but @fago in #3493780 commented the same, and I agree with him.
This is sufficiently abstract that we should make sure it keeps working. Add an example view (if not there yet) for every case that could produce different output / make different calls (3 or 4 cases?) and just test that some output still looks sane.
- 🇸🇮Slovenia useernamee Ljubljana
> while they are solving the same thing.
Well, actually this issue focuses on a different aspect of the views integration into lupus_decoupled than the #3493780 (with the support for entity reference fields in view data row result (which is independent of search)). What this ticket does is that it allows rendering of fields that are entity references (and could be rendered as entities). But this is not strictly needed for Drupal CMS Search. The focus of this ticket is the support of fields format in view (on the other hand the focus of the #3493780 ticket is the support of Search API and the rest of configuration that comes with the Drupal CMS Search recipe). There is high overlap between the tickets, but in the commit history of the #3493780 first approach was a bit different, but then I noticed that doing things the way this solves 2 problems in one go.
> This needs tests.
Well, I'm a bit reserved about that. I gave another look to this ticket because I got deep understanding of the views support functionality while working on #3493780. Tests are welcome, but this is not strictly needed for DrupalCMS, so if anything I'd create a follow up for them.
I'll fix the phpcs issues, but let's postpone this ticket until #3493780 is merged.
- 🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
OK. Last time I looked, I only saw the changes to views classes in 📌 Support for Search Index View and Rendered entity row Active . Now I see extra stuff there. So it makes sense to review that first and leave the "fields" part from this issue, out of it, yes.
But then, if I understand correctly, the title and summary of this issue will already be solved by #3493780. Which is great; that is (now) tested.
Since all the non-fields parts already have tests, I think the addition of another test for the 'fields' part would likely not be that much work? But we can quibble about those details later.