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

Deprecate EnumInterface::getName #15

Closed
3 tasks
yann-eugone opened this issue Nov 22, 2017 · 6 comments
Closed
3 tasks

Deprecate EnumInterface::getName #15

yann-eugone opened this issue Nov 22, 2017 · 6 comments

Comments

@yann-eugone
Copy link
Collaborator

yann-eugone commented Nov 22, 2017

An enum already has an identifier : it's class.

Symfony did the same with form types in 2.8.

EnumInterface::getName should be deprecated.

  • Deprecate the method
  • Update the doc
  • Add a note for next major
@yann-eugone yann-eugone mentioned this issue Nov 22, 2017
3 tasks
@yann-eugone
Copy link
Collaborator Author

@J-Ben87 what do you think ?

@J-Ben87
Copy link
Contributor

J-Ben87 commented Jan 14, 2018

I totally agree, I have already been emulating this behaviour for a while:

public function getName()
{
    return static::class;
}

@yann-eugone
Copy link
Collaborator Author

The more I think about it, the more I wonder if this is a good idea.

I never tried something like this, but I wonder if this can be even better than what we do already :

<?php

use Yokai\EnumBundle\Enum\EnumInterface;

class ConfigurableEnum implements EnumInterface
{
    private $name;
    private $choices;

    public function __construct($name, array $choices)
    {
        $this->name = $name;
        $this->choices = $choices;
    }

    public function getName()
    {
        return $this->name;
    }

    public function getChoices()
    {
        return $this->choices;
    }
}
<?php

use Symfony\Component\Translation\TranslatorInterface;
use Yokai\EnumBundle\Enum\AbstractTranslatedEnum;

class ConfigurableTranslatedEnum extends AbstractTranslatedEnum
{
    private $name;
    private $values;

    public function __construct(TranslatorInterface $translator, $transPattern, $name, array $values)
    {
        parent::__construct($translator, $transPattern);

        $this->name = $name;
        $this->values = $values;
    }

    public function getName()
    {
        return $this->name;
    }

    public function getValues()
    {
        return $this->values;
    }
}
services:
    enum.gender:
        class: ConfigurableEnum
        arguments:
            $name: 'member.gender'
            $choices:
                !php/const:AppBundle\Entity\Member::GENDER_MALE: 'Male'
                !php/const:AppBundle\Entity\Member::GENDER_FEMALE: 'Female'
    enum.state:
        class: ConfigurableTranslatedEnum
        arguments:
            $translator: '@translator'
            $transPattern: 'choice.member.state.%s'
            $name: 'member.state'
            $values:
                !php/const:AppBundle\Entity\Member::STATE_NEW
                !php/const:AppBundle\Entity\Member::STATE_VALIDATED
                !php/const:AppBundle\Entity\Member::STATE_DISABLED
  • Keep constants in domain
  • Not needed to create a new class for each enum

This require that Enum::getName exists...

@J-Ben87
Copy link
Contributor

J-Ben87 commented Jan 14, 2018

Always the same question: do we want to manipulate classes or configuration?

To be honest since constants can be used in yaml both ways seem acceptable to me, however I'm afraid we'll loose PHPStorm navigation feature with the configurable option.

Plus keeping constants in domain is not impossible with the current option.

@yann-eugone
Copy link
Collaborator Author

If that question is coming again and again, meens that there is a debate...
Who are we to decide for someone else preferences ?

So I decided to keep the getName method, but I'll add more documentation about usage and examples.

I consider adding the described generic enum to the bundle source code.

@yann-eugone
Copy link
Collaborator Author

yann-eugone commented Feb 13, 2018

So, I'm closing this issue, as #24 got a merge request

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