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

Allow per-entity instance primary locales #50

Open
mpdude opened this issue Apr 4, 2024 · 1 comment
Open

Allow per-entity instance primary locales #50

mpdude opened this issue Apr 4, 2024 · 1 comment

Comments

@mpdude
Copy link
Member

mpdude commented Apr 4, 2024

For now, each entity class has one fixed primary locale, expressed through the Locale attribute.

We have encountered cases in which some records were only available in a language different from the primary locale.

Therefore, it would be nice if we could remove the primary locale attribute and retrieve this information from a property, on a per-entity instance level. This would allow every entity instance to define its own primary locale.

@martingold
Copy link

martingold commented Sep 23, 2024

Thank you for the wonderful library.

I am now refactoring an application where the primary language of the entity is stored in different entity (😐) and this issue is a blocker for me to use this nice library. I was thinking of two approaches to this. Just ideas and I would like to discuss them before implementing them.

Interface

  • Introduce new interface DynamicLocaleEntity (just placeholder name for now)
  • Handle case when the entity is instance of DynamicLocaleEntity and has attribute at same time
  • Check if given entity is instance of DynamicLocaleEntity in TranslatableClassMetadata::injectNewPersistentTranslatables
  • If so, use the getLocale() value instead the attribute value
interface DynamicLocaleEntity
{
    public function getLocale(): string;
}
public function injectNewPersistentTranslatables(object $entity, EntityManager $entityManager, DefaultLocaleProvider $defaultLocaleProvider): void
    {
        foreach ($this->translatedProperties as $fieldName => $property) {
            $persistentTranslatable = new PersistentTranslatable(
                $entityManager->getUnitOfWork(),
                $this->class,
                $entity,
                // Handle case of missing #[Locale] attribute while no being instance of DynamicLocaleEntity
                $entity instanceof DynamicLocaleEntity ? $entity->getLocale() : $this->primaryLocale,
                $defaultLocaleProvider,
                $this->translationFieldMapping[$fieldName],
                $this->translationsCollectionProperty,
                $this->translationClass,
                $this->translationLocaleProperty,
                $this->translationMappingProperty,
                $property,
                $this->logger
            );
            $persistentTranslatable->inject();
        }
    }
  • Con: polluting every translatable entity with new method and interface (the interface seems rather pointless), very inconsistent since the library uses attributes everywhere else
  • Pro: easier to implement, less magical than the other approach

Questions:
Is the Locale attribute needed? The getLocale() could return literal string.

Extend Locale attribute

  • Add property method and property to Locale attribute
  • Ensure only one of primaryLocale, method or property is defined
  • Add primaryLocaleMethod and primaryLocaleProperty properties to TranslatableClassMetadata
  • Include logic for fetching the primaryLocale to injectNewPersistentTranslatables
#[Entity]
#[Locale(method: 'getEventLanguage')]
class Event
{
    public function getEventLanguage(): string
    {
        return $this->...
    }
}
#[Entity]
#[Locale(property: 'locale')]
class Event
{
    public string $locale;
}
public function injectNewPersistentTranslatables(object $entity, EntityManager $entityManager, DefaultLocaleProvider $defaultLocaleProvider): void
    {
        // More robust error handling needed to include reason and how to fixit → Method Event::getLocale() does not exist. Implement it or check if method specified in attribute #[Locale] of App\Entity\Event exists.
        $entityLocale = match (true) {
            $this->primaryLocale !== null => $this->primaryLocale,
            $this->primaryLocaleMethod !== null => call_user_func([$entity, 'primaryLocaleMethod']),
            $this->primaryLocaleProperty !== null => $entity->{$this->primaryLocaleProperty},
            default => throw new LogicException('...'), // Should be checked by the Locale attribute 
        };

        foreach ($this->translatedProperties as $fieldName => $property) {
            $persistentTranslatable = new PersistentTranslatable(
                $entityManager->getUnitOfWork(),
                $this->class,
                $entity,
                $entityLocale,
                $defaultLocaleProvider,
                $this->translationFieldMapping[$fieldName],
                $this->translationsCollectionProperty,
                $this->translationClass,
                $this->translationLocaleProperty,
                $this->translationMappingProperty,
                $property,
                $this->logger
            );
            $persistentTranslatable->inject();
        }
    }

Pro: more versatile and useful. Does not pollute every translatable entity with interface implementation
Con: calling method/getting property based on a string is a bit yikes and it may be very error-prone (would it not be nice if PHP supported first class callable syntax in attributes? 😁).

Questions:

  • Should the private properties be supported?
  • If no is the property access even worth implementing?

Which of these two approaches would have better chance of getting merged if i were to implement it?
Am I missing something critical @mpdude ?

Thank you!

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

No branches or pull requests

2 participants