search

Azure / azure-storage-php

Microsoft Azure Storage Library for PHP
MIT License
217 stars 198 forks source link

Fix deprecation warning in PHP8.1 #341

Closed Nek- closed 1 year ago

Nek- commented 1 year ago

src/Common/Internal/ServiceRestProxy.php:80 may call this function with null, the line return (substr($haystack, -$length) === $needle); is an issue because PHP 8.1 deprecated the use of substr on null.

sicet7 commented 1 year ago

@Nek-

Returning an empty string from a method that should return boolean seems kind of risky. Would it not be simpler to replace: return (substr($haystack, -$length) === $needle); With: return (substr($haystack ?? '', -$length) === $needle);

and then doing the same null coalescing for the strtolower call above to preserve the same functionality??

EDIT: And in the case where you want to keep PHP 5.6 compatibility you can simply use the ternary operator instead of null coalescing

Nek- commented 1 year ago

Hello, I fully agree with you but it's the current behavior of the function. My fix is not designed to make any behavior change.

sicet7 commented 1 year ago

@Nek- if current behavior should be preserved your return should be a boolean not a string. The line return (substr($haystack, -$length) === $needle); always returns a boolean which also is the type specified in the docBlock.

Nek- commented 1 year ago

Indeed, I don't know why I ended up with this fix. It's just wrong. Nice catch.

spaze commented 1 year ago

The doc block also specifies that $haystack is not nullable string so passing null should be invalid. Maybe it would be better to fix the caller to not call the method with null.

Nek- commented 1 year ago

@spaze about this however I was right because it happens when running the client.

You should add phpstan or psalm.

spaze commented 1 year ago

@spaze about this however I was right because it happens when running the client.

That's correct, but I'm not talking about the bug, I've suggested maybe a more correct fix :-)

The bug indeed seems to surface when you're creating the TableRestProxy object directly by calling its __construct(). I'm using TableRestProxy::createTableService() and that's working fine so that might be a workaround for you too.

You should add phpstan or psalm.

I agree that running a static analyzer would help the lib tremendously but I'm not a maintainer so I can't add anything :-)