Problem/Motivation
After the upgrade to consumer_image_styles version 4.0.10 I observed performance regressions on a project using the public stream wrapper from
S3 File System →
module. The regression was similar to the fixed issue
🐛
Follow-up: Very slow JSON:API responses when images are stored on AWS bucket
Needs review
.
Steps to reproduce
Use the public stream wrapper from s3fs and debug \Drupal\consumer_image_styles\ImageStylesProvider::buildDerivativeLink().
The function should return before determining the image dimensions (from local file system):
if ($this->streamWrapperManager->getViaScheme(StreamWrapperManager::getScheme($uri))->getType() & ~StreamWrapperInterface::LOCAL_NORMAL) {
return $info;
}
It does not. But it worked before the fix from
🐛
Default public scheme is wrongly not considered as a local filesystem
Active
, with following change:
diff --git a/src/ImageStylesProvider.php b/src/ImageStylesProvider.php
index bd04542..e629f41 100644
--- a/src/ImageStylesProvider.php
+++ b/src/ImageStylesProvider.php
@@ -124,7 +124,7 @@ class ImageStylesProvider implements ImageStylesProviderInterface {
];
// Sites with external images cannot afford to download the image to the
// webserver in order to inspect the image dimensions.
- if ($this->streamWrapperManager->getViaScheme(StreamWrapperManager::getScheme($uri))->getType() & ~StreamWrapperInterface::LOCAL) {
+ if ($this->streamWrapperManager->getViaScheme(StreamWrapperManager::getScheme($uri))->getType() & ~StreamWrapperInterface::LOCAL_NORMAL) {
return $info;
}
$image = $this->imageFactory->get($uri);
The s3fs module alters the stream_wrapper.public from the default one Drupal\Core\StreamWrapper\PublicStream to Drupal\s3fs\StreamWrapper\PublicS3fsStream, whose type is StreamWrapperInterface::NORMAL, which "does not include StreamWrapperInterface::LOCAL" (see documentation). If I understand correctly it's not a local stream wrapper, so it's usage should be fine.
To get a better understanding of the above mentioned if statement I did the following check:
StreamWrapperInterface::LOCAL_NORMAL; // 0000000000011101
~StreamWrapperInterface::LOCAL_NORMAL; // 1111111111100010
StreamWrapperInterface::LOCAL; // 0000000000000001
~StreamWrapperInterface::LOCAL; // 1111111111111110
StreamWrapperInterface::NORMAL; // 0000000000011100
As the statement uses the bitwise operation AND it will evaluate to false (= local) for StreamWrapperInterface::LOCAL_NORMAL, which was the intention/fix of
🐛
Default public scheme is wrongly not considered as a local filesystem
Active
:
StreamWrapperInterface::LOCAL_NORMAL & ~StreamWrapperInterface::LOCAL_NORMAL
0000000000011101
1111111111100010
0000000000000000
Before the fix it would have falsely evaluated to true (=not local):
StreamWrapperInterface::LOCAL_NORMAL & ~StreamWrapperInterface::LOCAL
0000000000011100
1111111111111110
0000000000011100
In that case the fix was fine. But in case of StreamWrapperInterface::NORMAL on the left site it will falsely evaluate to false (=local)
StreamWrapperInterface::NORMAL & ~StreamWrapperInterface::LOCAL_NORMAL
0000000000011100
1111111111100010
0000000000000000
BTW: Before the fix it worked fine, evaluated to true (=not local):
StreamWrapperInterface::NORMAL & ~StreamWrapperInterface::LOCAL
0000000000011100
1111111111111110
0000000000011100
Proposed resolution
As johnny5th commented in
[#3252023-25] →
it should actually be:
if (!($this->streamWrapperManager->getViaScheme(StreamWrapperManager::getScheme($uri))->getType() & StreamWrapperInterface::LOCAL)) {
return $info;
}
That is also much easier to read. The inner comparison will always evaluate to true (=local) for streamwrapper types where the right bit is set to 1, no matter what the other bits are:
***************1
0000000000000001
0000000000000001
The negation will turn it to true (= NOT local).