Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

Merged
merged 3 commits into from

3 participants

@neilime

No description provided.

@grizzm0 grizzm0 commented on the diff
...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

Use 4 spaces here, not tabs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

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
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

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
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 Owner

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

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

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 Owner

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
...ry/Zend/Mvc/View/Console/DefaultRenderingStrategy.php
((12 lines not shown))
$response->setContent(
- $response->getContent() . $responseText
+ $console->encodeText($response->getContent() . $responseText)
@weierophinney Owner

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

@weierophinney Owner

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

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 weierophinney commented on the diff
...ry/Zend/Mvc/View/Console/DefaultRenderingStrategy.php
((15 lines not shown))
// 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 Owner

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 referenced this pull request from a commit
@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 merged commit 72626b2 into zendframework:develop
@weierophinney

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

@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-console
@weierophinney weierophinney Merge pull request zendframework/zf2#4606 from neilime/develop
Supports the encoding of the console and encodes the text to display if needed
cb2e475
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-console
@weierophinney weierophinney [zendframework/zf2#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 referenced this pull request from a commit in zendframework/zend-console
@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
Commits on Jun 7, 2013
  1. @neilime
Commits on Jun 13, 2013
  1. @neilime
Commits on Jun 19, 2013
  1. @neilime

    Fix wrong indentation

    neilime authored
This page is out of date. Refresh to see the latest.
View
29 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

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
+ $text = $this->encodeText($text);
+
+ if ($color !== null || $bgColor !== null) {
echo $this->colorize($text, $color, $bgColor);
} else {
echo $text;
@@ -497,4 +501,27 @@ 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)
+ {
+ if ($this->isUtf8()) {
+ if (StringUtils::isValidUtf8($text)) {
+ return $text;
+ }
+
+ return utf8_encode($text);
+ }
+
+ if (StringUtils::isValidUtf8($text)) {
+ return utf8_decode($text);
+ }
+
+ return $text;
+ }
}
View
23 library/Zend/Mvc/View/Console/DefaultRenderingStrategy.php
@@ -59,17 +59,22 @@ public function render(MvcEvent $e)
}
}
- // Fetch result from primary model
- if ($result instanceof ConsoleViewModel) {
- $responseText .= $result->getResult();
- } else {
- $responseText .= $result->getVariable(ConsoleViewModel::RESULT);
- }
+ // Fetch service manager
+ $sm = $e->getApplication()->getServiceManager();
+
+ // Fetch console
+ $console = $sm->get('console');
// 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 Owner

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
+ );
+ } else {
+ $response->setContent(
+ $response->getContent() . $responseText
+ );
+ }
// Pass on console-specific options
if ($response instanceof ConsoleResponse
View
26 tests/ZendTest/Console/Adapter/AbstractAdapterTest.php
@@ -7,7 +7,7 @@
* @license http://framework.zend.com/license/new-bsd New BSD License
*/
-namespace ZendTest\Console\Adapater;
+namespace ZendTest\Console\Adapter;
use ZendTest\Console\TestAssets\ConsoleAdapter;
@@ -120,4 +120,28 @@ public function testReadCharWithMaskInsensitiveCase()
$char = $this->adapter->readChar('ar');
$this->assertEquals($char, 'r');
}
+
+ public function testEncodeText()
+ {
+ //Utf8 string
+ $text = '\u00E9\u00E9\u00E9';
+
+ //Console UTF8 - Text utf8
+ $this->adapter->setTestUtf8(true);
+ $encodedText = $this->adapter->encodeText($text);
+ $this->assertEquals($text,$encodedText);
+
+ //Console UTF8 - Text not utf8
+ $encodedText = $this->adapter->encodeText(utf8_decode($text));
+ $this->assertEquals($text,$encodedText);
+
+ //Console not UTF8 - Text utf8
+ $this->adapter->setTestUtf8(false);
+ $encodedText = $this->adapter->encodeText($text);
+ $this->assertEquals(utf8_decode($text),$encodedText);
+
+ //Console not UTF8 - Text not utf8
+ $encodedText = $this->adapter->encodeText(utf8_decode($text));
+ $this->assertEquals(utf8_decode($text),$encodedText);
+ }
}
View
14 tests/ZendTest/Mvc/View/Console/DefaultRenderingStrategyTest.php
@@ -15,8 +15,11 @@
use Zend\EventManager\EventManager;
use Zend\Mvc\MvcEvent;
use Zend\Mvc\View\Console\DefaultRenderingStrategy;
+use Zend\ServiceManager\ServiceManager;
use Zend\Stdlib\Response;
use Zend\View\Model;
+use ZendTest\Console\TestAssets\ConsoleAdapter;
+use ZendTest\ModuleManager\TestAsset\MockApplication;
/**
* @category Zend
@@ -65,7 +68,16 @@ public function testCanDetachListenersFromEventManager()
public function testIgnoresNonConsoleModelNotContainingResultKeyWhenObtainingResult()
{
- $event = new MvcEvent();
+ //Register console service
@grizzm0
grizzm0 added a note

Use 4 spaces here, not tabs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $sm = new ServiceManager();
+ $sm->setService('console', new ConsoleAdapter());
+
+ $mockApplication = new MockApplication;
+ $mockApplication->setServiceManager($sm);
+
+ $event = new MvcEvent();
+ $event->setApplication($mockApplication);
+
$model = new Model\ViewModel(array('content' => 'Page not found'));
$response = new Response();
$event->setResult($model);
Something went wrong with that request. Please try again.