DeepLcom / deepl-php

Official PHP library for the DeepL language translation API.
MIT License
207 stars 22 forks source link

Version 1.5.0 should be considered a breaking change? #28

Open rguttersohn opened 1 year ago

rguttersohn commented 1 year ago

Per, issue #27 DeepL PHP library now requires devs to add

require __DIR__ . '/../vendor/autoload.php';

prior to instantiating the translator.

In my composer.json, I had my constraints set to allow upgrades to non-breaking versions. However, because of this additional step, the DeepL library triggered fatal errors after a running composer install on a deployment.

However when v1.4.0 is installed and running composer outdated, it denotes v 1.5.0 only contains bug fixes. I disagree considering it will likely break the apps of people coming from v.1.5.0.

Anyhow thought I'd give you a heads up in case this was an oversight.

Also, the fix in the issue above should be mentioned in the docs, right?

JanEbbing commented 1 year ago

I agree it would be a breaking change if we now required this line, but I don't think anything changed, see e.g. the composer documentation here? If you use any class from a Library you need to tell PHP how to load it, e.g. with PSR-4 autoloading, in that regard nothing changed.

When I create a test project that depends on deepl-php:1.4.0 and instantiate a Translator and run it with php src/Script.php, I get the same error without this autoloading line:

Fatal error: Uncaught Error: Class "DeepL\Translator" not found in <snip>/deepl-php-test/src/Script.php:5

If I set up unit tests in this project and run them with phpunit, they work in both 1.5.0 and 1.4.0 without autoloading (I assume phpunit does that behind the scenes).

E: Maybe something actionable - if there is any project setup that now (in 1.5.0) requires a line like

require __DIR__ . '/../vendor/autoload.php';

which wasn't required in 1.4.0, please provide an example of it and I'll gladly change this.

diderich commented 1 year ago

FYI (you may disagree with my reasoning). I was implementing a customized loader (rather than the generic one require __DIR__ . '/../vendor/autoload.php'; because:

  1. The generic autoloader introduces some overhead that I did not want and did not provide value to me (I only used 2 composer-loaded libraries, one being DeepL).
  2. Having a customized autoloader gives me control over what classes are loaded, reducing the risk that a library loads classes for libraries that I don't trust, and more importantly, loads a library that breaks some parts of my code (yes, some libraries do this).
JanEbbing commented 1 year ago

@rguttersohn How was autoloading/loading dependencies configured beforehand for these apps that broke?

diderich commented 1 year ago

I loaded the class with the following code:

function my__autoload(string $class_name): void
{
  if(str_contains($class_name, 'DeepL\\')) {
      $class_name = str_replace('DeepL\\', '', $class_name);
      $class_file = _HOLIDAY_APP_ROOT_.'/vendor/deeplcom/deepl-php/src/'.$class_name.'.php';
      if(file_exists($class_file)) { require_once($class_file); return; }
  }
}
rguttersohn commented 1 year ago

@JanEbbing , perhaps I have a bit of a unique situation here. I am using the Bedrock/Sage stack maintained by Roots.io team. I am relying on the Bedrock autoloader which worked fine for autoloading versions <=1.40.

I haven't been able to figure out why that autoloader isn't working.

I also could install DeepL as a theme-level dependence in Sage. It uses a psr-4 autoloader, but I wanted this to be theme agnostic.

JanEbbing commented 1 year ago

Thanks for the info! I will take a stab at checking why that autoloader doesn't work with us next week. I am working on clearing up the installation and offering a way to load the library without using an autoloader, analogous to e.g. the stripe PHP library, this will be released in the next version/patch.