Twig integration #34

soullivaneuh · 2016-06-21T15:59:04Z

Add an optional Twig integration to have an extension for enum labels.

Working extension
Extension tests
Bundle service loading
Bundle integration tests
Documentation update (for both AbstractEnum and twig integration)

greg0ire · 2016-06-21T16:19:41Z

src/AbstractEnum.php

@@ -70,6 +72,28 @@
    }

    /**
+     * @param callable|null $callback
+     * @param string|null   $separator


Please document this. What is the callback called on? What arguments should it have? What does the separator separate?

soullivaneuh · 2016-06-22T08:20:45Z

https://travis-ci.org/greg0ire/enum/jobs/139421537#L388

I think I have to review my plugin...

greg0ire · 2016-06-22T08:23:21Z

https://travis-ci.org/greg0ire/enum/jobs/139421537#L388

I think I have to review my plugin...

O_o indeed…

soullivaneuh · 2016-06-22T08:24:34Z

Or... I have to get new eyes.

diff --git a/composer.json b/composer.json
index 9eb6c0c..337b589 100644
--- a/composer.json
+++ b/composer.json
@@ -17,8 +17,8 @@
     "require-dev": {
         "phpunit/phpunit": "^4.1",
         "sllh/php-cs-fixer-styleci-bridge": "^2.0",
-        "symfony/config": " ^2.7 || ^3.0",
-        "symfony/dependency-injection": " ^2.7 || ^3.0",
+        "symfony/config": "^2.7 || ^3.0",
+        "symfony/dependency-injection": "^2.7 || ^3.0",
         "symfony/form": "^2.7 || ^3.0",
         "symfony/http-kernel": "^2.7 || ^3.0",
         "symfony/twig-bundle": "^2.7 || ^3.0",

soullivaneuh · 2016-06-22T09:42:12Z

src/AbstractEnum.php

     *
     * @return array a hash with your constants and their value. Useful for
     *               building a choice widget
     */
-    final public static function getConstants($keysCallback = null)
+    final public static function getConstants($keysCallback = null, $classPrefixed = false, $namespaceSeparator = '_')


Maybe $classPrefixed can be true by default?

For most of the cases, translator will be used for a proper label format. Using class prefix make more sense IMHO.

Most of the key, translator will be used for a proper format. Using class prefix make more sense IMHO.

Can you proofread your sentence ? It looks strange…

key -> cases.

Don't ask me why, I don't know.

soullivaneuh · 2016-06-22T09:43:11Z

Not sure about that, but maybe we can make a enum_label_trans filter. A combination of both enum_label and trans workflow.

What do you think?

greg0ire · 2016-06-22T09:44:04Z

What do you think?

I was just thinking about that :) !

soullivaneuh · 2016-06-22T09:45:52Z

In this case, we have to consider this before: #34 (comment)

greg0ire · 2016-06-22T09:49:09Z

In this case, we have to consider this before: #34 (comment)

Not sure about that: Here is what would be best IMO : enum_trans(object, fieldName)

The class can be retrieved from the object, and used to build the catalog name for trans. So no need for a class prefix IMO.

soullivaneuh · 2016-06-22T09:53:03Z

And how the filter is supposed to know which enum is used? It will just get a value...

and used to build the catalog name for trans. So no need for a class prefix IMO.

I don't agree, in this case you don't let the choice of the translations keys.

This is also why I added a separator option.

greg0ire · 2016-06-22T09:56:01Z

And how the filter is supposed to know which enum is used?

This is why I'm recommending a function with two arguments instead of a filter

and used to build the catalog name for trans. So no need for a class prefix IMO.

Maybe this could be the default, while a third argument would provide the ability to change that ? And let's drop the trans suffix, translation might not be enabled on the project

enum(object, fieldname [, catalog])

soullivaneuh · 2016-06-22T10:00:27Z

This is why I'm recommending a function with two arguments instead of a filter

Example:

class OrganizationMember
{
    private $accessLevel = OrganizationAccessLevel::MEMBER;
}

And the related enum:

use Greg0ire\Enum\AbstractEnum;

final class OrganizationAccessLevel extends AbstractEnum
{
    const MEMBER = 500;
    const OWNER = 1000;
}

With your method, object will be a OrganizationMember and the value could be 500 or 1000.

So tell me how the filter, with an int as value will know this is related to OrganizationAccessLevel enum. 😉

Maybe this could be the default, while a third argument would provide the ability to change that ?

Already the case with the separator argument.

And let's drop the trans suffix, translation might not be enabled on the project

And what happen if the translator is enabled, the related key exist but the user does not want to translate it? Note: Translator always have a default catalog.

greg0ire · 2016-06-22T10:01:51Z

the filter

The function. With 2 arguments. There won't be a filter in my mind. You won't pass the int to it, you will pass the object and the field name (not its value)

greg0ire · 2016-06-22T10:16:07Z

Until #37 is done, the function should actually have one more argument: the enum class name

soullivaneuh · 2016-06-22T10:16:15Z

Function or filter, the argument is the same: how the filter, with an int as value will know this is related to OrganizationAccessLevel enum?

And for me a filter makes more sens because of transformation. The translator is a filter BTW.

But again, we can do both and let the end user choose. 😉

greg0ire · 2016-06-22T10:17:16Z

Yeah sorry I was confusing both classes, see my last comment.

greg0ire · 2016-06-22T17:04:22Z

README.md

@@ -194,6 +204,53 @@ $view = $this->factory->create(EnumType::class, null, array(
 ))->createView();
 ```

+#### Twig extension
+
+This package come with a `enum_label` filter, available on the `EnumExtension` Twig class.


come => comes
a => an
on => thanks to

soullivaneuh · 2016-06-22T18:21:44Z

@greg0ire Review fixed!

soullivaneuh · 2016-06-22T20:36:13Z

PR rebased and conflicts fixed.

greg0ire · 2016-06-22T21:38:45Z

src/Bridge/Twig/Extension/EnumExtension.php

+     *                                       - 'ø': I want 'ø' as a separator, which implies to prefix with a class.
+     *                                       - false: I don't want any separator, because I don't even need a prefix.
+     *                                       By default, the value is the configured one if the translator
+     *                                       is available and enabled, false otherwise.


After more consideration, this multi value argument does not look like such a good idea, sorry. Please split it back into two arguments ($classPrefix, boolean, and $separator).

soullivaneuh · 2016-06-23T07:55:04Z

@greg0ire Done.

soullivaneuh · 2016-06-23T07:55:30Z

Wait a little, I forgot the documentation.

greg0ire · 2016-06-23T07:55:44Z

Can you please rebase @soullivaneuh ?

greg0ire · 2016-06-23T07:56:49Z

src/Bridge/Twig/Extension/EnumExtension.php

+     *                                        string: Use the specified one
+     *                                        null: Use the default one
+     *                                        false: Do not use the translator
+     * @param bool        $classPrefixed      Prefix the label with the enum class. Default to true if the translator


"Default" => "Defaults"

The goal is to extract the class prefix logic from the form type to be used elsewhere.

greg0ire · 2016-06-23T07:57:29Z

src/Bridge/Twig/Extension/EnumExtension.php

+            return $translatedLabel ?: $label;
+        }
+
+        return $label;


This is way easier to understand @soullivaneuh , good job! 👏

Add an optional Twig integration to have an extension for enum labels.

greg0ire · 2016-06-23T07:59:40Z

src/Bridge/Twig/Extension/EnumExtension.php

+    {
+        // Determine if the translator can be used or not.
+        $useTranslation = $this->translator instanceof TranslatorInterface
+            && (is_null($translationDomain) || is_string($translationDomain));


Linebreaking at the || instead would be more logical, wouldn't it?

$useTranslation = $this->translator instanceof TranslatorInterface && (is_null($translationDomain) || is_string($translationDomain));

Honestly? No. The is_null and is_string are linked together for one conditon part.

This is harder to read with your way IMHO.

Oh sorry, you're actually right, didn't see the parenthesis

soullivaneuh · 2016-06-23T07:59:47Z

PR rebased and documentation updated, ready to go! 👍

greg0ire · 2016-06-23T08:06:56Z

RTM, let's wait for Travis.

greg0ire · 2016-06-23T08:12:31Z

Thanks @soullivaneuh !!!

soullivaneuh · 2016-06-23T08:29:13Z

You're welcome! Any chance for a release? :-)

greg0ire · 2016-06-23T08:35:47Z

Let me merge remaining PRs first, I'll release just after that.

soullivaneuh force-pushed the twig branch from ad8c478 to a88474c Compare June 21, 2016 15:59

greg0ire reviewed Jun 21, 2016
View reviewed changes

soullivaneuh force-pushed the twig branch 3 times, most recently from 4116aaa to c4696da Compare June 22, 2016 08:05

soullivaneuh force-pushed the twig branch 4 times, most recently from cf0e91d to b60793a Compare June 22, 2016 09:36

soullivaneuh reviewed Jun 22, 2016
View reviewed changes

soullivaneuh force-pushed the twig branch from b60793a to 198a78d Compare June 22, 2016 09:55

soullivaneuh mentioned this pull request Jun 22, 2016

Enum mapping aware service #37

Open

greg0ire reviewed Jun 22, 2016
View reviewed changes

soullivaneuh force-pushed the twig branch 2 times, most recently from 8d61ba8 to afbc75f Compare June 22, 2016 18:21

soullivaneuh force-pushed the twig branch from afbc75f to 94f2dac Compare June 22, 2016 20:35

greg0ire reviewed Jun 22, 2016
View reviewed changes

soullivaneuh force-pushed the twig branch from 94f2dac to ef42e8d Compare June 23, 2016 07:54

greg0ire reviewed Jun 23, 2016
View reviewed changes

Introduce AbstractEnum::getClassPrefixedKeys

b78bc42

The goal is to extract the class prefix logic from the form type to be used elsewhere.

greg0ire reviewed Jun 23, 2016
View reviewed changes

Twig integration

34dffca

Add an optional Twig integration to have an extension for enum labels.

soullivaneuh force-pushed the twig branch from ef42e8d to 34dffca Compare June 23, 2016 07:59

greg0ire reviewed Jun 23, 2016
View reviewed changes

greg0ire merged commit 0dd3b3a into greg0ire:master Jun 23, 2016

soullivaneuh deleted the twig branch June 23, 2016 08:28

Twig integration #34

Twig integration #34

Conversation

soullivaneuh commented Jun 21, 2016 • edited Loading

Choose a reason for hiding this comment

soullivaneuh commented Jun 22, 2016

greg0ire commented Jun 22, 2016

soullivaneuh commented Jun 22, 2016 • edited Loading

soullivaneuh Jun 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soullivaneuh commented Jun 22, 2016

greg0ire commented Jun 22, 2016

soullivaneuh commented Jun 22, 2016

greg0ire commented Jun 22, 2016

soullivaneuh commented Jun 22, 2016

greg0ire commented Jun 22, 2016

soullivaneuh commented Jun 22, 2016

greg0ire commented Jun 22, 2016 • edited Loading

greg0ire commented Jun 22, 2016

soullivaneuh commented Jun 22, 2016

greg0ire commented Jun 22, 2016

Choose a reason for hiding this comment

soullivaneuh commented Jun 22, 2016

soullivaneuh commented Jun 22, 2016

Choose a reason for hiding this comment

soullivaneuh commented Jun 23, 2016

soullivaneuh commented Jun 23, 2016

greg0ire commented Jun 23, 2016

Choose a reason for hiding this comment

greg0ire Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

greg0ire Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soullivaneuh commented Jun 23, 2016 • edited Loading

greg0ire commented Jun 23, 2016

greg0ire commented Jun 23, 2016

soullivaneuh commented Jun 23, 2016

greg0ire commented Jun 23, 2016

soullivaneuh commented Jun 21, 2016 •

edited

Loading

soullivaneuh commented Jun 22, 2016 •

edited

Loading

soullivaneuh Jun 22, 2016 •

edited

Loading

greg0ire commented Jun 22, 2016 •

edited

Loading

greg0ire Jun 23, 2016 •

edited

Loading

greg0ire Jun 23, 2016 •

edited

Loading

soullivaneuh commented Jun 23, 2016 •

edited

Loading