fixed string helper encoding when Yii::$app is not available #13263

cebe · 2016-12-21T22:48:26Z

Q	A
Is bugfix?	yes
New feature?	no
Breaks BC?	no
Tests pass?	let travis decide on this one
Fixed issues	#13254

fixes #13254

yii-bot · 2016-12-21T22:52:48Z

Thank you for putting effort in the improvement of the Yii framework.
We have reviewed your pull request.

In order for the framework and your solution to remain stable in the future, we have a unit test requirement in place. Therefore we can only accept your pull request if it is covered by unit tests.

Could you add these please?

Thanks!

P.S. If you have any questions about the creation of unit tests? Don't hesitate to ask for support. More information about unit tests

This is an automated comment, triggered by adding the label pr:request for unit tests.

klimov-paul · 2016-12-22T09:00:20Z

See #13254 (comment)

We should either fix all the dependencies on Yii application around the helper classes and validators or leave everything be as it is.
Why should we fix StringValidator to work without an application instance, while leaving FileHelper depending on it?

alexraputa · 2016-12-22T09:09:45Z

We should either fix all the dependencies on Yii application around the helper classes and validators or leave everything be as it is.

Sounds good. This would make development more flexible.

dynasource · 2016-12-31T13:32:36Z

IMO all dependencies on an active Yii application should be minimized in the helpers. For three reasons:

Static behavior: Helpers are static and focussed on processing instead of depending on global state. You are expecting loosely tied helper methods
Testing: enough scenarios are thinkable in a PHP unit environment in which you are testing something in which you don't need an Application to be active
Use of Yii helpers in non Yii applications

dynasource · 2016-12-31T13:33:56Z

framework/helpers/BaseStringHelper.php

@@ -104,12 +104,15 @@ public static function dirname($path)
     */
    public static function truncate($string, $length, $suffix = '...', $encoding = null, $asHtml = false)
    {
+        if ($encoding === null) {
+            $encoding = Yii::$app ? Yii::$app->charset : 'UTF-8';


shouldnt this be put once in an ApplicationHelper?

or Yii::encoding().

what exactly and why?

A helper to replace a simple ternary operator check?

I don't think a new helper is needed but that ternary operator seem to happen a lot. How about a new method?

StringHelper::getEncoding() or Yii::encoding()

Standalone valdator and helper running will become impossible in case you need default encoding different from UTF-8.

For the helper it is no problem as long as you specify the $encoding param of the method.
For the validator we could add a property that defaults to Yii::$app->charset or 'UTF-8' if not set.

For the helper it is no problem as long as you specify the $encoding param of the method.

You can do it now, with current code, and thus avoid coupling with Application. Following such argument - no change is needed at all.

Yii::$app should ideally be removed out of this helper

I'm for Yii::ENCODING constant or just leaving UTF-8 string in ternary check. This approach is already used in multiple places in core. Symfony uses UTF-8 as "magic string", and Laravel don't even care about different encodings and have hardcoded UTF-8 in multiple places. It is not worth complicating this only for extremely edge cases.

Leaving a string there sounds OK to me.

making StringHelper and StringValidator independent of Yii::$app fixes #13254

cebe · 2017-04-25T21:16:04Z

rebased and made sure tests cover it by forcing Yii::$app to be null in test cases. This should be done for more helpers, will prepare a separate PR for that.

klimov-paul · 2017-04-26T09:55:39Z

framework/helpers/BaseStringHelper.php

@@ -104,12 +104,15 @@ public static function dirname($path)
     */
    public static function truncate($string, $length, $suffix = '...', $encoding = null, $asHtml = false)
    {
+        if ($encoding === null) {
+            $encoding = Yii::$app ? Yii::$app->charset : 'UTF-8';


Usage of ternary operator introduces a "magic string" - UTF-8.
Keeping default encoding at Application field is a one thing, but put putting same string constant all over the code is a different one.
While in current state UTF-8 is covering most of the needs and is de-facto standard it may change in the future. After several years we may use UTF-16 for example.

We should either introduce a static level for default encoding (e.g. Yii::$encoding or Yii::encoding() or Yii::$defaultEncoding) or abandon the whole idea.

tom-- · 2017-09-06T21:47:57Z

framework/helpers/BaseStringHelper.php

+        if (mb_strlen($string, $encoding) > $length) {
+            return rtrim(mb_substr($string, 0, $length, $encoding)) . $suffix;
+        } else {
+            return $string;


What do the above two lines do?

If string length is more than limit defined in $length it trims string to the length specified, then adding $suffix.

It's the two new lines

} else { return $string;

that i think need explanation. The next line after the else {} is return $string;

tom-- · 2017-09-06T21:55:25Z

When there is no framework "global context" (Yii::$app) where the default or user configuration can go, I would expect the framework to get out of the way rather than force any hardcoded value. Instead, I would expect PHP's INI directive default_charset to apply.

samdark · 2017-09-06T21:58:47Z

Could be a good idea. @cebe what do you think?

cebe · 2017-09-06T22:10:26Z

default_charset is PHP 5.6.0, so that would be the way to go in 2.1.

sergeymakinen · 2017-09-06T22:42:28Z

As we discuss a framework helper, I'd like to implement both @tom-- & @klimov-paul ideas: StringHelper::encoding() which returns Yii::$app->charset / ini_get('default_charset') / 'UTF-8' depending on a context and I'm fine with it.

dynasource · 2017-09-07T07:27:37Z

tests/framework/helpers/StringHelperTest.php

-        $this->mockApplication();
+
+        // destroy application, Helper must work without Yii::$app
+        $this->destroyApplication();


shouldnt we extend from a PhpUnit base class in which no application is setup?

@dynasource yiiunit\TestCase dose not mock application by default. destroyApplication() is used only to make sure that Yii::$app is not setup as leftovers after previous tests.

dynasource · 2017-09-07T07:29:34Z

I agree. The encoding logic should be taken out.

rob006

I'm for merging this as is. Yii::encoding() could be added in another PR, there is no sense to block this one.

samdark · 2017-09-07T19:32:38Z

Merged. Thanks @cebe for working on it and everyone for reviews and suggestions.

cebe added the type:bug Bug label Dec 21, 2016

cebe added this to the 2.0.12 milestone Dec 21, 2016

cebe added the pr:request for unit tests Unit tests are needed. label Dec 21, 2016

dynasource reviewed Dec 31, 2016

View reviewed changes

samdark requested review from samdark, dynasource, klimov-paul, SilverFire and arogachev April 25, 2017 13:18

samdark approved these changes Apr 25, 2017

View reviewed changes

fixed string helper encoding when Yii::$app is not available

74d2d02

making StringHelper and StringValidator independent of Yii::$app fixes #13254

cebe force-pushed the stringhelper-encoding branch from 0cdb073 to 74d2d02 Compare April 25, 2017 21:15

cebe removed the pr:request for unit tests Unit tests are needed. label Apr 25, 2017

cebe mentioned this pull request Apr 25, 2017

Helper independent #14050

Merged

klimov-paul suggested changes Apr 26, 2017

View reviewed changes

cebe added the status:under discussion label Apr 26, 2017

cebe modified the milestones: 2.0.13, 2.0.12 Apr 26, 2017

cebe added type:enhancement and removed type:bug Bug labels Apr 26, 2017

samdark added 2 commits September 6, 2017 23:16

Merge branch 'master' into stringhelper-encoding

6e3698a

Adjusted changelog [skip ci]

86ff9ce

samdark requested review from schmunk42, sergeymakinen and tom-- September 6, 2017 21:28

samdark requested review from Faryshta and dmirogin September 6, 2017 21:28

tom-- reviewed Sep 6, 2017

View reviewed changes

dynasource reviewed Sep 7, 2017

View reviewed changes

tom-- and others added 2 commits September 7, 2017 14:30

excise non-functional code

b7a5c57

Update CHANGELOG [skip ci]

842b1e2

rob006 approved these changes Sep 7, 2017

View reviewed changes

samdark removed the status:under discussion label Sep 7, 2017

samdark merged commit 36b535c into master Sep 7, 2017

samdark deleted the stringhelper-encoding branch September 7, 2017 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed string helper encoding when Yii::$app is not available #13263

fixed string helper encoding when Yii::$app is not available #13263

cebe commented Dec 21, 2016 •

edited

Loading

yii-bot commented Dec 21, 2016

klimov-paul commented Dec 22, 2016

alexraputa commented Dec 22, 2016

dynasource commented Dec 31, 2016

dynasource Dec 31, 2016

Faryshta Apr 25, 2017

cebe Apr 25, 2017

cebe Apr 25, 2017

Faryshta Apr 25, 2017 •

edited

Loading

cebe Apr 26, 2017

klimov-paul Apr 26, 2017

dynasource May 5, 2017

rob006 Sep 6, 2017

samdark Sep 6, 2017

cebe commented Apr 25, 2017

klimov-paul Apr 26, 2017

tom-- Sep 6, 2017

samdark Sep 6, 2017 •

edited

Loading

tom-- Sep 6, 2017

tom-- commented Sep 6, 2017

samdark commented Sep 6, 2017

cebe commented Sep 6, 2017

sergeymakinen commented Sep 6, 2017

dynasource Sep 7, 2017

rob006 Sep 7, 2017

dynasource commented Sep 7, 2017

rob006 left a comment

samdark commented Sep 7, 2017

fixed string helper encoding when Yii::$app is not available #13263

fixed string helper encoding when Yii::$app is not available #13263

Conversation

cebe commented Dec 21, 2016 • edited Loading

yii-bot commented Dec 21, 2016

klimov-paul commented Dec 22, 2016

alexraputa commented Dec 22, 2016

dynasource commented Dec 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Faryshta Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cebe commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdark Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-- commented Sep 6, 2017

samdark commented Sep 6, 2017

cebe commented Sep 6, 2017

sergeymakinen commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dynasource commented Sep 7, 2017

rob006 left a comment

Choose a reason for hiding this comment

samdark commented Sep 7, 2017

cebe commented Dec 21, 2016 •

edited

Loading

Faryshta Apr 25, 2017 •

edited

Loading

samdark Sep 6, 2017 •

edited

Loading