3
\$\begingroup\$

I'm learning and want to improve as much as possible. In doing so I have written this custom block that has an image on one side and an inner block on the other. It's using ACF to get the variables and comments should state most of the items.

I'm looking for recommendations to improve, readability, functionality, or security issues I may not have addressed. I made an image function that should be responsive. Everything functions fine but I'd like to make it cleaner and faster.

Looking forward to seeing recommendations - I want to improve in any way possible!

First off the block file

<?php
/**
 * Two Column Block with Image & Innerblock
 * 
 * A two column block with innerblock on one side and image on the other
 * options of 50/50 and 40/60 sizes available
 * Can switch content sides and image is always on top
 * 
 * @param function wg_child_acf_image_block() to return images output
 * @author Andrew
 */

$padding          = get_field( 'padding' );                        //Padding 
$top              = get_field( 'top' );                            //Custom padding top
$bottom           = get_field( 'bottom' );                         //Custom Padding bottom
$image            = get_field( 'image' );                          //Image field
$sizing           = get_field( 'sizing' ) ?: 'fiftyFifty';         //Select field that sets sizes to 50/50 or 40/60
$side             = get_field( 'side' ) ?: 'normal';               //Switch Content Sides
$combined         = $sizing . ' ' . $side;                         //Combined for output
$image_fallback   = 'wg-align-image-left';                         //Set Default Image alignment
$inlineStyle      = '';                                            //Create variable for optional custom padding
$anchor           = '';                                            //Create variable for custom anchor option
$center           = '';                                            //Create variable for align-self innerblock

//Change default image alignment if reversed
if( $side == 'reverse' ) {
    $image_fallback = 'wg-align-image-right';
}

//Alignment of image w/ fallback
$align_image  = get_field( 'alignimage' ) ?: $image_fallback;  

//Determining Padding For Block - default is else
if( 'narrow' == $padding ) {
    $padding = 'blockPadding-narrow';
} elseif( 'none' == $padding ) {
    $padding = 'blockPadding-none';
} elseif( 'custom' == $padding ) {
    $inlineStyle = 'style="padding-top: ' . $top . 'vh; padding-bottom: ' . $bottom . 'vh;"';
    $padding = false;
} else {
    $padding = 'blockPadding';
}

//Determines if padding is a class or inline and created classes array
if( $padding ) {
    $classes = [$padding];
} else {
    $classes = [];
}

//Adds custom class if provided
if( !empty( $block['className'] ) )
    $classes = array_merge( $classes, explode( ' ', $block['className'] ) );

//Adds custom anchor if provided
if( !empty( $block['anchor'] ) )
    $anchor = ' id="' . sanitize_title( $block['anchor'] ) . '"';

//Default data that goes inside innerblock when created but can be removed
$template = array(
    array('core/heading', array(
        'content' => 'Title Text Goes Here',
    )),
    array( 'core/paragraph', array(
        'content' => 'Enter in your paragraph text here for further information',
    ) )
);

//Option to turn off align-self: center;
if( get_field( 'center' ) == false ) {
    $center = ' center-self';
}

//Opening Div - determines if class or inline for padding
if( $inlineStyle ) {
    echo '<section class="' . join( ' ', $classes ) . '"' . esc_attr($anchor) . ' ' . esc_attr($inlineStyle) . '>';
} else {
    echo '<section class="' . join( ' ', $classes ) . '"' . esc_attr($anchor) . '>';
}

?>
    <div class="wrapper">
        <div class="twoColumns <?php echo esc_attr($combined); ?>">
            <div class="leftSide">
                <div class="imageWrap">
                    <?php //arguments are image array, optional image class, optional image size ?>
                    <?php wg_child_acf_image_block( $image, $align_image ); ?>
                </div>
            </div>
            <div class="rightSide<?php if($center) { echo esc_attr($center); } ?>">
                <?php echo '<InnerBlocks template="' . esc_attr( wp_json_encode( $template ) ) . '" />'; ?>                
            </div>
        </div>
    </div>

<?php
//closing div 
echo '</section>';
?>    

Next is the image function which is used in the above file:

<?php
/**
 * Function to call ACF Image array and convert it to a SEO friendly image result
 *
 * Image function that takes an image array from ACF (or other similiar plugins) and 
 * converts it into a responsive image.
 * Required argument of $image which is the array
 * Optional argument of $imageClass to pass an optional class
 * Optional argument of $imgsize that passes a different size iamge
 *
 * @param array $image uses ACF image array
 * @since 1.0.0
 * @author: Andrew
 */

function wg_child_acf_image_block($image, $imageClass = '', $imgsize = 'large') {

    if( $image ) {

    // Image attribuates.
    $url         = $image['url'];
    $title       = $image['title'];
    $alt         = $image['alt'] ?: $title;
    $caption     = $image['caption'];
    $imgsize     = 'large';

    //adds w on the end of sizes
    $w           = 'w';  

    // Image sizes and src sets.
    $thumb       = $image['sizes'][ $imgsize ];
    $width       = $image['sizes'][ $imgsize . '-width' ];
    $height      = $image['sizes'][ $imgsize . '-height' ];
    $medlg       = $image['sizes']['medium_large'];
    $medlg_width = $image['sizes']['medium_large-width'] . $w;
    $med         = $image['sizes']['medium'];
    $med_width   = $image['sizes']['medium-width'] . $w;
    $tn          = $image['sizes']['thumbnail'];
    $tn_width    = $image['sizes']['thumbnail-width'] . $w;    
    
    //Returned Result
    $result = '';

    // Begin caption wrap.
    if( $caption ):
        $result .= '<div class="wp-caption">';
    endif;

    //img item
    $result .= '<img src="' . esc_url($thumb) . '" srcset="' . esc_url($medlg) .' '. esc_attr($medlg_width) . ', ' . esc_url($med) . ' ' .  esc_attr($med_width) . ', ' . esc_url($tn) . ' ' .  esc_attr($tn_width) . '" alt="' . esc_attr($alt) . '" width="' . esc_attr($width) . '" height="' . esc_attr($height) . '" title="' . esc_attr($title) . '" class="' . esc_attr($imageClass) . ' wg-image-class" />';

    // End caption wrap.
        if( $caption ):
            $result .= '<p class="wp-caption-text">' . esc_html($caption) . '</p>';
            $result .= '</div>';
            ?>
        <?php endif; ?>
    
    <?php 
    //end if($image) statement
    } else {
        //Fallback image if no image
        $result = '<img src="' . get_stylesheet_directory_uri() . '/assets/images/filler.jpg' . '" alt="Filler Image" class="' . esc_attr($imageClass) . ' wg-image-class">';
    }

    //Output the image
    echo $result;
}
\$\endgroup\$
1
  • \$\begingroup\$ it would be nice to group code into functions or into classes to make it easier to understand what is going on. \$\endgroup\$ Commented Nov 7, 2021 at 9:25

2 Answers 2

3
\$\begingroup\$

Regarding readability/maintainability, my eye is drawn toward missed opportunities to practice D.R.Y. coding practices. I will suggest ways to make less verbose/repeated code, avoid heavily concatenated strings, and minimize the total number of variables declared.

The adjustments to padding are all checking the same variable; this makes a switch() (or match() if you are using the latest and greatest PHP version) appropriate. I also don't see any benefit to overwriting $padding only to conditionally populate the $classes variable further down the script. In fact, with my snippet below, $padding becomes a single-use variable and this indicates that it shouldn't be declared at all -- just use get_field('padding').

$classes = [];
switch(get_field('padding')) {
    case 'narrow':
        $classes[] = 'blockPadding-narrow';
        break;
    case 'none':
        $classes[] = 'blockPadding-none';
        break;
    case 'custom':
        $inlineStyle = 'style="padding-top: ' . $top . 'vh; padding-bottom: ' . $bottom . 'vh;"';
        break;
    default:
        $classes[] = 'blockPadding';
}

Next, I don't see anywhere in the first snippet where the $block array is declared. This makes it very hard for me to review its relevance. I'll just mention that if you are going to write a condition block, always obey PSR-12 coding standards and use curly braces to encapsulate your condition body. This makes your code far less vulnerable to typos/oversights.


The handling of $center could be cleaned up. You declare it as an empty string, then you check if get_field('center') is loosely false, then in your html you are only conditionally printing a value. I think the preparation should be closer to the top and consolidated/simplified.

$center = get_field('center') ? '' : ' center-self'; // Option to turn off align-self: center;

As for the remaining HTML generation, I recommend using printf() to avoid concatenation. If you get lost counting the %s placeholders, you can number them (e.g. %1$s, %2$s, etc.).

$template = <<<HTML
<section class="%s"%s%s>
    <div class="wrapper">
        <div class="twoColumns %s">
            <div class="leftSide">
                <div class="imageWrap">%s</div>
            </div>
            <div class="rightSide%s">
                <InnerBlocks template="%s" />
            </div>
        </div>
    </div>
</section>
HTML;
printf(
    $template,
    implode(' ', $classes),
    esc_attr($anchor),
    $inlineStyle ? ' ' . esc_attr($inlineStyle) : '',
    esc_attr($combined),
    wg_child_acf_image_block($image, $align_image), //arguments are image array, optional image class, optional image size
    $center ? esc_attr($center) : '',
    esc_attr(wp_json_encode($template))
);

I'll curb the urge to repeat myself and keep this answer D.R.Y. Suffice to say that the same general guidance for your first snippet can be re-applied to your second snippet.

\$\endgroup\$
2
\$\begingroup\$
  1. Imagine, instead of having a long list of code within the block file, to read:

    setDefaultImage()
    setAlignment()
    setPadding()
    
    //...
    

    As a reviewer, I would see the key parts of what is happening without having to think about specific code. Should I want to know what specific function does, I can click into it.

  2. The long list of variables can also be extracted into parameter objects - which can then be re-used in other files.

    i.e.

    $padding          = get_field( 'padding' );                        //Padding 
    $top              = get_field( 'top' );                            //Custom padding top
    $bottom           = get_field( 'bottom' );    
    
    // ...
    

    Alternative

    class ComponentParameters {
      string $padding;
      string $top;
      string $bottom;
    
      pubilc function toArray(): array {
        // ...
      }
    }
    
    

    You can use PHP's built in function to extract array keys to variables.

  3. You keep mixing HTML and PHP in the code. I am referring to use of <?php endif; ?>

  4. How do you know that $image will have correct array format? What if someone makes a typo when calling that function?

  5. The code under //img item is a very long string. Which requires reviewer to scroll to the right to see what's happening further on the right. This slows review down by getting reviewer to think about scrolling to the right instead of building logic in the head.

  6. How do you know that this code works? I don't know if you have it already, but it would be worth adding at least one test to make your code future-proof. Just checking for inputs and outputs would make it easier to refactor and more stable.

  7. You can use early returns to prevent arrow pattern.

    i.e.

    
    function wg_child_acf_image_block(...) { 
      if (!$image) {
        return '<img src="' . get_stylesheet_directory_uri() . '/assets/images/filler.jpg' . '" alt="Filler Image" class="' . esc_attr($imageClass) . ' wg-image-class">';
      }
    
      // ...
    }
    

Those are just some suggestions. The scale of refactoring is dependent on the scale of your application. If it's a large scale critical code, you may want to refactor even further. However, majority of WordPress packages look similar to what you've written.

Also, WordPress is not so friendly towards latest practices on how to do both OOP and Functional code, nor does it support the latest PHP functionality due to its core, still stuck within PHP 5.*. There are some hacky ways around it, and maybe there are now out-of-the box solutions for this (the last time I worked with WordPress was over a year ago).

\$\endgroup\$

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.