3
\$\begingroup\$

As a beginner in php, I welcome you feedback on improving or simplifying the following php code that generates this responsive image html (image sizes and formats are auto-generated using Gulp). For example, is having three nested foreach loops bad practice?

Generated HTML:

<figure>
    <picture>
        <source media="(orientation: portrait)" type="image/avif"
            srcset="assets/img/faith/faith-3x4-xs.avif 576w, assets/img/faith/faith-3x4-sm.avif 768w, assets/img/faith/faith-3x4-md.avif 992w, assets/img/faith/faith-3x4-lg.avif 1200w, assets/img/faith/faith-3x4-xl.avif 1400w, assets/img/faith/faith-3x4-xxl.avif 2048w">
        <source media="(orientation: portrait)" type="image/webp"
            srcset="assets/img/faith/faith-3x4-xs.webp 576w, assets/img/faith/faith-3x4-sm.webp 768w, assets/img/faith/faith-3x4-md.webp 992w, assets/img/faith/faith-3x4-lg.webp 1200w, assets/img/faith/faith-3x4-xl.webp 1400w, assets/img/faith/faith-3x4-xxl.webp 2048w">
        <source media="(orientation: portrait)" type="image/jpg"
            srcset="assets/img/faith/faith-3x4-xs.jpg 576w, assets/img/faith/faith-3x4-sm.jpg 768w, assets/img/faith/faith-3x4-md.jpg 992w, assets/img/faith/faith-3x4-lg.jpg 1200w, assets/img/faith/faith-3x4-xl.jpg 1400w, assets/img/faith/faith-3x4-xxl.jpg 2048w">
        <source media="(orientation: landscape)" type="image/avif"
            srcset="assets/img/faith/faith-3x2-xs.avif 576w, assets/img/faith/faith-3x2-sm.avif 768w, assets/img/faith/faith-3x2-md.avif 992w, assets/img/faith/faith-3x2-lg.avif 1200w, assets/img/faith/faith-3x2-xl.avif 1400w, assets/img/faith/faith-3x2-xxl.avif 2048w">
        <source media="(orientation: landscape)" type="image/webp"
            srcset="assets/img/faith/faith-3x2-xs.webp 576w, assets/img/faith/faith-3x2-sm.webp 768w, assets/img/faith/faith-3x2-md.webp 992w, assets/img/faith/faith-3x2-lg.webp 1200w, assets/img/faith/faith-3x2-xl.webp 1400w, assets/img/faith/faith-3x2-xxl.webp 2048w">
        <source media="(orientation: landscape)" type="image/jpg"
            srcset="assets/img/faith/faith-3x2-xs.jpg 576w, assets/img/faith/faith-3x2-sm.jpg 768w, assets/img/faith/faith-3x2-md.jpg 992w, assets/img/faith/faith-3x2-lg.jpg 1200w, assets/img/faith/faith-3x2-xl.jpg 1400w, assets/img/faith/faith-3x2-xxl.jpg 2048w">
        <img src="assets/img/faith/faith-3x2-lg.jpg" width="1200" height="800" alt="Earth Day, Victoria, BC">
    </picture>
</figure>

Php code:

 <?
$image= [
    'name' => 'faith',
    'srcset' =>
        [
        'sizes' =>
                [
                'size' => ['xs','sm','md','lg','xl','xxl'],
                'vw' => [ 576, 768, 992, 1200, 1400, 2048 ]
                ],
        'ratio' => ['portrait' => '3x4','landscape' => '3x2' ],
        'format' => ['avif', 'webp','jpg']
        ],
    'caption' => 'Earth Day, Victoria, BC',
];

$sizes = array_combine($image['srcset']['sizes']['size'], $image['srcset']['sizes']['vw']);
?>

 <figure>
     <picture>
         <?
        foreach ($image['srcset']['ratio'] as $orientation => $ratio):
        $basePath = "assets/img/{$image['name']}/{$image['name']}-{$ratio}";
            foreach($image['srcset']['format'] as $format): $comma = '';
            $source = "<source media=\"(orientation: ${orientation})\" type = \"image/{$format}\" srcset=\"";
                foreach($sizes as $size => $vw):
                $source .= "{$comma}{$basePath}-{$size}.{$format} {$vw}w"; $comma=', ';
                endforeach;
            echo $source .= '">';
            endforeach;
        endforeach;
        ?>
         <img src="<?=$basePath?>-lg.jpg" width="1200" height="800" alt="<?=$image['caption']?>">
     </picture>
 </figure>
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Personally, I don't like that much logic in the template. Even hate it. The code becomes unreadable. In such a case I would rather move all this intricate logic into the PHP part, doing some preprocessing that will result in the plain array, so the end code would be like this

<figure>
    <picture>
<?php foreach ($image['srcset'] as $row): ?>
        <source media="(orientation: <?=$row['orientation']?>)" type="image/<?=$row['format']?>" srcset="<?=$row['srcset'] ?>">
<?php endforeach ?>
        <img src="<?=$imagePath?>lg.jpg" width="1200" height="800" alt="<?=$image['caption']?>">
    </picture>
</figure>

The preprocessing PHP code should be exactly like your current code, just collecting the data into array instead of echoing it

$new = [];
foreach ($image['srcset']['ratio'] as $orientation => $ratio) {
    $basePath = "assets/img/{$image['name']}/{$image['name']}-{$ratio}";
    foreach($image['srcset']['format'] as $format) {
        $srcset = '';
        foreach($sizes as $size => $vw) {
            $srcset .= $srcset ? ",":"";
            $srcset .= "{$basePath}-{$size}.{$format} {$vw}w"; 
        }
        $new[] = [
            'orientation' => $orientation,
            'format' => $format,
            'srcset' => $srcset,
        ];
    }
}

another possibility is to write sort of a helper function

<figure>
    <picture>
        <?php foreach ($image['srcset']['ratio'] as $orientation => $ratio): ?>
            <?php foreach($image['srcset']['format'] as $format): ?>
                <source media="(orientation: <?=$orientation?>)" type="image/<?=$format?>" srcset="<?=getsrcset($image, $ratio, $format, $key)">
            <?php endforeach ?>
         <? endforeach ?>
         <img src="<?=$imagePath?>lg.jpg" width="1200" height="800" alt="<?=$image['caption']?>">
     </picture>
</figure>

And two generic suggestions:

  • short open tag is forbidden, always use <?php instead of <?
  • do not neglect the code indentation. The proper formatting is very important, it helps to understand what does the code do
\$\endgroup\$
6
  • \$\begingroup\$ Thanks for your detailed feedback! \$\endgroup\$
    – Sam Miller
    Aug 1, 2022 at 16:56
  • 1
    \$\begingroup\$ Just use your two nested loops but not to generate HTML, but to create a 1-level array. It should be quite the same. \$\endgroup\$ Aug 1, 2022 at 17:59
  • \$\begingroup\$ Ping me If you don't manage, I'll get you a sketch \$\endgroup\$ Aug 2, 2022 at 3:47
  • 1
    \$\begingroup\$ I already added it, not tested tho \$\endgroup\$ Aug 2, 2022 at 15:55
  • \$\begingroup\$ Works great. I just had to change "<?php foreach ($image['srcset'] as $row): ?>", in our original answer ("end code"), to your $new array, "<?php foreach $new as $row): ?>. Thanks again. I learnt a lot from your answer. \$\endgroup\$
    – Sam Miller
    Aug 2, 2022 at 16:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.