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

Upgrade to 1.5-beta (complete package) does not work on PHP < 7.1 #7975

Closed
alecpl opened this issue Mar 26, 2021 · 17 comments
Closed

Upgrade to 1.5-beta (complete package) does not work on PHP < 7.1 #7975

alecpl opened this issue Mar 26, 2021 · 17 comments
Milestone

Comments

@alecpl
Copy link
Member

alecpl commented Mar 26, 2021

https://www.roundcubeforum.net/index.php/topic,29361.msg74738.html

@alecpl
Copy link
Member Author

alecpl commented Mar 26, 2021

@thomascube Probably some mistake in the release process.

@alecpl alecpl added this to the 1.5-rc milestone Mar 26, 2021
@alecpl alecpl changed the title 1.5-beta complete realease package does not work on PHP < 7.1 Upgrade to 1.5-beta (complete package) does not work on PHP < 7.1 Mar 27, 2021
@alecpl
Copy link
Member Author

alecpl commented Mar 27, 2021

The problem is with Guzzle dependencies:

  • ralouphie/getallheaders v3 requires PHP >= 5.6 (this we could probably workaround by forcing v2, which still should work)
  • symfony/polyfill-intl-idn requires PHP >= 7.1 (this is the main problem).

We might just use composer's --ignore-platform-reqs option when building the package.

@thomascube
Copy link
Member

@alecpl I'm not sure that --ignore-platform-reqs would solve the problem. I thought that only has an effect then installing but not at runtime.

@alecpl
Copy link
Member Author

alecpl commented Mar 29, 2021

If I run composer update --ignore-platform-reqs the platform_check.php file is not generated, and if it existed before it is removed.

@thomascube
Copy link
Member

Indeed, that does the trick. I just hope that Guzzle dependencies that would not run on PHP5 are not used by our code.
We'll shit the next release with platform check disabled and figure it out.

@kgl2001
Copy link

kgl2001 commented Mar 29, 2021

I got an error when I tried that. Am I doing something wrong?

https://www.roundcubeforum.net/index.php/topic,29361.msg74761.html#msg74761

@alecpl
Copy link
Member Author

alecpl commented Aug 13, 2021

Reopening. The solution might not be working according to the above forum thread.

@alecpl
Copy link
Member Author

alecpl commented Aug 13, 2021

All symfony/polyfill-* packages in the version it is included in the vendor folder require PHP 7.1. And their code is executed always, not when needed. Maybe we can fix that by requiring symfony/polyfill-intl-idn:1.19.0 (which is the last version with PHP5 support).

@alecpl alecpl modified the milestones: 1.5-rc, 1.5.0 Aug 14, 2021
@alecpl
Copy link
Member Author

alecpl commented Aug 14, 2021

Nah, this sucks. Maybe we should just run composer config platform.php 5.5 for packaging, but then will this work with PHP8?

Maybe we need just some extra INSTALL instructions when installing/updating on PHP < 7.1.

@thomascube what do you think?

@staticvoidmaine
Copy link

An unsolicited opinion from a kind stranger:

Supporting both installation options seems like the ideal approach for this release in order to offer backward compatibility as well as support for those who can or prefer to use more up to date software. While supporting both is not great long term, a deprecation plan would give users on older versions of PHP time to plan their migration while allowing you to drop support after the window closes.

@alecpl
Copy link
Member Author

alecpl commented Aug 22, 2021

@thomascube I was thinking about something like this:

--- a/INSTALL
+++ b/INSTALL
@@ -40,6 +40,11 @@ INSTALLATION
   tar command `--no-same-owner` option might be helpful.
 2. In case you don't use the so-called "complete" release package,
   you have to install PHP and javascript dependencies.
+  --------------------------------------------------------------------
+  IMPORTANT: For PHP < 7.1 you have to always run `composer update` or
+             `composer install` as described below, even when you use
+             the complete package.
+  --------------------------------------------------------------------
   2.1. Install PHP dependencies using composer:
     - get composer from https://getcomposer.org/download/
     - rename the composer.json-dist file into composer.json
diff --git a/UPGRADING b/UPGRADING
index 4719dbbe7..aeab03452 100644
--- a/UPGRADING
+++ b/UPGRADING
@@ -22,6 +22,8 @@ removed again.
 
 WARNING: Make sure files have proper owner/group for your setup. If you use
          tar to extract the package, `--no-same-owner` option might be helpful.
+WARNING: When you're using the complete package on PHP < 7.1 you have to run
+         `composer update --no-dev` before executing the installto.sh script.
 WARNING: See Post-Upgrade Activities section below.
 WARNING: If you use MySQL < 5.7.7 or MariaDB < 10.2.2 make sure to configure it with:
             innodb_large_prefix=true
diff --git a/program/include/iniset.php b/program/include/iniset.php
index 9e279953e..00f1530d2 100644
--- a/program/include/iniset.php
+++ b/program/include/iniset.php
@@ -57,6 +58,12 @@ if (set_include_path($include_path) === false) {
 
 // include composer autoloader (if available)
 if (@file_exists(INSTALL_PATH . 'vendor/autoload.php')) {
+    if (PHP_VERSION_ID < 70100) {
+        if (@file_exists(INSTALL_PATH . 'vendor/symfony/polyfill-php72')) {
+            die("Found dependencies incompatible with PHP < 7.1. Run `composer update` first.");
+        }
+    }
+
     require INSTALL_PATH . 'vendor/autoload.php';
 }
 

@thomascube
Copy link
Member

The complete package is also meant for those who cannot execute any scripts on their host so this might only be half of a solution. I guess we‘ll end up with two flavors of complete packages to provide.

@thomascube
Copy link
Member

Nah, this sucks. Maybe we should just run composer config platform.php 5.5 for packaging, but then will this work with PHP8?

@alecpl does this mean that the fix from commit beb4008 does not work?
I don't have PHP8 installed anywhere so you might be able to verify if packages with composer config platform.php 5.5 will work on PHP8?

@alecpl
Copy link
Member Author

alecpl commented Aug 29, 2021

@thomascube, the fix didn't work, so it's reverted. It looks that composer config platform.php 5.5 dependencies are working on PHP 8.0, but I didn't test much. Executing tests reveals only a single deprecation notice from phpunit. So, maybe we should do this for packaging, it's bad it wasn't tested with a release candidate, but is it a reason to release another one?

@thomascube
Copy link
Member

It looks that composer config platform.php 5.5 dependencies are working on PHP 8.0, but I didn't test much.

I also successfully tested this with a php8 docker image but when looking at the package, the vendor/symfony/polyfill-php72 folder still is there which makes me believe that it still includes a polyfill which will not run on php5.

@thomascube
Copy link
Member

Building the package with composer config platform.php 5.5; composer require symfony/polyfill-intl-idn:1.19.0 seems to work with all PHP versions. Tested with PHP 5.6 and PHP 8.0.10.

@alecpl
Copy link
Member Author

alecpl commented Aug 31, 2021

1.19.0 is the version that does not have the issue, as it supports php 5.3, higher versions dropped support for php5. From the two commands you mentioned we need only the former. Let's do it. I think we can close this ticket.

@alecpl alecpl closed this as completed Aug 31, 2021
marczi pushed a commit to marczi/roundcubemail that referenced this issue Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants