Merge ~bryce/ubuntu/+source/php7.4:fix-lp1865218-mod-php-upgrade-groovy into ubuntu/+source/php7.4:ubuntu/devel

[Bryce Harrington]

SRU to fix failure when upgrading with mod-php enabled.

I've verified this works using the test case from the bug #1865218, using this PPA:

PPA: https://launchpad.net/~bryce/+archive/ubuntu/php-defaults-fix-lp1865218-mod-php-upgrade

[Christian Ehrhardt  (paelzer)]

Generally I like the new approach, to disable the old one to avoid the latter conflict.
By not existing early the new one would be enabled as intended.

A few questions:

Improved wording
"Disabling $PHP_MODULE" -> "Disabling old $PHP_MODULE"
If you insist on being perfect since this would also trigger on downgrades you can version-compare and say "old/new" but downgrades are nonono anyway, so a static "old" will do as well.

  "failed to enable $PHP_MODULE"
That should now better be something like:
  "failed to disable old $PHP_MODULE module"

In some of the SRUs backports the php version in the message needs to change at "favor of using PHP 7.4".
If you want you might use @PHP_VERSION@ to get code that is the same for all releases.

review: Needs Fixing

[Bryce Harrington (bryce)]

Suggested changes applied, ready for re-review.

[Christian Ehrhardt  (paelzer)]

thanks, I was going for a practical test, but the PPA is odd, it has 2 bionic and 2 eoan versions (two php versions each).
But no Focal/Groovy builds at all.

I'd have expected builds as proposed here in the PPA.

I tried to setup a system that would trigger the upgrade path as needed, but it gets more and more (like php-common >73) and so on.

Therefore I have manually done what the Maintscripts would do.
That worked well, but let us make sure that on the actual SRU we really test the real builds.

Finally, I know Debian does handle php versions differently but is this in some way upstreamable to them still or will we have to carry it forever? If we have to, please add a bunch of extra text to the groovy-commit so we can all remember.

review: Approve

[Bryce Harrington (bryce)]

On Thu, May 14, 2020 at 11:52:37AM -0000, Christian Ehrhardt  wrote:
> Review: Approve
>
> thanks, I was going for a practical test, but the PPA is odd, it has 2 bionic and 2 eoan versions (two php versions each).
> But no Focal/Groovy builds at all.
>
> I'd have expected builds as proposed here in the PPA.

I've uploaded the updated binary builds for the PPA as well.

> I tried to setup a system that would trigger the upgrade path as needed, but it gets more and more (like php-common >73) and so on.
>

Interesting, I am thinking the steps to trigger the condition should be
straightforward, but maybe easier with the updated ppa.

> Therefore I have manually done what the Maintscripts would do.
> That worked well, but let us make sure that on the actual SRU we really test the real builds.
>
> Finally, I know Debian does handle php versions differently but is this in some way upstreamable to them still or will we have to carry it forever? If we have to, please add a bunch of extra text to the groovy-commit so we can all remember.

That's a good point to provide comments in the groovy-commit, however
this package isn't (yet) in git-ubuntu so the commit messages will be
lost. Hopefully the commits in the bug report will be sufficient as
institutional memory.

Regarding sending this to debian, I've thought a bit about this. I
think we have a bit of a philosophical difference in that Ubuntu accepts
a restriction of no more than one PHP version in a given Ubuntu release
with a goal of simplified user experience, whereas Debian strives for
allowing a more permissive multi-PHP version experience at the expense
of a more complex user experience. So, I'm not sure this patch - at
least as I've coded it - is going to meet their needs.

What I'd like to do is land it for us and make sure it meets our own
userbase's needs, let Debian have a chance to review and adjust if they
wish, and then perhaps for Ubuntu h-series or i-series see if we can
find a common solution with Debian.

Bryce

[Bryce Harrington (bryce)]

I went ahead and mentioned the fix on the associated Debian bug. Again, I doubt they would want to take this patch as-is but might give them a starting point for a better fix.

diff --git a/debian/changelog b/debian/changelog
index a8f481d..5cd557a 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+php7.4 (7.4.3-4ubuntu3) groovy; urgency=medium
+
+  * libapache2-mod-php.postinst.extra: Disable other mod-php versions.
+    Fixes failure when upgrading from previous versions of mod-php.
+    (LP: #1865218)
+
+ -- Bryce Harrington <[e-mail]>  Tue, 21 Apr 2020 23:04:30 +0000
+
 php7.4 (7.4.3-4ubuntu2) focal; urgency=medium
 
   * SECURITY UPDATE: Read one byte of uninitialized memory
diff --git a/debian/libapache2-mod-php.postinst.extra b/debian/libapache2-mod-php.postinst.extra
index 923e475..6c6ef99 100644
--- a/debian/libapache2-mod-php.postinst.extra
+++ b/debian/libapache2-mod-php.postinst.extra
@@ -13,9 +13,14 @@ if [ -e /usr/share/apache2/apache2-maintscript-helper ]; then
 	fi
 
 	PHP_MODULE=$(a2query -m | sed -n 's/^\(php[\.0-9]*\) (enabled.*)/\1/p')
-        if [ -n "$PHP_MODULE" -a "$PHP_MODULE" != "php@PHP_VERSION@" ]; then
-	    apache2_msg "err" "$DPKG_MAINTSCRIPT_PACKAGE: $PHP_MODULE module already enabled, not enabling PHP @PHP_VERSION@"
-	    return 1
+	if [ -n "$PHP_MODULE" -a "$PHP_MODULE" != "php@PHP_VERSION@" ]; then
+	    local a2invoke_ret=0
+	    apache2_msg "info" "$DPKG_MAINTSCRIPT_PACKAGE: Disabling old $PHP_MODULE in favor of using PHP @PHP_VERSION@"
+	    apache2_invoke dismod $PHP_MODULE || a2invoke_ret=1
+	    if [ "${a2invoke_ret}" -ne 0 ]; then
+		apache2_msg "err" "$DPKG_MAINTSCRIPT_PACKAGE: (${a2invoke_ret}) failed to disable old $PHP_MODULE"
+		return 1
+	    fi
 	fi
 
 	mpm=$(a2query -M)