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

PHP Deprecated: strlen(): Passing null to parameter #1 #32

Closed
retry-the-user opened this issue Apr 18, 2023 · 6 comments · Fixed by #33
Closed

PHP Deprecated: strlen(): Passing null to parameter #1 #32

retry-the-user opened this issue Apr 18, 2023 · 6 comments · Fixed by #33

Comments

@retry-the-user
Copy link

I am currently evaluating php 8.2 upgrade from 8.1 . I decided to turn on deprecation errors this time. I am getting this error a lot with the checkbox element. I exclusively use advcheckbox and the most common form is like so:

$qf->addElement('advcheckbox', 'redo_sha1', 'Redo SHA1');

which leads to errors like so
Got error 'PHP message: PHP Deprecated: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in /var/www/project/lib/qf/HTML/QuickForm/checkbox.php on line 79

If I add an empty string as the 4th parameter, the error goes away:
$qf->addElement('advcheckbox', 'redo_sha1', 'Redo SHA1', '');

But maybe the default for the optional parameter should just be an empty string and not a null since it's going to get strlen() applied no matter what? I think you fixed a similar problem in an earlier commit with an explicit string cast in the radio.php file, but that would not cover the checkbox.php . I think the same solution will cover it.

However, I think the root of the problem is actually in the constructors using null for defaults that should probably be empty strings instead.

checkbox constructor:
public function __construct($elementName=null, $elementLabel=null, $text='', $attributes=null)

advcheckbox constructor:
public function __construct($elementName=null, $elementLabel=null, $text=null, $attributes=null, $values=null)

radio constructor:
public function __construct($elementName=null, $elementLabel=null, $text=null, $value=null, $attributes=null)

I don't know why the checkbox constructor already uses '' for $text while the others do not. I can see that a lot of tests for null happen (at least for $value) so setting that to default to '' might cause regressions. But I think it is probably safe to use '' for $text in these 3 element constructors at least.

@flack
Copy link
Collaborator

flack commented Apr 19, 2023

Hmm, yeah, backwards compatibility is going to be a bit hairy on this one...

But I've looked over all the null checks in the repo and AFAICT there's nothing that works on those text fields (most are related to values, where null actually makes sense). I've created #33 which should fix all the strlen-related issues (strangely, I don't see them on my machine), can you check if that works for you?

@retry-the-user
Copy link
Author

Can you also apply changes to the advcheckbox constructor? That's actually the one triggering all my QF errors. Overall it's pretty annoying that the php devs decided to stop just casting null to '' in string functions. I had to extend and override simpleXML just to stop setAttribute() from throwing errors on null by inserting a check for null and cast to string.

@retry-the-user
Copy link
Author

Hmm how do I download your latest changes before they're committed into a new release?

@flack
Copy link
Collaborator

flack commented Apr 19, 2023

Ah, sorry, must have overlooked that. I just pushed a second commit, that should hopefully be everything then.

And yes, it is a bit annoying that strlen is not ?string $input, at least for old code..

@flack
Copy link
Collaborator

flack commented Apr 19, 2023

re installing: Easiest would probably be to clone the repo, checkout the issue32 branch and put that in your vendor folder

@flack
Copy link
Collaborator

flack commented Apr 19, 2023

or you point your composer.json to the branch like here: https://lornajane.net/posts/2014/use-a-github-branch-as-a-composer-dependency

@flack flack closed this as completed in #33 Apr 20, 2023
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

Successfully merging a pull request may close this issue.

2 participants