WordPress - Media Upload

There are at least 2 design problems in the media upload section

It allows an edit link on images that the user does not have permission to edit
When you click the link, it shows 500 - Internal server error.

In total, I had to modify 3 core WordPress files to fix these problems.

Incorrect "edit" link | Basic Fix | Better Fix | Why two files | 500 - Internal server error

Incorrect "edit" link

There are 2 failures in upload.php (and edit-attachment-rows.php) when the user does not have permission to edit the attachment.

The first places an edit link around the icon
The second places an empty link around the attachment title

The code fragments shown below are modified to improve readability.

This is the code that places a link around the icon. Notice that there is no test for user permissions.

<td class="media-icon"><?php
if ( $thumb = wp_get_attachment_image( $post->ID, array(80, 60), true ) ) { ?>
  <a href="media.php?action=edit&attachment_id=<?php echo $post->ID; 
   ?>" title="<?php echo esc_attr(sprintf(__('Edit “%s”'), $att_title)); ?>"><?php echo $thumb; ?></a>
<?php	} ?></td>

This code places an "edit" link around the image title.

<td class="media column-media"><strong><a href="<?php echo get_edit_post_link( $post->ID ); 
  ?>" title="<?php echo esc_attr(sprintf(__('Edit “%s”'), $att_title)); 
  ?>"><?php echo $att_title; ?></a></strong><br />

get_edit_post_link() checks that the user is able to edit the post and returns either the "correct" string or null. However, the link should not even exist if the action is not permitted.

And this is the code that determines what links should be displayed under the title.

if ( current_user_can('edit_post', $post->ID) )
  $actions['edit'] = '<a href="' . get_edit_post_link($post->ID, true) . '">' . __('Edit') . '</a>';

if ( current_user_can('delete_post', $post->ID) )
  $actions['delete'] = "<a class='submitdelete' href='" . wp_nonce_url("post.php?action=delete&post=$post->ID",
     'delete-post_' . $post->ID) . "' onclick=" code not shown ">" . __('Delete') . "</a>";

$actions['view'] = '<a href="' . get_permalink($post->ID) . 
   '" title="' . esc_attr(sprintf(__('View “%s”'), $title)) . '" rel="permalink">' . __('View') . '</a>';

The important point is the test to see if the person is allowed to edit the "post" before adding the link. This test should have also been added to the icon.

The same fixes had to be placed in both files (great design).

Basic Fix

Normally, I never delete other peoples code - but I do comment it out so it can be used as a reference. However, there is no way I know to comment out a block of code that hops into and out of php mode. (I tried - it produced a lot of errors.) As a result, I decided not to leave in the original code.

The first thing I noticed was that the code checked to see if the current user had permission to edit the information more than 4 times. Well, that made no sense at all. Therefore, I rewrote the code to check for permission only once and stored the result in a local variable ($edit_link). Basically, get_edit_post_link performs the check and returns an appropriately formatted "edit link" (if the permissions are correct) or it returns a null string (if they are not).

Long statements should not be repeated - it is better to save the result is a variable and then just reuse the variable (if for no other reason than it reduces the chance for error). It takes 3 subroutine calls to produce the string stored in $edit_title. In the original code, this identical string was created 3 separate times ... when only one is enough, and it speeds up the code.

The following code was placed inside the loop right after $att_title was defined.

// code fixed 09-01-2009 by rlc
    $edit_link  = get_edit_post_link( $post->ID );                            // this is used 3 times
    $edit_title = esc_attr(sprintf(__('Edit “%s”'), $att_title)); // this is used 3 times

The existing code switches in and out of php-mode many times and, therefore, is very hard to read. The following places the icon (if it exists) in a table column.

	case 'icon':
		$attributes = 'class="column-icon media-icon"' . $style;
		?>
		<td <?php echo $attributes ?>><?php
			if ( $thumb = wp_get_attachment_image( $post->ID, array(80, 60), true ) ) {

// code fixed 09-01-2009 by rlc

  if ($edit_link){
     echo "<a href=\"{$edit_link}\" title=\"{$edit_title}\">{$thumb}</a>";
  } else {
     echo $thumb;
  }
}
		?></td>
		<?php
		// TODO
		break;

	case 'media':
		?><td <?php echo $attributes ?>><strong><?php 

  if ($edit_link){
     echo "<a href=\"{$edit_link}\" title=\"{$edit_title}\">{$att_title}</a>";
  } else {
     echo $att_title;
  }

The rest of the code was not changed.

There is a similar design error associated with the parent post (if any) which I did not fix.

Better Fix

The code is way more complicated than necessary.

There is no reason at all to make the icon or the title clickable
upload.php and edit-attachment-rows.php contain a lot of the same code and both need exactly the same patches
Those stupid “double quotes” (“ & ”) should not be used. "Normal double quotes" are fine and are easy to search for.
Refactoring (creating subroutines) would make this a lot simpler

The following places the icon (if it exists) in a table column (as before), but does not make it clickable. (What's the point?) The second case condition displays the attachment title and also does not make it clickable. Since neither of these can be clicked, there is no reason for the fancy quoted title and that is removed.

  case 'icon':
    $attributes = 'class="column-icon media-icon"' . $style;
    ?>
    <td <?php echo $attributes ?>><?php
    if ( $thumb = wp_get_attachment_image( $post->ID, array(80, 60), true ) ) {
       echo $thumb;  // code fixed 09-01-2009 by rlc
    }
    ?></td><?php
    // TODO
    break;

  case 'media':
    ?><td <?php echo $attributes ?>><strong><?php 
    echo $att_title;  // code fixed 09-01-2009 by rlc

This is also in the code.

if ( current_user_can('edit_post', $post->ID) )
  $actions['edit'] = '<a href="' . get_edit_post_link($post->ID, true) . '">' . __('Edit') . '</a>';

It should be replaced with

$edit_link  = get_edit_post_link( $post->ID );
if ( $edit_link )
  $actions['edit'] = "<a href=\"{$edit_link}\">" . __('Edit') . '</a>';

(Using double quotes permits variable substitution ... and, therefore, requires escaping double quotes desired in the final string.)

Why two files

upload.php and edit-attachment-rows.php are similar and different. The primary differences I see are

One uses orphan attachments and the other uses normal attachments (what ever that means)
The classes are different

This is from edit-attachment-rows.php when defining the styles used on icons

$style = '';
if ( in_array($column_name, $hidden) )
  $style = ' style="display:none;"';    // This supports hidden columns

$attributes = "$class$style";

switch($column_name) {

  case 'icon':
    $attributes = 'class="column-icon media-icon"' . $style;
    ?>
    <td <?php echo $attributes ?>><?php

and this is the similar code from upload.php (which is just straight code, no case structure).

<td class="media-icon"><?php

It is hard to tell if this is an intentional design difference, or simply an error. (Opinions may differ.)

Otherwise, they contain a lot of the same code and both need exactly the same patches. (This is almost the definition of a bad design.)

Note: Orphan attachments do not have parents - parent ID < 1.

500 - Internal server error

I found the design problems presented on this page because clicking icons on the Library (attachments) page produced 500 - Internal server error's with absolutely no useful information.

Details of this design problem are presented on their own page. Basically, there is a design problem (feature) in Internet Explorer 6 that throws away useful information when a server error is indicated.

In my opinion, the WordPress developers should have provided a useful (and user friendly) work around for this problem ... but decided to ignore it. (That's right, they knew about the problem and decided not to do anything about it.)

My page contains a partial fix - it works for the problems I have seen. Not perfect, but better than nothing.

Author: Robert Clemenzi