Supports the encoding of the console and encodes the text to display if needed

[neilime]

No description provided.

[grizzm0]

Use 4 spaces here, not tabs

[weierophinney]

This also applies to master; I will cherry-pick when merging so that we can get it in both branches.

[weierophinney]

This is a no-go, as changing an interface creates a backwards compatibility break. Considering that it's an implementation detail, we can omit it from the interface.

[weierophinney]

In looking at this, I have reservations about it as implemented:

• First, CS. Please read the contributor guidelines, and follow the CS rules. While php-cs-fixer can fix some of it, I worry about losing intent as we clean it up to fit the CS.
• Second: when you return from a condition, you don't need an elseif or else block. And avoid storing results of methods that return booleans that you later use in conditionals -- just call the method inside the condition. Essentially: look at your logic flows and break them up to make them more readable and to return early whenever possible.
• Third: adding or altering an interface breaks backwards compatibility. Full stop. There are a few ways around this.
  • First, if you can, try and arrange it so that no interface addition is even needed. Look at the normal flow and see if there's another route you can take.
  • If not, create a new interface defining the encodeText() method, and have the AbstractAdapter implement it. Then test for it before trying to call it. This allows graceful degradation for those who already have custom implementations.
  • Alternately, use duck typing (i.e., method_exists()) to do the same.

Make the above changes, push them to your branch, and I'll review again.

[neilime]

I'm ok with your first and third points. For the second point, do you think that the code below is better ?

    /**
     * Encode a text to match console encoding
     *
     * @param  string $text
     * @return string the encoding text
     */
    public function encodeText($text)
    {
        if ($this->isUtf8()) {

            $textIsUtf8 = StringUtils::isValidUtf8($text);

            if ($textIsUtf8) {
                return $text;
            } else {
                return utf8_encode($text);
            }
        }

        $textIsUtf8 = StringUtils::isValidUtf8($text);

        if ($textIsUtf8) {
            return utf8_decode($text);
        } else {
            return $text;
        }
    }

[weierophinney]

do you think that the code below is better ?

Yes and No. :)

I missed that you were using the value within the first condition. As such, caching the value kind of makes sense. However, it's just as readable, if not more, to call it when needed.

Try this:

    /**
     * Encode a text to match console encoding
     *
     * @param  string $text
     * @return string the encoding text
     */
    public function encodeText($text)
    {
        if ($this->isUtf8()) {
            if (StringUtils::isValidUtf8($text)) {
                return $text;
            }
            return utf8_encode($text);
        }

        if (StringUtils::isValidUtf8($text)) {
            return utf8_decode($text);
        }

        return $text;
    } 

Note the removal of the else/elseif blocks -- you're returning from the previous condition, so why define them for code that will never consider them? That's the main bit I was getting at.

With regards to caching the value -- that means that when I hit the condition, I have to backtrack to where the variable was declared to see what it means and how it was calculated. Using the method call directly in the condition makes this more clear -- and combined with the way the conditionals are written, it will only be executed when that conditional is tested.

[neilime]

Ok for the function encodeText (thanks for explanation).

I've got a question about your note "getContent() should call encodeText(), not vice versa.". Does it mean that you want that Zend\Console\Response::getContent should call encodeText ?
I think that's not a good solution, because :

• This implies that Zend\Console\Response has the Console object.
• The content will be encoded everytime getContent will be called.

Am I Wrong ?

[weierophinney]

I've got a question about your note "getContent() should call encodeText(), not vice versa.". Does it mean that you want that Zend\Console\Response::getContent should call encodeText ?

I backpedaled on that one in a later response. :) Use either a new interface or ducktyping to determine if you can call encodeText(), and only call it if the implementation allows it. Otherwise, use the original routine.

[weierophinney]

You can simplify this to $console->encodeText($response->getContent() . $responseText).

[weierophinney]

I incorporated feedback when merging, including replacing all tabs with spaces.