Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version 1.5.0 should be considered a breaking change? #28

Open
rguttersohn opened this issue Jun 29, 2023 · 6 comments
Open

Version 1.5.0 should be considered a breaking change? #28

rguttersohn opened this issue Jun 29, 2023 · 6 comments

Comments

@rguttersohn
Copy link

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
Copy link
Member

JanEbbing commented Jun 30, 2023

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
Copy link

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
Copy link
Member

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

@diderich
Copy link

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
Copy link
Author

@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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants