Closed alexmigf closed 2 years ago
I forgot to mention that I tested with DOMPDF version 1.1.1
, which the minimum version is PHP version 7.1
, but it depends on php-svg-lib
which the minimum version is currently PHP 7.4
. So, DOMPDF minimum PHP version isn't in fact 7.1
because it depends on the php-svg-lib
version which is 7.4
, if I'm not mistaken.
When installing DOMPDF 1.1.1, composer pushes the version of the php-svg-lib
to the 0.3.4
, which already has dropped the older PHP versions: https://github.com/dompdf/php-svg-lib/blob/f627771eb854aa7f45f80add0f23c6c4d67ea0f2/.travis.yml#L3-L6
Composer installed packages output:
The goal is not to make any PHP version specific fixes, but just to keep PHP7.1 support as long as it doesn't cause any issues, if possible, since it doesn't require any code changes. The library seems owned by DOMPDF itself, so it makes sense to keep it matching with the main product I believe.
Indeed you cannot get the latest dompdf, if you don't use supported PHP. That makes sense to me.
The problem with old PHP version is not "doesn't require any code changes", but that it prevent modernisation of the codebase. Eg: as long as we support PHP 7.1, we cannot reasonable offer any strict types guarantees.
To avoid the false expectations that you experienced, the proper fix would be to only support PHP 7.4 in dompdf too. I created https://github.com/dompdf/dompdf/pull/2705 for that.
The lack of coordination in PHP version support is problematic. The libraries used to be semi-independent. php-svg-lib was only recently folded back into the Dompdf project.
Honestly, if lowering the PHP dependency to 7.1 doesn't break things I am fine doing so. If php-svg-lib can run on PHP 7.1 then why not allow it. Just because we nominally support an older version of PHP now (because we're only using features supported by it) doesn't mean we have to adhere to that with future development.
Then again, php-svg-lib has already bumped the PHP dependency to 7.4. Does it make sense to re-target the earlier version with a new release?
it prevents modernisation of the codebase.
No it doesn't. If you modernize the code and you can no longer support 7.1-7.3, you drop support for those versions. But dropping support now, without changing code, doesn't do anything for modernisation.
Honestly, if lowering the PHP dependency to 7.1 doesn't break things I am fine doing so.
I agree. I definitely see the point @PowerKiKi makes about code modernization, but I personally think it doesn't make sense to bump the dependency before actually doing this. Like @alexmigf indicated, no code changes are required to support 7.1.
I have a lot of clients using dompdf on older PHP versions and while I would love to see them update, their systems (with other components that are developed at a slower pace) sometimes prevent them from doing so.
In short, I think dropping 7.1-7.3 would be better suited for a 1.0 release, perhaps coupled with the 2.0 dompdf release. But since dompdf itself still supports 7.1, it makes sense to have php-svg-lib
support 7.1 as well (it already does!).
Then drop support when you implement changes that can't be done for 7.1.
And people stuck with old software will still be stuck with old software 🤷
Keep in mind that this library can be used in distributed software too, where one package needs to be able to run on multiple different configurations/platforms. It's not possible to bundle multiple versions of the same library and then have the composer autoloader pick the appropriate one. The result is that some people stuck with old software keep people with modern software from getting updates.
If you care about more strict typing - the syntax for it works fine in PHP7.1 too (I know the strictness itself has evolved, but from a code contribution perspective that's less of a problem), so there's no reason to not start doing that now. Personally I wouldn't use a code style that doesn't fit the rest of the project's code when making contributions.
That said, I can definitely see your argument about code contributions, but to me it makes sense to keep the PHP version of this library matching with the "parent" project, which I think was the main argument for this PR.
I believe dependency requirements should reflect the minimum known supported environment and not aspirational development priorities. As such I'm inclined to support this PR.
Concerns about code modernization can be addressed through a CONTRIBUTING.md in which we can indicate that features of newer PHP releases are welcome. I also suggest we outline the parameters by which they would be accepted, along the lines of:
Hey guys, it's a breaking change to add a newer php version requirement in a patch release, causing for us, for example, some issues in production, as dompdf requeres "^0.3.3" of the library and suddenly stopped working on php7.3, I see that this is referenced in 0.4.1, but it can't go only to 0.4. version as it will not fix a broken dompdf 1.0.2 dependency. Would it be possible please to fix it 0.3. version?
Problem 1
- phenx/php-svg-lib is locked to version 0.3.4 and an update of this package was not requested.
- phenx/php-svg-lib 0.3.4 requires php ^7.4 || ^8.0 -> your php version (7.3.33) does not satisfy that requirement.
Problem 2
- phenx/php-svg-lib 0.3.4 requires php ^7.4 || ^8.0 -> your php version (7.3.33) does not satisfy that requirement.
- dompdf/dompdf v1.0.2 requires phenx/php-svg-lib ^0.3.3 -> satisfiable by phenx/php-svg-lib[0.3.4].
- dompdf/dompdf is locked to version v1.0.2 and an update of this package was not requested.
Thanks.
it's a breaking change to add a newer php version requirement in a patch release
No, it is not. Dompdf will continue to install, and run, without issue with PHP 7.3 as shown here:
php7.3 /usr/local/bin/composer require dompdf/dompdf
Using version ^1.1 for dompdf/dompdf
./composer.json has been created
Running composer update dompdf/dompdf
Loading composer repositories with package information
Updating dependencies
Lock file operations: 4 installs, 0 updates, 0 removals
- Locking dompdf/dompdf (v1.1.1)
- Locking phenx/php-font-lib (0.5.4)
- Locking phenx/php-svg-lib (v0.3.3)
- Locking sabberworm/php-css-parser (8.4.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 4 installs, 0 updates, 0 removals
- Installing sabberworm/php-css-parser (8.4.0): Extracting archive
- Installing phenx/php-svg-lib (v0.3.3): Extracting archive
- Installing phenx/php-font-lib (0.5.4): Extracting archive
- Installing dompdf/dompdf (v1.1.1): Extracting archive
2 package suggestions were added by new dependencies, use `composer suggest` to see details.
Generating autoload files
You might not get the latest version of phenx/php-svg-lib
, and that might be an inconvenience for you, but that is not a breaking change.
You are installing dompdf 1.1, it breaks 1.0.2
Regards
Dompdf 1.0.2 can also be installed with PHP 7.3. If something else does not work, please share a reproduction case similar to mine.
php7.3 /usr/local/bin/composer require dompdf/dompdf:1.0.2
./composer.json has been created
Running composer update dompdf/dompdf
Loading composer repositories with package information
Updating dependencies
Lock file operations: 4 installs, 0 updates, 0 removals
- Locking dompdf/dompdf (v1.0.2)
- Locking phenx/php-font-lib (0.5.4)
- Locking phenx/php-svg-lib (v0.3.3)
- Locking sabberworm/php-css-parser (8.4.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 4 installs, 0 updates, 0 removals
- Installing sabberworm/php-css-parser (8.4.0): Extracting archive
- Installing phenx/php-svg-lib (v0.3.3): Extracting archive
- Installing phenx/php-font-lib (0.5.4): Extracting archive
- Installing dompdf/dompdf (v1.0.2): Extracting archive
2 package suggestions were added by new dependencies, use `composer suggest` to see details.
Generating autoload files
Hi @PowerKiKi my apologies, looks like I wasn't entirely correct, running composer require installs the library fine, it conflicts with requirements in composer.json.
The simplest composer.json file:
{
"require": {
"php": ">=7.3.4 <8.1",
"dompdf/dompdf": "1.0.2"
}
}
composer install results in:
php7.3 /usr/local/bin/composer install
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
Your lock file does not contain a compatible set of packages. Please run composer update.
Problem 1
- phenx/php-svg-lib is locked to version 0.3.4 and an update of this package was not requested.
- phenx/php-svg-lib 0.3.4 requires php ^7.4 || ^8.0 -> your php version (7.3.33) does not satisfy that requirement.
Problem 2
- phenx/php-svg-lib 0.3.4 requires php ^7.4 || ^8.0 -> your php version (7.3.33) does not satisfy that requirement.
- dompdf/dompdf v1.0.2 requires phenx/php-svg-lib ^0.3.3 -> satisfiable by phenx/php-svg-lib[0.3.4].
- dompdf/dompdf is locked to version v1.0.2 and an update of this package was not requested.
The reason that I'm calling it a breaking change is that we haven't changed our composer.json and once phenx/php-svg-lib 0.3.4 was released composer update started to fail. dompdf 1.0.2 itself lists php7.1 as required php hence its dependencies should not introduce a hard requirement suddenly to a higher php version.
Regards, Sasha.
UPD looks like install fails if lock file exists, running composer update downgrades the library correctly.
So to follow up, the problem is reproducible, if originally dependencies were installed using php7.4 which pulls the latest version of the library, then attempt to run composer install with 7.3 it fails.
So the simple reproduction steps would be:
php7.4 composer require dompdf/dompdf:1.0.2
rm -rf ./vendor
php7.3 composer install
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
Your lock file does not contain a compatible set of packages. Please run composer update.
Problem 1
- phenx/php-svg-lib is locked to version 0.3.4 and an update of this package was not requested.
- phenx/php-svg-lib 0.3.4 requires php ^7.4 || ^8.0 -> your php version (7.3.33) does not satisfy that requirement.
Problem 2
- phenx/php-svg-lib 0.3.4 requires php ^7.4 || ^8.0 -> your php version (7.3.33) does not satisfy that requirement.
- dompdf/dompdf v1.0.2 requires phenx/php-svg-lib ^0.3.3 -> satisfiable by phenx/php-svg-lib[0.3.4].
- dompdf/dompdf is locked to version v1.0.2 and an update of this package was not requested.
originally dependencies were installed using php7.4 which pulls the latest version of the library, then attempt to run composer install with 7.3 it fails
If you do this, then you are not using composer correctly, and there cannot be any guarantees at all about anything for any packages. You must run all composer commands with the same PHP version that will be used in production. Anything else is asking for trouble, especially if production has a lower version of PHP as you implied.
This came up during a routine dependencies review by one of the developers, we do have dependencies installed with the minimum supported version. Narrowing it down to only the described above case makes it ok I guess, but my point to this is that I don't quite understand why such a change was introduced in a patch release, and not only went to 0.4.
I understand the reasons requiring a newer php version to allow use of new language features, what I'm trying to say is that because of the various constraints vendors have to retain a lower php version as a minimum supported despite their software running fine with php7.4 or php8, they won't get other fixes that landed to 0.3.4. 🤷♂️
Sorry for the inconvenience, if any. :)
Whilst you can run composer to successfully install DomPDF and all its dependencies still on PHP 7.1, the issue for some of us is that we're building software for distribution, not knowing which PHP version it will end up on. The change means that if we're building on PHP 7.4, then we now get a version of the dependencies whose composer tree requires PHP >= 7.4. So now we have to specify a specific, old, version of php-svg-lib in composer.json
in order to avoid that - even though the latest version is actually still PHP 7.1-compatible. i.e. The change forces us to choose one from these options, and all of them aren't good in at least one way: 1) End up with a composer tree that specifies a requirement on a later version than our software is for, and ignore that (so, the composer tree no longer describes reality, it has hidden caveats). Also you need to disable platform checks (i.e. stop composer's autoloader throwing an unwanted fatal error). 2) Specify an older version explicitly so that the tree is valid, and choose to forego on fixes in the more recent version.
Hence I also agree with the PR, that the version dependency on a release should accurately specify what versions it runs on, rather than something else.
specify a specific, old, version of php-svg-lib in
composer.json
That's not a good idea. Instead you should tell composer what is the oldest PHP version that you are willing to support and let composer pick the versions automatically. To do that, always run composer with the oldest PHP version. So something like that:
php7.1 /usr/local/bin/composer require dompdf/dompdf
or
php7.1 /usr/local/bin/composer update
If you don't always tell composer which PHP version you support, then you will run in trouble, with this lib or any other transient dependencies of your project.
forego on fixes in the more recent version
Of course you cannot expect to always get latest version of libraries if you refuse/cannot upgrade your environnement (PHP in this case). But it's not like you cannot still use the previous version of php-svg-lib. They still exists and are totally usable.
I think you've missed the point being made. The requirements attached to the releases do not accurately state which versions of PHP the releases actually support. So, yes, composer can be run under PHP 7.1. The result will be that an older version is downloaded and distributed. The point is that that older version is unnecessarily old. A newer version will actually run fine on all versions of PHP that the distributor actually cares about. It's not about "expecting" to get the latest version, despite refusing to upgrade. It's about the information in the release being used to be a statement about future aspirations, rather than being a statement about what the release actually requires.
I see what you mean about raising the minimum "without good reason". I expressed my opinion on that in https://github.com/dompdf/php-svg-lib/pull/78#pullrequestreview-839347581.
Thank you. I can't speak for others, but reflecting on why this change caused trouble, I think the main factor is that it happened between 0.3.3
and 0.3.4
. Semantic versioning means that people expect that a change in the 'patch' version won't bring any incompatibilities. I realise that technically that means "no incompatibilities for consumers of the provided APIs", and it could be argued that meta-requirements of the release itself are excluded from that. But in practice, if people put ^0.3.3
in their composer.json, they're not expecting any sort of surprise where it used to work, and now doesn't, in a later 0.3.X
release.
I think your desire to express what code can be submitted is perfectly valid - but that arguably such changes should go on new major release branches, rather than existing branches and patch releases. Otherwise, the version number change no longer has the expected meaning: the end-developer-user can't rely on it indicating any sort of compatibility, but is required to also go elsewhere to learn about whether it's still compatible.
You should almost never specify patch release in composer.json and instead specify minor release. So not ^0.3.3
, but only ^0.3
(which is the default behavior of composer require
).
But most importantly, and even if you specify ^0.3.3
in you composer.json, as long as you use composer correctly and let it do its jobs of picking the package versions according to your environnement, then you will never run in "incompatibilities".
See also https://www.doctrine-project.org/2017/07/25/php-7.1-requirement-and-composer.html and https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api
I think enough has been said on both sides of the argument. I'm planning to move forward with this PR for the reason I stated earlier.
Running a PHP lint Github action, that ended with error, I discovered that
php-svg-lib
, which DOMPDF depends on, requires php version7.4
when DOMPDF minimum version is7.1
. Also PHPUnit version isn't matching because version9.5
requires PHP version7.3
.This PR addresses this issue be setting
php-svg-lib
andphpunit
to PHP minimum version of7.1
to match the DOMPDF one.