sitegeist / sms-responsive-images

This TYPO3 extension provides ViewHelpers and configuration to render valid responsive images based on TYPO3's image cropping tool.
GNU General Public License v2.0
34 stars 18 forks source link

ViewHelper classes should not be final #101

Closed dmitryd closed 6 months ago

dmitryd commented 1 year ago

If they are final, they cannot be further extended by custom extensions. They were not final before and making them final breaks compatibility. Now we either have to patch your extension, or make a local copy removing final.

Please, remove final from viewhelpers because it does not add any value but only restricts inheritance.

This is about typo3v12 branch.

Thanks!

s2b commented 1 year ago

The problem is that with TYPO3 v12 declaring the core ViewHelpers as final, they can introduce breaking changes in-between major versions, see:

https://docs.typo3.org/c/typo3/cms-core/main/en-us/Changelog/11.5/Important-95298-FluidViewhelpersWillBeDeclaredFinalInV12.html

My intention was to stay compatible with TYPO3 core, which means that the internal implementation of the ViewHelpers can change and thus they should not be extendable.

What is your use case to extend the included ViewHelpers? Maybe this could be added to the extension or could be solved with a better API (e. g. Signal/Slot)?

dmitryd commented 1 year ago

We add support for webp and make images to use webp by default by making fileExtrension default to webp. I do not think this would be generally universal change.

If core decided to declare all viewhelpers final and thus disallow people to build upon core viewhelpers, it only will cause copying of those viewhelpers to "user space" and prevent people from using proper OOP. It will also make harder to fix any kind of security issues inside copied viewhelpers. Bad decision in my opinion...

dmitryd commented 1 year ago

Added a TYPO3 ticket: https://forge.typo3.org/issues/100196

s2b commented 1 year ago

We add support for webp and make images to use webp by default by making fileExtrension default to webp. I do not think this would be generally universal change.

Would it help to add a PSR-14 Event to the initializeArguments() method? Then you could use overrideArgument() to set webp as the default file extension.

dmitryd commented 1 year ago

Well, we solved it now another way. Let's hope core revises its decision...

s2b commented 1 year ago

Ok. Just let me know if I should add the event or something similar.

LeoniePhiline commented 1 year ago

For avif / webp support, simply configure nginx to inspect the Accept-Encoding HTTP header and respond with an avif / webp image for every jpeg / png request.

All you need is image.jpg, image.jpg.avif and image.jpg.webp next to each other in your file tree. (Or put other image encodings into different directories - as you like.)

Prefer avif for all browsers that support it. Fall back to webp for some. Fall back to jpg for the rest. No need to edit any viewhelpers.

You can set up scripts like these as systemd services:

AVIF conversion with file watcher (using https://github.com/kornelski/cavif-rs/):

echo "Setting up AVIF conversion watcher.";
inotifywait -q -m -r --format '%e %w%f' -e close_write -e moved_from -e moved_to -e delete $1 \
| grep -i -E '\.(jpe?g|png)$' --line-buffered \
| while read operation path; do
  avif_path="$path.avif";
  if [ $operation = "MOVED_FROM" ] || [ $operation = "DELETE" ]; then # if the file is moved or deleted
    if [ -f "$avif_path" ]; then
      rm -f "$avif_path";
    fi;
  elif [ $operation = "CLOSE_WRITE,CLOSE" ] || [ $operation = "MOVED_TO" ]; then  # if new file is created
     if [ $(grep -i '\.png$' <<< "$path") ]; then
       cavif --speed=3 --quality=80 --threads 1 --overwrite "$path" -o "$avif_path";
     else
       cavif --speed=3 --quality=65 --threads 1 --overwrite "$path" -o "$avif_path";
     fi;
  fi;
done;

WebP conversion with file watcher (using https://developers.google.com/speed/webp/docs/cwebp):

echo "Setting up WEBP conversion watcher.";
inotifywait -q -m -r --format '%e %w%f' -e close_write -e moved_from -e moved_to -e delete $1 \
| grep -i -E '\.(jpe?g|png)$' --line-buffered \
| while read operation path; do
  webp_path="$path.webp";
  if [ $operation = "MOVED_FROM" ] || [ $operation = "DELETE" ]; then # if the file is moved or deleted
    if [ -f "$webp_path" ]; then
      rm -f "$webp_path";
    fi;
  elif [ $operation = "CLOSE_WRITE,CLOSE" ] || [ $operation = "MOVED_TO" ]; then  # if new file is created
     if [ $(grep -i '\.png$' <<< "$path") ]; then
       cwebp -mt -lossless "$path" -o "$webp_path";
     else
       cwebp -mt -q 75 "$path" -o "$webp_path";
     fi;
  fi;
done;

You can also convert your images in one go for initial setup (take - requires lots of CPU):

AVIF:

# converting JPEG images
find $1 -type f -and \( -iname "*.jpg" -o -iname "*.jpeg" \) \
-exec bash -c '
avif_path="$0.avif";
if [ ! -f "$avif_path" ]; then
  sleep $[ ( $RANDOM % 10 )  + 1 ]s;
  cavif --speed=3 --quality=65 --threads 1 "$0" -o "$avif_path";
fi;' {} \;

# converting PNG images
find $1 -type f -and -iname "*.png" \
-exec bash -c '
avif_path="$0.avif";
if [ ! -f "$avif_path" ]; then
  sleep $[ ( $RANDOM % 10 )  + 1 ]s;
  cavif --speed=3 --quality=80 --threads 1 "$0" -o "$avif_path";
fi;' {} \;

WEBP:

# converting JPEG images
find $1 -type f -and \( -iname "*.jpg" -o -iname "*.jpeg" \) \
-exec bash -c '
webp_path="$0.webp";
if [ ! -f "$webp_path" ]; then
  sleep $[ ( $RANDOM % 10 )  + 1 ]s;
  cwebp -q 75 "$0" -o "$webp_path";
fi;' {} \;

# converting PNG images
find $1 -type f -and -iname "*.png" \
-exec bash -c '
webp_path="$0.webp";
if [ ! -f "$webp_path" ]; then
  sleep $[ ( $RANDOM % 10 )  + 1 ]s;
  cwebp -lossless "$0" -o "$webp_path";
fi;' {} \;

Configure nginx:

/etc/nginx/conf.d/avif.conf

map $http_accept $avif_suffix {
    default ".no-avif"; # try_files fails for non-existent file.jpg.no-avif
    "~*avif" ".avif";
}

/etc/nginx/conf.d/webp.conf

map $http_accept $webp_suffix {
    default ".no-webp"; # try_files fails for non-existent file.jpg.no-webp
    "~*webp" ".webp";
}

In http { server { ... } } section:

        location /fileadmin/_processed_ {
            location ~ \.(png|jpe?g)$ {
                add_header Vary "Accept-Encoding";
                log_not_found off;
                expires max;
                try_files $uri$avif_suffix $uri$webp_suffix $uri =404;
            }
        }

This serves avif if accepted, else webp if accepted, else original png or jpg.

Enjoy your 100% Google Lighthouse rating.

dmitryd commented 1 year ago

@LeoniePhiline This requires additional packages on the server, which is not the easiest thing to do in our environment. TYPO3 GraphicsMagic is capable of making webp, so we can simply use standard TYPO3 functions without any external packages. avif - yes, good, but gm does not support it and it takes im 39 times longer to write an avif file than a webp file (useless).

So for now we prefer to stay with webp. This is the most practical at the moment.

But the code is very interesting, I will see what we can reuse. Thanks for tips!