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

#1116 - add language option to the entity buffer #1117

Open
wants to merge 5 commits into
base: 8.x-4.x
Choose a base branch
from

Conversation

hchonov
Copy link

@hchonov hchonov commented Nov 24, 2020

Fixes #1116

Copy link
Contributor

@klausi klausi left a comment

Choose a reason for hiding this comment

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

Thanks, I think we could do that. The language adds another dimension to the buffer key I think, so we could split up the buckets per language as well. Loaded entities are cached and getting another translation should not trigger database queries (as far as I can see), so would make sense.

The only downside then is that if you use different entities and different languages that the entity load is done multiple times per language.

And instead of touching the EntityBuffer we could make an EntityLanguageBuffer, like we do for UUID and revisions?

Then again I don't fully understand your use case - can you use the EntityLoad data producer which has language support already?

@@ -55,6 +58,7 @@ protected function getBufferId($item) {
*/
public function resolveBufferArray(array $buffer) {
$type = reset($buffer)['type'];
$language = reset($buffer)['language'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently the buffer is keyed by entity type. Do we also need to key it by language as well? You are assuming that all items in this buffer want the same language. Maybe that is an ok assumption to make? Not sure here.

if (!empty($entities[$current])) {
array_push($carry, $entities[$current]);
$entity = $language ? $entities[$current]->getTranslation($language) : $entities[$current];
Copy link
Contributor

Choose a reason for hiding this comment

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

this will throw fatal errors if the entity type is not translatable. That is also why PHPStan is complaining. Maybe that is ok and we don't care? We could already throw an exception if a language is passed in the add() method on not translatable entity types.

@klausi klausi added the 4.x label Dec 6, 2020
@Kingdutch
Copy link
Contributor

The use case for this is not entirely clear to me. The EntityBuffer exists to efficiently load a lot of entities at once using loadMultiple, this prevents the notorious N+1 problem. Drupal's built-in entity system expects all translations for a given entity to be loaded at once (i.e. translations are always part of the same entity data object, and not separate things). So loading an entity for any language will load it for all languages, which means that there's no benefit in making the EntityBuffer language aware.

We can see this in the SQL storage implementation which loads translations from shared tables while converting the results from storage records to entities will also query the translation tables to fetch translation data: https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php#L498

We can also see that calling getTranslation on an entity will not cause any additional loading, but will do a lookup in the entity's translation data or create a new translation based on the default language if there was none before: https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/lib/Drupal/Core/Entity/ContentEntityBase.php#L857

Unless I'm missing something we should probably close this as a won't fix, and the correct usage would be to call getTranslation() on the result of the entity buffer (since that function call does not cause additional data to be loaded from the database).

@hchonov hchonov force-pushed the entity_buffer_language_option branch from bc35d60 to b06b46e Compare June 9, 2023 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add language option to the entity buffer
3 participants