Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StringHelper::truncate(null, 10) causes error - userland fix #19736

Closed
cgsmith opened this issue Jan 9, 2023 · 26 comments
Closed

StringHelper::truncate(null, 10) causes error - userland fix #19736

cgsmith opened this issue Jan 9, 2023 · 26 comments
Milestone

Comments

@cgsmith
Copy link
Contributor

cgsmith commented Jan 9, 2023

What steps will reproduce the problem?

Create a StringHelper::truncate() call to a null property. Quick note: I don't think that this is a framework issue and think it is more userland code but wanted to open the issue in case someone else receives the error.

I solved it by wrapping the helper function in a userland check:
image

What is the expected result?

Was expecting an upgrade to 8.1 to go smoothly. I had to change a few things in the user code to make this work. Not sure if documentation should be updated to note this? If not feel free to close this issue.

What do you get instead?

Deprecated: mb_strlen(): Passing null to parameter #1 ($string) of type string is deprecated

Additional info

Error:

yii\base\ErrorException: mb_strlen(): Passing null to parameter #1 ($string) of type string is deprecated in /app/vendor/yiisoft/yii2/helpers/BaseStringHelper.php:132
Q A
Yii version 2.0.47
PHP version 8.1
Operating system Docker yii2 container
@samdark
Copy link
Member

samdark commented Jan 9, 2023

I think we may add a null-check there for convenience. @bizley, @rob006 what do you think?

@rob006
Copy link
Contributor

rob006 commented Jan 9, 2023

What it should return in case of null? For grid/list view returning null would be more practical, but IMO it is weird to return null in methods for string processing.

@cgsmith
Copy link
Contributor Author

cgsmith commented Jan 10, 2023

I do have a draft with a null check but didn't want to do too much work without checking with you. I have some new tests as well written for null passed to ::truncate and the other functions.

Can we set the data type to string on the first parameter for this call and similar calls or will it break B/C?

@bizley
Copy link
Member

bizley commented Jan 10, 2023

If we add null check for convenience , shouldn't we add it everywhere where we denied change because the docs are not allowing null to be passed?

@InsaneSkull
Copy link
Contributor

InsaneSkull commented Jan 10, 2023

What it should return in case of null? For grid/list view returning null would be more practical, but IMO it is weird to return null in methods for string processing.

May be empty string '' as per default method return type.

Not sure about going strict type. truncate expects string parameter.

@cgsmith
Copy link
Contributor Author

cgsmith commented Jan 10, 2023

If truncate expects a string parameter shouldn't we enforce the type on the parameter?

@bizley
Copy link
Member

bizley commented Jan 10, 2023

I would add the null-checks everywhere (since we cannot do the arguments type-forcing), I'm just mentioning that because we used to argue that since the docs are not allowing null (like with StringHelper::truncate) we should not do any checks and make it a developer's issue.

@terabytesoftw
Copy link
Member

Checking for nulls everywhere will make it compatible beyond 8.2, but the docs are based on when they were created, now the language is moving forward, adding more type checking and the framework needs to move along with the language.

@cgsmith
Copy link
Contributor Author

cgsmith commented Jan 10, 2023

I agree that this is a developer issue - I just wanted the issue opened because the error was a little harder to troubleshoot between 8.0 and 8.1.

Curious @bizley why type forcing can't be done on the arguments?

@bizley
Copy link
Member

bizley commented Jan 10, 2023

It's because we are still supporting PHP 5.4.

@rob006
Copy link
Contributor

rob006 commented Jan 10, 2023

now the language is moving forward, adding more type checking and the framework needs to move along with the language.

Accepting null here and silently treating it as empty string is reverting progress made by language. On the other hand checking for null and throwing more verbose exception will add some additional overhead without much of a gain (you will still get an exception, just with different message).

@bizley
Copy link
Member

bizley commented Jan 10, 2023

you will still get an exception, just with different message

I prefer controlled flow.

@lubosdz
Copy link
Contributor

lubosdz commented Jan 10, 2023

Any reason why $string cannot be simply checked inside function ? e.g.
$string = trim($string) or casted $string = (string) $string

@bizley
Copy link
Member

bizley commented Jan 11, 2023

Yes, for cases where an argument should be a string we could do that (and we do in many places). The null check should be for the multi-types arguments.

@wartur
Copy link

wartur commented Jan 11, 2023

I think this is same history in Yii2 Gii ActiveRecords model generator

==================================================Running 'Model Generator'...

PHP Deprecated Warning 'yii\base\ErrorException' with message 'trim(): Passing null to parameter #1 ($string) of type string is deprecated'

in /var/www/html/vendor/yiisoft/yii2/validators/FilterValidator.php:81

Stack trace:
#0 [internal function]: yii\base\ErrorHandler->handleError(8192, 'trim(): Passing...', '/var/www/html/v...', 81)
#1 [internal function]: trim(NULL)
#2 /var/www/html/vendor/yiisoft/yii2/validators/FilterValidator.php(81): call_user_func('trim', NULL)
#3 /var/www/html/vendor/yiisoft/yii2/validators/Validator.php(260): yii\validators\FilterValidator->validateAttribute(Object(yii\gii\generators\model\Generator), 'queryClass')
#4 /var/www/html/vendor/yiisoft/yii2/base/Model.php(368): yii\validators\Validator->validateAttributes(Object(yii\gii\generators\model\Generator), Array)
#5 /var/www/html/vendor/yiisoft/yii2-gii/src/console/GenerateAction.php(35): yii\base\Model->validate()
#6 [internal function]: yii\gii\console\GenerateAction->run()
#7 /var/www/html/vendor/yiisoft/yii2/base/Action.php(93): call_user_func_array(Array, Array)
#8 /var/www/html/vendor/yiisoft/yii2/base/Controller.php(178): yii\base\Action->runWithParams(Array)
#9 /var/www/html/vendor/yiisoft/yii2/console/Controller.php(180): yii\base\Controller->runAction('model', Array)
#10 /var/www/html/vendor/yiisoft/yii2/base/Module.php(552): yii\console\Controller->runAction('model', Array)
#11 /var/www/html/vendor/yiisoft/yii2/console/Application.php(180): yii\base\Module->runAction('gii/model', Array)
#12 /var/www/html/vendor/yiisoft/yii2/base/Controller.php(212): yii\console\Application->runAction('gii/model', Array)
#13 /var/www/html/commands/ExecController.php(94): yii\base\Controller->run('gii/model', Array)
#14 [internal function]: app\commands\ExecController->actionGar()
#15 /var/www/html/vendor/yiisoft/yii2/base/InlineAction.php(57): call_user_func_array(Array, Array)
#16 /var/www/html/vendor/yiisoft/yii2/base/Controller.php(178): yii\base\InlineAction->runWithParams(Array)
#17 /var/www/html/vendor/yiisoft/yii2/console/Controller.php(180): yii\base\Controller->runAction('gar', Array)
#18 /var/www/html/vendor/yiisoft/yii2/base/Module.php(552): yii\console\Controller->runAction('gar', Array)
#19 /var/www/html/vendor/yiisoft/yii2/console/Application.php(180): yii\base\Module->runAction('exec/gar', Array)
#20 /var/www/html/vendor/yiisoft/yii2/console/Application.php(147): yii\console\Application->runAction('exec/gar', Array)
#21 /var/www/html/vendor/yiisoft/yii2/base/Application.php(384): yii\console\Application->handleRequest(Object(yii\console\Request))
#22 /var/www/html/yii(19): yii\base\Application->run()
#23 {main}

@tomaszkane
Copy link

It's because we are still supporting PHP 5.4.

@bizley Why? I think I know but would like to make sure.

@bizley
Copy link
Member

bizley commented May 8, 2023

I think it was a promise made by the original team.

@My6UoT9
Copy link
Contributor

My6UoT9 commented May 8, 2023

https://github.com/yiisoft/yii2#installation

The minimum required PHP version of Yii is PHP 5.4.

@tomaszkane
Copy link

I think it was a promise made by the original team.

OK. Keeping promises is very good. But today 5.4 is dead: https://stitcher.io/blog/php-version-stats-january-2023

Do Yii2 team has any statistic how many installation (updates) is made from hosts with old PHP versions?
Is keeping support for this promile of users worth keeping Yii2 core outdated?

@samdark

@schmunk42
Copy link
Contributor

Some stats: https://packagist.org/packages/yiisoft/yii2/php-stats

If 5.4 is dropped an ie. 7.4 is the new minimum, I'd strongly advocate for a 2.1.0 version.

@lubosdz
Copy link
Contributor

lubosdz commented May 9, 2023

PHP 7.0 might pass tests.

@samdark samdark added this to the 2.0.48 milestone May 9, 2023
@samdark
Copy link
Member

samdark commented May 9, 2023

  1. We can drop PHP 5.4 but I'd make it Yii 2.1 then as @bizley suggested.
  2. Type-forcing for Yii2 might be very troublesome. That's big amount of work and unless either someone performs it and sends a PR or sponsors one of the team memers to do it, I don't think it is worth focusing on.

@samdark
Copy link
Member

samdark commented May 9, 2023

For current code I'd add checks everywhere.

@My6UoT9
Copy link
Contributor

My6UoT9 commented May 10, 2023

As far as I know, With PHP 8.2 and JIT types will improve performance, before that it does not, and in many cases will be slower as it now has to type check. I am not entirely sure if PHP 8.2 brings those benefits.
https://stackoverflow.com/a/32943651/5542121

While I am for making PHP 8.x the minimum required version, its not like PHP 5.4 is not used. But these might be just automated testing systems.
PHP 7 is actually already unsupported https://www.php.net/supported-versions.php
https://w3techs.com/technologies/history_details/pl-php/5

@bizley
Copy link
Member

bizley commented May 10, 2023

Let's continue discussion about 2.1 here #19831

Here let's focus on the issue only please.

@uaoleg
Copy link
Contributor

uaoleg commented May 11, 2023

I did a fix: #19833

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests