I need help with a shortcode question.

I have this code:

add_shortcode("archive", "archive_render");
function archive_render($atts) {

   extract(shortcode_atts(array(
      "rientro" => "no",
      "year" => "",
   ), $atts));

   global $wpdb;

   $rientro == "si" ? $rientro = "yes" : "no";

   $query = "SELECT event_name FROM wp_em_events WHERE EXTRACT(YEAR FROM event_end_date) = ".$year." AND event_end_date < CURDATE()";
   $pasts_event = $wpdb->get_col($query);

   function get_pasts_event( $pasts_event ){

      foreach ( $pasts_event as $past_event_slug ) {
         $output .= "<li><a href='".get_site_url()."/eventi/".$past_event_slug."'>$past_event_slug</a></li>";
      }

      return $output;
   } 

   $string = '[one_third last="'.$rientro.'" class="" id=""][accordian class="" id=""][toggle title="'.$year.'" open="no"]<ul>'.get_pasts_event($pasts_event).'</ul>[/toggle][/accordian][/one_third]';

   echo do_shortcode( $string );
}

If I add it twice or more in the WP editor, only the first will execute and the page stops working.

Anybody have any idea why my shortcode can't be used more than once?

  • I would advise against the use of extract, and recommend you indent consistently so that yourself and people reading your question can read the code easier – Tom J Nowell Jan 16 '15 at 17:03
  • 1
    Also I may be misreading but you declare a function inside another function? This is bad – Tom J Nowell Jan 16 '15 at 17:04
  • I'm here to learn :) – Daniele Pavinato Jan 16 '15 at 21:24
  • Why do you advise me of using extract? – Daniele Pavinato Jan 16 '15 at 21:26
  • Using extract pulls all items out into local variables, even those you do not know are there. By doing what I show below, you pull them into a variable as an array. If you are working in short functions in your own code, extract isn't too bad... but if you are working in the global namespace, on large functions, or on projects with other people, avoiding extract helps prevent hard to debug accidental variable overwriting. – Privateer Jan 16 '15 at 22:50

I believe the real issue you had was with echoing your data instead of returning it.

A shortcode should return the output to be drawn, which is then drawn in place of the short code.

Here is the code using HEREDOC syntax for outputs and the query. I believe this makes things much more readable once you get used to it. See http://php.net/manual/en/language.types.string.php#language.types.string.syntax.heredoc

Changes to get_past_events:

  1. Moved it out of the other function (for clarity)
  2. Provided a default input variable (for clarity of input expectations)
  3. Skip processing if no events received
  4. Moved get_site_url() call out of loop (same for every item)
  5. Used HEREDOC notation (for clarity and clean output)
  6. Renamed the function to make it less likely to conflict

The get_past_events function:

function do_my_get_past_events( $past_events = array() ) {
   $output = '';

   if ( is_array($past_events) && 0 < count($past_events) ) {
      $site_url = get_site_url();
      foreach ( $past_events as $past_event_slug ) {
         $output .= <<<HTML
<li><a href="{$site_url}/eventi/{$past_event_slug}">{$past_event_slug}</a></li>

HTML;

      }
   }
   return $output;
}

Changes to archive_render:

  1. Pull shortcode attributes into an array (clarity)
  2. Declare event_items (so we can skip processing if needed)
  3. Added prepare statement (prevent sql injection) using digit
  4. Used HEREDOC for query string (clarity)
  5. Adjusted rientro variable (left hand declaration - clarity)
  6. Changed string variable to custom_short_code (clarity)
  7. Return the final output (the actual fix)
  8. Added optional block in case a default is wanted
  9. Renamed the function to make it less likely to conflict with anything

The archive_render function:

add_shortcode("archive", "do_my_custom_archive_render");
function do_my_custom_archive_render($atts) {

   $options = shortcode_atts(
      array(
         'year' => 0,
         'rientro' => 'no'
      ),
      $atts
   );

   $past_event_items = '';

   if ( !empty( $options['year'] ) ) {
      global $wpdb;

      $query = <<<SQL
 SELECT event_name 
 FROM wp_em_events 
 WHERE 
   EXTRACT(YEAR FROM event_end_date) = %d 
   AND event_end_date < CURDATE()
SQL;

      $query = $wpdb->prepare( $query, (int) $options['year'] );

      $past_events = $wpdb->get_col($query);

      $past_event_items = do_my_get_past_events( $past_events );
   }

   $rientro = ( 'si' == "{$options['rientro']}" )? 'yes' : 'no';

   # If there are no event items for the year provided the user will get an empty box
   # If that is not what is wanted, uncomment the following 3 lines
   # if ( empty($past_event_items) ) {
   #    $past_event_items = '<li>No Events Found</li>';
   # }

   $custom_short_code = <<<HTML
[one_third last="{$rientro}" class="" id=""]
   [accordian class="" id=""]
      [toggle title="{$options['year']}" open="no"]
         <ul>
            {$past_event_items}
         </ul>
      [/toggle]
   [/accordian]
[/one_third]
HTML;

   return do_shortcode( $custom_short_code );
}

Here is the same code without using HEREDOC for those unfamiliar with it.

function do_my_get_past_events( $past_events = array() ) {
   $output = '';

   if ( is_array($past_events) && 0 < count($past_events) ) {
      $site_url = get_site_url();
      foreach ( $past_events as $past_event_slug ) {
         $output .= '<li><a href="' . $site_url . '/eventi/' . $past_event_slug . '">' . $past_event_slug . '</a></li>' . "\r\n";
      }
   }

   return $output;
}

add_shortcode("archive", "do_my_custom_archive_render");
function do_my_archive_render($atts) {

   $options = shortcode_atts(
      array(
         'year' => 0,
         'rientro' => 'no'
      ),
      $atts
   );

   $past_event_items = '';

   if ( !empty( $options['year'] ) ) {
      global $wpdb;

      $query = "SELECT event_name FROM wp_em_events WHERE EXTRACT(YEAR FROM event_end_date) = %d AND event_end_date < CURDATE()";

      $query = $wpdb->prepare( $query, (int)$options['year'] );

      $past_events = $wpdb->get_col($query);

      $past_event_items = do_my_get_past_events( $past_events );
   }

   $rientro = ( 'si' == "{$options['rientro']}" )? 'yes' : 'no';

   # Uncomment the following 3 lines if you want.
   # if ( empty($past_event_items) ) {
   #    $past_event_items = '<li>No Events Found</li>';
   # }

   $custom_short_code = '[one_third last="' . $rientro . '" class="" id=""][accordian class="" id=""][toggle title="' . $options['year'] . '" open="no"]<ul>' . $past_event_items . '</ul>[/toggle][/accordian][/one_third]';

   return do_shortcode( $custom_short_code );
}
  • Thanks for your help. A questions: 1) why moving function outside? – Daniele Pavinato Jan 16 '15 at 10:31
  • I just find the code cleaner to look over if functions are not declared inside of each other. The only time I declare one in another is when using include and the included file declares one or more functions. Cleaner looking code is typically easier to debug. – Privateer Jan 16 '15 at 15:48
  • 1
    You shouldn't declare functions inside of other functions, aside from being easier to read and debug, it can cause issues with code intel and other tools designed to catch bugs or help coders – Tom J Nowell Jan 16 '15 at 17:06
  • Thank for the precious help. Inside HEREDOC, why using curly brackets with php variables? – Daniele Pavinato Jan 16 '15 at 21:29
  • Force of habit mostly. Some types of variables (like array entries such as $var[something]) require the curly braces around them. Rather than run into problems on occasion I just always use them so that I don't forget them when actually needed. – Privateer Jan 16 '15 at 22:46

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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