Code review comment for ~athos-ribeiro/ubuntu/+source/apache2:postinst-triggered

Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

Thanks, Athos.

I'm leaving a couple of minor comments below, but they're cosmetic.

Code-wise, this change is simple and correct. There's not much to comment, I believe.

Feature-wise, I'm always a bit reticent about introducing a "breaking" change like this, especially when it comes to Apache. I mean, the goal of forcing apache2 to reload the php module when something changes is a good one, and as you mentioned, it actually should have always worked like this. But given that it doesn't (and never has, apparently?), I'm wondering if it will catch any user out there by surprise. For example, when you use "a2enmod" to enable an Apache module, the command doesn't restart apache2 automatically for you; instead, it prints a message saying that you should do it. So I think I'd expect a similar behaviour when it comes to PHP modules...?

Something else that comes to mind is how docker.io works. It will ask the user whether they want the service (and the containers) to be restarted after upgrade. Maybe PHP could do the same.

Anyway, these are all thoughts that occurred to me while reviewing the changes, but not something I'd expect you to implement. And I agree with your assessment in the bug when you say that we should hear what Debian has to say about this change. OTOH, time is running out for Noble and I believe that this is the right moment to introduce such a change.

I'm approving the MP for Noble based on that. I believe it's worth mentioning this change in the release notes as well, because it may catch some users unprepared.

LGTM, +1.

review: Approve

« Back to merge proposal