cloudinary / cloudinary_php

PHP extension for Cloudinary
https://cloudinary.com/documentation/php_integration
MIT License
389 stars 151 forks source link

non-static library in the future? #44

Closed justintilson closed 4 years ago

justintilson commented 8 years ago

Hey guys. Whoever coded this and wrote the docs clearly invested a lot of time and energy making all calls static. To the novice PHP coder, this may seem to make the library easier to use but in fact it does not when using your lib in the context of a modern dependency injection container (Symfony, Laravel, Pimple, etc). Having all static calls gets in the way test driven development as well.

Instantiated class variables with dependency injection is a much better approach.

Please resist any urge to make function calls static!

artjomsimon commented 6 years ago

Any update here?

holtkamp commented 6 years ago

probably combinable with https://github.com/cloudinary/cloudinary_php/issues/75, this library seems to have little attention of the maintainers

roeeba commented 6 years ago

Hey guys, I apologize for the very delayed response. Thank you very much for your feedback. We're planning to resolve this issue on our next library version, which is currently in its planning phase. We'll share an ETA as soon as we'll have one.

cloudinaraz commented 4 years ago

Just a quick update that version 2.0 of the plugin is already in the beta phase and is available here.

holtkamp commented 2 years ago

@cloudinaraz and @roeeba in v2 early static instantiation of the Configuration using \Cloudinary\Configuration\Configuration::instance() seems still mandatory:

namespace Project\Domain\Service;

use Cloudinary\Asset\Media as CloudinaryMediaAsset;
use Cloudinary\Configuration\Configuration as CloudinaryConfiguration;
use Psr\Http\Message\UriInterface;

class Media
{

  /**
   * The configuration is instantiated, but static analysis tools like PHPStan will complain about an unused private property
   */
  private CloudinaryConfiguration $cloudinaryConfiguration;

  public function __construct(CloudinaryConfiguration $cloudinaryConfiguration)
  {
    $this->cloudinaryConfiguration = $cloudinaryConfiguration;
  }

  public function getMediaUri(string $publicId, array $options) : UriInterface
  {
    $cloudinaryMediaAsset = CloudinaryMediaAsset::fromParams(publicId, $options); //This now magically works

    return $cloudinaryMediaAsset->toUrl();
  }
}

When using constructor based dependency injection this should not be required.

It would be nice to be able to handover a Configuration instance to for example MediaAsset::fromParams();

Something like this:

namespace Project\Domain\Service;

use Cloudinary\Asset\Media as CloudinaryMediaAsset;
use Cloudinary\Configuration\Configuration as CloudinaryConfiguration;
use Psr\Http\Message\UriInterface;

class Media
{
  private CloudinaryConfiguration $cloudinaryConfiguration;

  public function __construct(CloudinaryConfiguration $cloudinaryConfiguration)
  {
    $this->cloudinaryConfiguration = $cloudinaryConfiguration;
  }

  public function getMediaUri(string $publicId, array $options) : UriInterface
  {
    $cloudinaryMediaAsset = CloudinaryMediaAsset::fromParams(publicId, $options, $this->cloudinaryConfiguration);

    return $cloudinaryMediaAsset->toUrl();
  }
}

Currently this seems not possible, mainly due to https://github.com/cloudinary/cloudinary_php/blob/f1a678314c914205fa42732f88a583cb83311833/src/Asset/BaseAsset.php#L204

cloudinaraz commented 2 years ago

Hey @holtkamp, just so that I understand your use case, are you trying to instantiate multiple Cloudinary configuration objects (instead of using a Singleton), each associated with a different cloud under your parent account so then you can dynamically choose which cloud object to use with each separate asset call in your code?

holtkamp commented 2 years ago

Hoi @cloudinaraz,

thanks for your swift reply:

are you trying to instantiate multiple Cloudinary configuration objects (instead of using a Singleton), each associated with a different cloud under your parent account so then you can dynamically choose which cloud object to use with each separate asset call in your code?

No.

What I would prefer/expect is that the Cloudinary SDK does not "rely" on Configuration::instance() being invoked. In case that is not done, code like this will trigger an exception Invalid configuration, please set up your environment:

echo \Cloudinary\Asset\Image::fromParams('somePublicAssetId', [
  'width' => 100,
  'height' => 100,
])->toUrl();

But nevermind my feedback: instead of using the static fromParams function of the assets, I found out the following more verbose approach seems to work properly when using a constructor injected $cloudinaryConfiguration property:

$cloudinaryImageAsset = new \Cloudinary\Asset\Image('somePublicAssetId', $this->cloudinaryConfiguration);
$cloudinaryImageAsset->setTransformation(
  \Cloudinary\Transformation\Transformation::fromParams([
    'width' => 100,
    'height' => 100,
  ])
);
$cloudinaryMediaAsset->analytics(false);
$cloudinaryMediaAsset->deliveryType(\Cloudinary\Asset::PRIVATE_DELIVERY);
$cloudinaryMediaAsset->signUrl(true);
$cloudinaryMediaAsset->version(123456789);
echo $cloudinaryImageAsset->toUrl();

Maybe it would be useful to add such a more verbose / less "magic" example to the documentation?