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

Avoid deprecated call #128

Closed
wants to merge 1 commit into from

Conversation

odolbeau
Copy link

Deprecated: strtoupper(): Passing null to parameter #1 ($string) of type string is deprecated in /var/www/html/vendor/globalcitizen/php-iban/php-iban.php on line 589

@globalcitizen
Copy link
Owner

Hi, thanks for your contribution.

This is a very minimal pull request with insufficient context. What exactly is claimed to be deprecated? Looking at the proposed change, it seems:

  • This is unclear and therefore undesirable syntax.
  • Unwrapping the suggestion, the apparent implication is that running string functions on null inputs is now disallowed. I would need clear evidence of such and a version before taking action.

Remembering that clarity is more important than cleverness.

At a minimum, could you please outline how to reproduce the issue? php version, any non-default settings, small example code, etc.

FWIW here's my test.

$ echo '<?php $x = null; print strtoupper($x) . "\n"; ?>' >>test.php
$ php --version
PHP 8.2.15 (cli) (built: Mar 24 2024 17:50:37) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.15, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.15, Copyright (c), by Zend Technologies
$ php test.php 
$

This executes fine with no errors using a default configuration.

@odolbeau
Copy link
Author

odolbeau commented Mar 24, 2024

Hi, thanks for your response.

I only copy / pasted the deprecation notice provided by PHP because I thought the deprecation message was clear enough : what's deprecated is calling strtoupper with null as argument.
I'll be glad to add more context, don't hesitate to tell me which info you need.

This is unclear and therefore undesirable syntax.

This is your opinion (which I respect) but you're criticising the code of this PR without providing any hint to write it your way. Not very useful.

Unwrapping the suggestion, the apparent implication is that running string functions on null inputs is now disallowed. I would need clear evidence of such and a version before taking action.

Congrats for the unwrap and the very good interpretation. You're (almost perfectly) right : as the deprecation message says, this is now deprecated to run string functions on null inputs (this is not disallowed yet but soon or later, it will).

Here is a very simple reproducer and the PHP RFC introducing those deprecations in 8.1.

@globalcitizen
Copy link
Owner

Ahh OK, so apparently my local default config doesn't care. Yours does.

I will implement a simpler fix like the below and credit you.

if(isnull($country)) { return false; }

Thanks again for bringing this to my attention, I rarely write php these days.

@globalcitizen
Copy link
Owner

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

Successfully merging this pull request may close these issues.

2 participants