Revert php version to 7.1+ #78
Conversation
There was a problem hiding this comment.
Installing dompdf with PHP 7.1 will succeed, as shown below. I fail to see what this PR would change.
And more specifically PHP 7.1 has been EOL for two years. We don't have any reason to support unsupported software, and we certainly don't have the resources for it.
What really is the problem you are facing ?
php7.1 /usr/local/bin/composer require dompdf/dompdf
Using version ^0.6.2 for dompdf/dompdf
./composer.json has been created
Running composer update dompdf/dompdf
Loading composer repositories with package information
Updating dependencies
Lock file operations: 2 installs, 0 updates, 0 removals
- Locking dompdf/dompdf (v0.6.2)
- Locking phenx/php-font-lib (0.2.2)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 2 installs, 0 updates, 0 removals
- Downloading phenx/php-font-lib (0.2.2)
- Downloading dompdf/dompdf (v0.6.2)
- Installing phenx/php-font-lib (0.2.2): Extracting archive
- Installing dompdf/dompdf (v0.6.2): Extracting archive
Generating autoload files
|
I forgot to mention that I tested with DOMPDF version |
|
When installing DOMPDF 1.1.1, composer pushes the version of the Lines 3 to 6 in f627771 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 dompdf/dompdf#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? |
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.
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 Then drop support when you implement changes that can't be done for 7.1. |
There was a problem hiding this comment.
The lack of coordination in PHP version support is problematic.
I agree that this was something that I did not forsee. And it would have been better to coordinate before making the change. But on the other hand I don't really see anything blocking either. People with modern software, will get modern software. And people stuck with old software will still be stuck with old software 🤷
dropping support now, without changing code, doesn't do anything for modernisation
Dropping now opens the door for external contributors to use newer syntax/features right now. They don't need to wonder whether they are allowed to raise requirements as part of their unrelated PR. If you publicly broadcast that you support PHP 7.1, contributions will come as PHP 7.1. And the code will not naturally evolves.
Most likely only a core contributors would "dare" to introduce those kind of changes. Again, this drastically reduces the chance to keep the code up to date.
I still disagree to merge this PR. So I won't approve it. But bsweeney can merge it even without my approval if he thinks that important enough.
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? Thanks. |
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 filesYou might not get the latest version of |
|
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. |
|
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: composer install results in: 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, 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: |
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 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. |
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/dompdfor php7.1 /usr/local/bin/composer updateIf 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.
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 #78 (review). |
|
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 |
|
You should almost never specify patch release in composer.json and instead specify minor release. So not But most importantly, and even if you specify 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.4when DOMPDF minimum version is7.1. Also PHPUnit version isn't matching because version9.5requires PHP version7.3.This PR addresses this issue be setting
php-svg-libandphpunitto PHP minimum version of7.1to match the DOMPDF one.