Add composer dependency on ext-zlib

Seems your php version does not have the zlib extension installed.

Actually this is a bug, because we don't explicitly declare a dependency on ext-zlib, but installing the extension should resolve this for you.

Hi mstrelan,

Thank you. However do i need to still install extension for php or this patch/merge can resolve the issue?

Yeah you need to install it. The patch will just prevent people upgrading of they don't have it installed

AssetDumper has a check for zlib which we could remove if we are adding a dependency:

core/lib/Drupal/Core/Asset/AssetDumper.php
63:    // If CSS/JS gzip compression is enabled and the zlib extension is available
71:    if (extension_loaded('zlib') && \Drupal::config('system.performance')->get($file_extension . '.gzip')) {

I guess almost everyone must have this extension installed or we would have had more reports along the lines of this one.

I guess if we add this in 10.2.x it will just mean folks without zlib will get stuck on 10.2.5 which will also be broken for them. So possibly we need a runtime check in 10.2.6 (or later) to handle it gracefully.

btw that's not that easy to build PHP with zlib extension as shared https://github.com/php/php-src/pull/4681

I think we can skip compression when zlip isn't available - it'll mean very long asset query strings but can't really be helped. And then open an 11.x issue to require zlib and remove the checks.

Actually let's make this the 11.x issue, I'll open an 10.3.x one.

Needs work for #8.

pradhumanjain2311 → made their first commit to this issue’s fork.

Remove zlib as per 8

There's an open thread on the MR that I don't know the answer to. Also the build is failing so I obviously didn't follow the correct procedure to update composer.json.

Requiring zlib doesn't seem like too big of an ask. But has me wondering - I guess there was some reason why it's better to pass all the asset data in long URLs, rather than just storing it persistently and looking it up via the asset file name when needed?

@mfb see 🐛 Stampedes and cold cache performance issues with css/js aggregation Fixed for the change.

@catch the part that confuses me is that asset data is both cached in the cache_data bin, and also sent to the browser in the long query string. I'd have thought one or the other would be sufficient. But, I didn't read thru all the related issues yet, I'm probably missing some basics.

I ran COMPOSER_ROOT_VERSION=11.x-dev composer update drupal/core to update the lock file. The command is part of this, https://www.drupal.org/about/core/policies/core-dependency-policies-and-... →

@mfb the asset generation routes need to be able to independently and reliably generate an asset based on the information on the URL. It would be possible to store the information in in state or key/value so it can't be e.g. evicted from memcache, then only have the lookup key in the URL, but that then creates the potential for a database stampede after cache clears. There's also not a reliable way to know when any particular aggregate would be stale, so the amount of data stored could grow very large over time. The agrcache module in Drupal 7 did things this way, and I think advagg had something similar-ish, but they both had their own problems. The idea to encode the libraries in the URL was from about ten years ago, it took adding the core libraries API and various further optimisations to get there.

Obviously with the new core solution we've introduced different problems, but IMO they're lesser ones.

@catch thanks for further explanation. It surprised me to see how many cache_data insert queries run on a cold cache: a "js:..." cache item is inserted during the page load, and further "js:..." and "route:..." cache items are inserted during asset load/generation. So that's why I was thinking the data in the URL could simply be persistently stored and queried rather than passed around. But yes, those cache sets would be a lot more lightweight with a non-database cache backend in place.

AFAIK if we remove the check from AssetDumper, UrlHelper::compressQueryParameter and UrlHelper::uncompressQueryParameter would need to be updated too.

@penyaskito, yes that needs to be done to.

I also grepped for zlib and found a few more things to change.

I think we need to add the dependency to core/lib/Drupal/Component/Utility/composer.json so that can be used as a standalone library. Might be wrong though, so leaving NR.

I would suggest adding zlib to system_requirements() as well, because of various scenarios such as zlib extension could be disabled after the instance was installed, instance could be moved to an environment without zlib extension, etc.

@mfb there's no reason to add it system_requirements because the extension is bundled into PHP and it's someones hacky attempt to build PHP with zlib as shared extension.

So it's a waste of CPU for no reason and slowdown for "not fast" page, even #28 suggestion does not make sense as no common way to get PHP without this extension even to make a test

See https://www.php.net/manual/en/extensions.membership.php#extensions.membe...

No reason to waste time on the non-reproducible bug

The page linked in #31 lists zlib in the same category as gd and PDO yet we explicitly list those in composer.json. Is zlib somehow different or can we drop all those too?

Yes, it means the extension must be available in distribution but I also pointed link in #10 that since PHP 8 there's no way to build PHP with zlib extension as shared (means it it's a separate file and it can be disabled)

So if someone hacked PHP source for some reason - they they know what are they doing and core must not care about such hacky cases

I think this change is fine, it is explicit in Composer to try and prevent installs on weird environments and therefore we can remove the runtime checks as well.

Yeah same here, but #34 is a good reason not to bother with a hook_requirements() entry, which means less to do here.

I'm ++ to removal of useless extension_loaded('zlib') but as the issue marked bug - it should change title/category or provide steps to reproduce

@andypost It's not true that the zlib extension is always bundled into PHP. On FreeBSD, for example, it is a separate package which is installed via pkg install php83-zlib and I'm sure some other distributions work in a similar way. I.e. each distro has its own definition of the PHP package and provides packages for the other extensions.

@mfb I mean it's supposed to be bundled but distro builders can change it with patches.

Moreover it looks weird what they're doing https://cgit.freebsd.org/ports/tree/archivers/php83-zlib/files/patch-zlib.c
So the same way you should require hash,PDO and other packages for no reason, just slowdown each composer install with dependencies for 0.5% of users

PHP is designed to be configurable at compile time, so patches aren't necessarily needed. You can build with or without various extensions/libraries, whether for running in a memory-constrained environment or any other reason. On the one hand, I'd appreciate if PHP could be guaranteed to be built the same way, but on the other hand I can appreciate it being configurable..

Discussed with @quietone, in order to get the upcoming betas out today we need to commit these today, so committing from NR. Unfortunately we need different patches for each branch, I solved the issues in 11.x and 11.0.x but marking PTBP to get a 10.3.x and 10.4.x patch ready.

Committed 0a4f122 and pushed to 11.x. Thanks!

Committed b3a8376 and pushed to 11.0.x. Thanks!

@quietone reminded me this is major version only. 🐛 Skip query string compression if zlib extension isn't available Fixed works around the issue if zlib is not available, but we can still require it to make things simpler in 11.0.

mstrelan → closed merge request !7382

Automatically closed - issue fixed for 2 weeks with no activity.