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

OEL-1571: Display country instead skos entity reference country URL. #150

Merged
merged 3 commits into from
Jun 2, 2022

Conversation

escuriola
Copy link
Contributor

No description provided.

Comment on lines 79 to 81
'definition' => $field->getFieldDefinition()->getType() === 'skos_concept_entity_reference'
? $field->referencedEntities()
: $field->first()->getValue()['value'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong indentation. I know it is PhpStorm's fault but we should fix manually.

Suggested change
'definition' => $field->getFieldDefinition()->getType() === 'skos_concept_entity_reference'
? $field->referencedEntities()
: $field->first()->getValue()['value'],
'definition' => $field->getFieldDefinition()->getType() === 'skos_concept_entity_reference'
? $field->referencedEntities()
: $field->first()->getValue()['value'],

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, by passing a Concept entity to the template, the template will implicitly call ->label() on that entity, through this line in bcl-descritpion-list.html.twig:

                {{- _definition.label -}}

This is how it works: If the value is an object, then it will try $obj->$key and then $obj->$key().
See twig_get_attribute(). Crazy, eh?

Let's not rely on this magic.
Instead, let's do something like this:
(please don't copy this 1:1, it is only for the rough direction)

/** @var \Drupal\rdf_skos\Entity\Concept[] $concept_entities */
$concept_entities = $field->referencedEntities();
if (!isset($concept_entities[0])) {
  continue;
}
$items[] = [
  'term' => $field->getFieldDefinition()->getLabel(),
  'definition' => [['label' => $concept_entities[0]->label()]],
];

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is complex enough that we should use if/else instead of a ternary.

$concept_entities = $field->referencedEntities();
if (isset($concept_entities[0])) {
$definition = [['label' => $concept_entities[0]->label()]];
}
Copy link
Contributor

@donquixote donquixote May 25, 2022

Choose a reason for hiding this comment

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

If $concept_entities[0] is not set, we would rather skip (= continue;) this field instead of showing empty.
On the other hand, this case cannot really happen because we checked ->isEmpty() above.

I imagine @brummbar or @drishu would say we don't even need to check isset() because we fully rely on the ->isEmpty() check. I am not sure about it, there could be a case where the field is not empty, but the referenced entity does not exist or cannot be loaded.

Btw In oe_theme, we call $field->view(['label' => 'hidden', ...]) instead of what we do here.
The result is a render element.
But if we use this, we would want to get rid of field wrappers, ideally.
But for now let's use ->label(), we can rethink later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw: As long as the value is a simple string, we can skip the [[..]] array levels here, and also the 'label'.
This is due to how bcl-item-list works:

  • If $item['definition'] is "iterable" (always true for arrays), then the format has to be $item['definition'][]['label'] = 'hello'.
  • If $item['definition'] is a string or non iterable object, then the format can be $item['definition'] = 'hello', so we save multiple array levels. This is what we already do for other values right now. But this only works because these are simple strings instead of render elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, for me the empty check is sufficient

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so let's

  • Drop the [[.
  • Drop the isset check, and assume that the skos entity exists.

if ($field->getFieldDefinition()->getType() == 'skos_concept_entity_reference') {
$value = 'target_id';

$definition = '';
Copy link
Contributor

@donquixote donquixote May 31, 2022

Choose a reason for hiding this comment

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

This line is not needed now.

…ecessary as it's a required value and skipping the double array as the value it's a simple string.
@escuriola escuriola merged commit 4fb1bd1 into 1.x Jun 2, 2022
@escuriola escuriola deleted the OEL-1571 branch June 2, 2022 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants