[wp-meta] [Making WordPress.org] #205: Add a UI for adding a session time on "add session" pages

[Making WordPress.org]

#205: Add a UI for adding a session time on "add session" pages
--------------------------+--------------------------------------
  Reporter:  melchoyce    |      Owner:  iandunn
      Type:  enhancement  |     Status:  accepted
  Priority:  normal       |  Component:  wordcamp.org
Resolution:               |   Keywords:  good-first-bug has-patch
--------------------------+--------------------------------------

Comment (by iandunn):

 Thanks Nowell, the UI from the patch is a big improvement :)

 I am seeing a bug while testing on my sandbox, though; when I load a post
 that already has a time saved, the date/time is displayed correctly, but
 if I change the time and save it, the wrong date/time is shown after the
 refresh. It looks like the meta field is being saved as an empty string
 instead of the new time.

 Let me know if you can't reproduce that on your sandbox and I'll dig into
 it to figure out what's going on.

 The rest of the things I noticed are just minor tweaks:

 * Normally it's best to keep any JavaScript and CSS in external files to
 keep the separation of concerns and maintainability, but since there's
 just a small snippet for this, it's not really worth the extra HTTP
 request. I think it'd be a little better to just have an
 `admin_print_footer_scripts` callback function that prints it.
 * `jquery-ui.min.css` should use `1.11.2` as the cachebust parameter to
 `wp_enqueue_scripts()`, since it's an external project with an actual
 version. Cachebusters like `20141028` are just used when we don't have a
 version to tie it to.
 * We should add a note to the line that enqueues `jquery-ui.min.js` to
 remind us that we won't need to bundle it anymore once it's included in
 Core in wordpress:#18909
 * `printf()` is a simpler way of doing `echo sprintf()`
 * It'd good to use `esc_attr()` and the other escaping functions, even
 when you know the value is your outputting is safe (like the hardcoded
 `1-12` for hours). It may be a bit overkill, but there are a few benefits:
   * 1) It saves time for other devs who are looking at the code, because
 they don't have to trace the value back to where it's assigned to make
 sure it's safe to echo without escaping; and
   * 2) It protects against situations where the value is changed to
 something unsafe in the future and the developer doesn't realize they now
 need to escape the value. It's not a huge deal, but it's a good habit to
 get into.
 * The meridiem `select` options can just be hardcoded instead of having a
 loop, since there's only 2 of them. I think they'd be a bit more readable
 that way.
 * I like using `sprintf()` in many situations, but when creating the
 markup I think it'd be more readable output the HTML and use inline PHP:
 {{{
 <select name="wcpt-session-hour" aria-label="Session Start Hour">
         <?php for ( $i = 1; $i <= 12; $i++ ) : ?>
                 <option value="<?php esc_attr( $i ); ?>" <?php selected(
 $i, $session_hours ); ?>>
                         <?php echo esc_html( $i ); ?>
                 </option>
         <?php endfor; ?>
 </select>
 }}}
 * When saving the new time, we're only using the input values once, so we
 don't really need to store them in variables, which makes it a bit
 cleaner:
 {{{
 $session_time = strtotime( sprintf(
         '%s %d:%d:00%s',
         sanitize_text_field( $_POST['wcpt-session-date'] ),
         sanitize_text_field( $_POST['wcpt-session-hour'] ),
         sanitize_text_field( $_POST['wcpt-session-minutes'] ),
         sanitize_text_field( $_POST['wcpt-session-meridiem'] )
 ) );
 }}}
 * It shouldn't be an issue in this situation, but as a general rule, it's
 good to use `absint()` instead of `sanitize_text_field()` when expecting a
 number. In some situations, malicious input could be crafted in a way that
 exploits the fact that the input is evaluated as a string.

--
Ticket URL: <https://meta.trac.wordpress.org/ticket/205#comment:6>
Making WordPress.org <https://meta.trac.wordpress.org/>
Making WordPress.org