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

Merged
merged 3 commits into from Jun 28, 2013

2 participants

@neilime

No description provided.

@grizzm0 grizzm0 commented on the diff Jun 7, 2013
...est/Mvc/View/Console/DefaultRenderingStrategyTest.php
@@ -65,7 +68,16 @@ public function testCanDetachListenersFromEventManager()
public function testIgnoresNonConsoleModelNotContainingResultKeyWhenObtainingResult()
{
- $event = new MvcEvent();
+ //Register console service
@grizzm0
grizzm0 added a note Jun 7, 2013

Use 4 spaces here, not tabs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@grizzm0 grizzm0 commented on an outdated diff Jun 7, 2013
tests/ZendTest/Console/Adapter/AbstractAdapterTest.php
@@ -120,4 +120,28 @@ public function testReadCharWithMaskInsensitiveCase()
$char = $this->adapter->readChar('ar');
$this->assertEquals($char, 'r');
}
+
+ public function testEncodeText(){
@grizzm0
grizzm0 added a note Jun 7, 2013

Missing new line before bracket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@grizzm0 grizzm0 commented on the diff Jun 7, 2013
library/Zend/Console/Adapter/AbstractAdapter.php
@@ -53,7 +53,11 @@
*/
public function write($text, $color = null, $bgColor = null)
{
- if ($color !== null || $bgColor !== null) {
+
+ //Encode text to match console encoding
@grizzm0
grizzm0 added a note Jun 7, 2013

Use space instead of tabs here aswell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on an outdated diff Jun 11, 2013
library/Zend/Console/Adapter/AbstractAdapter.php
@@ -497,4 +501,29 @@ public function readChar($mask = null)
fclose($f);
return $char;
}
+
+ /**
+ * Encode a text to match console encoding
+ *
+ * @param string $text
+ * @return string the encoding text
+ */
+ public function encodeText($text)
+ {
+ $textIsUtf8 = StringUtils::isValidUtf8($text);
@weierophinney
Zend Framework member

Push this to later -- don't execute it unless the isUtf8 flag is false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney
Zend Framework member

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

@weierophinney weierophinney commented on an outdated diff Jun 11, 2013
library/Zend/Console/Adapter/AdapterInterface.php
@@ -246,4 +246,12 @@ public function readLine($maxLength = 2048);
* @return string
*/
public function readChar($mask = null);
+
+ /**
+ * Encode a text to match console encoding
+ *
+ * @param string $text
+ * @return string the encoding text
+ */
+ public function encodeText($text);
@weierophinney
Zend Framework member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on an outdated diff Jun 11, 2013
...ry/Zend/Mvc/View/Console/DefaultRenderingStrategy.php
$response->setContent(
- $response->getContent() . $responseText
+ $console->encodeText($response->getContent() . $responseText)
@weierophinney
Zend Framework member

getContent() should call encodeText(), not vice versa.

@weierophinney
Zend Framework member

Actually... we should test for encodeText(), and, if present, invoke it; otherwise, use the content/response text as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney
Zend Framework member

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
Zend Framework member

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
Zend Framework member

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 weierophinney commented on the diff Jun 28, 2013
...ry/Zend/Mvc/View/Console/DefaultRenderingStrategy.php
// Append console response to response object
- $response->setContent(
- $response->getContent() . $responseText
- );
+ if (is_callable(array($console,'encodeText'))) {
+ $response->setContent(
+ call_user_func(array($console,'encodeText'), $response->getContent() . $responseText)
@weierophinney
Zend Framework member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney added a commit that referenced this pull request Jun 28, 2013
@weierophinney weierophinney [#4606] CS and logic flow fixes
- `s/\t/    /g`
- Reflow logic in Console\DefaultRenderingStrategy to make it easier to
  read, and to re-use common values
e0afc0d
@weierophinney weierophinney added a commit that referenced this pull request Jun 28, 2013
@weierophinney weierophinney Merge branch 'feature/4606' into develop
Close #4606
eaaed78
@weierophinney weierophinney merged commit 72626b2 into zendframework:develop Jun 28, 2013

1 check failed

Details default The Travis CI build failed
@weierophinney
Zend Framework member

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

@weierophinney weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#4606 from neilime/develop
Supports the encoding of the console and encodes the text to display if needed
cb2e475
@weierophinney weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#4606] CS and logic flow fixes
- `s/\t/    /g`
- Reflow logic in Console\DefaultRenderingStrategy to make it easier to
  read, and to re-use common values
bcb4552
@weierophinney weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/4606' into develop 2a7fc79
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment