Azure / azure-storage-php

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

Check $copyProgress is not null before using it in strpos on PHP 8.1 #321

Closed spaze closed 2 years ago

spaze commented 2 years ago

Passing nulls to string params is deprecated in PHP 8.1.

Fix #320

pimjansen commented 2 years ago

Any update on this? PHP 8.1 is release almost a month ago as stable and this is still blocking the use of it

spaze commented 2 years ago

Unfortunately, releases supporting new major or minor PHP versions can take very long.

Take for example the previous two major/minor PHP versions:

After the 7.4 support took 9 months, the maintainers have promised quarterly releases but there never was a quarterly release since then. I can only suggest local modifications using the composer-patches plugin.

This is what we have done to have this fix (and others too) without needing to wait for a release. I've done this for 8.0 support, and I'm now running a patched version with PHP 8.1.

  1. Added extra config to composer.json:
    "extra": {
        "patches-file": "patches/composer.patches.json"
    }

patches/composer.patches.json contains the following:

{
    "patches": {
        "microsoft/azure-storage-blob": {
            "Check null before passing to strpos": "patches/azure-321-check-null.diff"
        }
    }
}

You can create the patches/azure-321-check-null.diff file by appending .diff to the URL of this PR like this https://github.com/Azure/azure-storage-php/pull/321.diff and saving the generated file. Good luck.

pimjansen commented 2 years ago

Yeah guess we will fork this then until this gets update. However we use the league adapter package which has a dependency here so its a pain in the ass.

Pretty bad from Microsoft side to maintain this so badly

spaze commented 2 years ago

Yeah guess we will fork this then until this gets update. However we use the league adapter package which has a dependency here so its a pain in the ass.

Either patching or forking (using the composer replace mapping) is way easier and faster than waiting for a release :-)

I personally prefer patching as it's also a bit easier to revert to the original package when released, at least for me. Otherwise I'd have to use three forked packages so I could start using PHP 8.1.

pimjansen commented 2 years ago

@katmsft @XiaoningLiu can any of you have a look? Its a shame that Microsoft SDK is still not compatible with PHP.

It is even against the readme where 7.x+ is stated

SamMousa commented 2 years ago

A workaround, more broadly for 3rd party code is this:

error_reporting(E_ALL & ~E_DEPRECATED);

It will still break on the next major PHP version though..

nuil commented 2 years ago

So, any update?

SamMousa commented 2 years ago

Move away from Azure is the best approach I reckon. The API SDK is not really being worked on, I'm seeing incidental response times > 3 sec on some of the calls...

pimjansen commented 2 years ago

I think it is time to fork. Will we do that on OSS bases in new organisation or what do you guys want?

SamMousa commented 2 years ago

I'd say do it in a new organisation whether or not it becomes a real community effort. That way you're ready for the future. I myself will probably not have a lot of time to contribute.

SamMousa commented 2 years ago

@M4SX5 is this a sign that there will be actual maintenance on this package?

spaze commented 2 years ago

@M4SX5 is this a sign that there will be actual maintenance on this package?

You too can approve a pull request! 😅 (No, not a sign of anything, unfortunately)

kyleweishaupt commented 2 years ago

Just ran into this issue after migrating our app to Docker & PHP 8.1. Using the League adapter as well. Hope this gets merged soon.

spaze commented 2 years ago

Hope this gets merged soon.

Hopefully but please don't hold your breath, it's 9 months already after all! I'd suggest following this guide https://github.com/Azure/azure-storage-php/pull/321#issuecomment-991373427 and patch your lib.

katmsft commented 2 years ago

/azp run

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
pimjansen commented 2 years ago

@spaze much wow!