Media::fromParams doesn't default to secure, nor respect configuration

dwightwatson commented 3 years ago

Bug report for Cloudinary PHP SDK

Describe the bug in a sentence or two.

I've upgraded to version 2 of the Cloudinary PHP SDK and used the migration guide to convert from cloudinary_url to Media::fromParams. However, this method doesn't default to secure URLs (odd behaviour), and it doesn't respect a global secure configuration option either.

Cloudinary::instance([
    'cloud' => [
        'cloud_name' => config('services.cloudinary.cloud'),
        'api_key' => config('services.cloudinary.key'),
        'api_secret' => config('services.cloudinary.secret'),
    ],
    'url' => [
        'secure' => true,
        'analytics' => false,
    ],
]);

From reading the source it looks like that is where the secure option is meant to go, but it doesn't appear to affect Media::fromParams. It's not documented, but it probably should be.

My workaround for this is padding ['secure' => true] as an additional option to the second argument of Media::fromParams but that's painful and prone to error.

Issue Type (Can be multiple)

[ ] Build - Can’t install or import the SDK
[ ] Performance - Performance issues
[x] Behaviour - Functions aren’t working as expected (such as generate URL)
[x] Documentation - Inconsistency between the docs and behaviour
[ ] Other (Specify)

Steps to reproduce

Create a URL using Media::fromParams and it'll return an HTTP URL.

Error screenshots or Stack Trace (if applicable)

…

Operating System

[ ] Linux
[ ] Windows
[ ] macOS
[x] All

Environment and Frameworks (fill in the version numbers)

PHP Cloudinary SDK version - 2.0.2
PHP Version - 8.0.2
Framework (Laravel, Symphony, etc) - Laravel 8.28.1

const-cloudinary commented 3 years ago

@dwightwatson , thank you for reporting the issue.

The reason for this behavior is that now the default value for secure is true, and in v1 it was set to false. The primary goal of Media::fromParams() is to provide backwards compatibility and produce the same URL. Right now it is impossible to know whether 'secure' => true comes from the default value or it was set explicitly. One of the solutions would be to track the keys that were set and respect their values. We'll release a fix for this issue soon.

dwightwatson commented 3 years ago

Alright - that makes sense. Re-reviewing the migration docs now I see that I had missed that fromParams is more a of a migration assistant rather than the way of doing things. Still, I was getting secure images before and after the change I wasn't.

const-cloudinary commented 3 years ago

@dwightwatson The issue should be fixes in 2.0.3

cloudinary / cloudinary_php