Countable translation objects

[Matthimatiker]

Introduced countable translation objects.

This BC break allows the usage of translatable objects in Twig in a way that feels more like a real string:

{# Length checks and output of length are possible now #}
{% if translatableObject|length > 5 %}
    { translatableObject|length }}
{% endif %}

{# Empty check works now #}
{% if translatableObject is not empty %}
    {{ translatableObject }}
{% endif %}

This pull request also includes the cleanup changes. The cleanup pull request should be merged first.

[coveralls]

Coverage increased (+2.5%) to 87.097% when pulling 567a54af94366a3516c154398bdf61f3b683319b on feature/countable_translation_objects into 08f427673702146028f57d03010f9ef36d6c69ea on master.

[coveralls]

Coverage increased (+2.5%) to 87.097% when pulling 567a54af94366a3516c154398bdf61f3b683319b on feature/countable_translation_objects into 08f427673702146028f57d03010f9ef36d6c69ea on master.

[mpdude]

I don't really like the use of \Countable here because the TranslatableInterface does not really implement a "countable" thing (a collection). Or, one could expect this to provide the count of translations (languages) available... 😒

On the other hand, I see what you're trying to address.

Either way, I've opened twigphp/Twig#2420 which might also be a way out?

[mpdude]

The ... is empty support clearly is a :+1: for this solution.

I remember discussing something similar a while ago with @dritter, yet I cannot exactly recall what the markup looked like. But it also had to do with testing whether a translatable object actually contained text, so chances are it was the {{ ... is not empty }} case.

[mpdude]

Regarding BC:

IMO, TranslatableInterface is nothing our users should implement. It is an interface they're supposed to use, and in that case (if I am not overlooking something) this change should do them no harm.

So, I would not make this a major version increment with all the pain this brings. Instead, I'd leave a message explaining this in the changelog. Then, focus on documentation explaining what is the intended and stable package API and what are the internal (moving) parts you'd better not touch. This would also give us more manoeuvring space for future changes.

[mpdude]

@MalteWunsch maybe you have an opinion on this as well?

[Matthimatiker]

My thoughts regarding the BC break:

I agree that this change should not affect casual users of the library at all.

However, at least technically this is clearly a BC break that may affect advanced users. In my opinion, as responsible maintainers we are in charge to not allow our users to hurt themselves. A warning is not enough for a change that might lead to fatal errors in PHP.

There might be advanced use cases that are perfectly valid, for example when using decorators:

/* Worked with previous version, does not contain a count() method yet.*/
class MissingTranslationLogger implements TranslatableInterface 
{
    public function __construct(TranslatableInterface $monitoredTranslation)
    {
        // [...]
    }
    // [...|
}

class MyEntity 
{
    /**
     * @ORM\Column(type="string")
     * @Polyglot\Translatable
     * @var TranslatableInterface
     */
    private $text;

    /**
     * @return TranslatableInterface
     */
    public function getText()
    {
        return new MissingTranslationLogger($this->text);
    }
}

Such implementations would break immediately, the hard way.

We failed to mark any class/interface as "api" or "internal", so we are missing the foundation to justify this as a minor change on a business level.

Even if we decided to distinguish between "internal" and "api" interfaces, I am not sure if TranslatableInterface is an eligible candidate for an internal interface: Instances of this interface are returned by entities and user code directly interacts with these instances. Therefore, these implementations are our "official" connection to calling code. It's nothing that is just used internally.

Anyway, for now I would prefer going the safe way by aiming for a new major version 2.0, even if that is (temporarily) harder for us.

[mpdude]

Twigphp/Twig#2420 looks promising.

In the meantime, what if we implement \Countable on ManagedTranslationProxy and Translatable only but do not change the TranslatableInterface? Also, we should put a notice/comment there to remind ourselves that the \Countable implementation is available only to work around the missing feature in Twig...

[Matthimatiker]

Adding Countable only to ManagedTranslationProxy and Translatable makes it "work" somehow. But it implies also, that user code must not rely on this feature as it should be implemented against TranslatableInterface.

In my opinion, from an architecture perspective: Don't do that.

[mpdude]

I think this has been fixed as of Twig 1.33 :-)