media_handle_upload creates two entries

  • Resolved jester48

    (@jester48)


    3 months, 3 weeks ago

    I have the following code to upload PDFs to a secure file location, i.e.: outside the public html directory structure. But it creates a duplicate entry and I don’t see where or how it is happening.

    // ajax report upload
    function mycode_report_upload() {

        // set the upload directory
        add_filter( 'upload_dir', 'mycode_support_custom_upload_dir' );
        
        // Security check (nonce verification)
        //if(!check_ajax_referer('mycode_report_upload', 'nonce')){
        //    wp_die('Invalid security check, unable to proceed!');
        //}
        $secure_dir_name = 'mycode_secure_files';
        $report_nice_name =  sanitize_text_field($_POST['report-name']);
        $school_board = sanitize_text_field($_POST['school-board']);
        $report_type = sanitize_text_field($_POST['report-type']);
        $file_date = sanitize_text_field($_POST['mycode-file-date']);
        $target_path = dirname(__DIR__, 5); 
        $secure_dir = $target_path . DIRECTORY_SEPARATOR . $secure_dir_name . DIRECTORY_SEPARATOR . $school_board . DIRECTORY_SEPARATOR . $report_type;
        $base_file_name = ! empty( $_FILES['mycode-file-upload'] ) ?  basename($_FILES['mycode-file-upload']['name']) : '';
        $report_name = ! empty( $_FILES['mycode-file-upload'] ) ? $_FILES['mycode-file-upload']['name'] : ''; 
        $report_name_no_ext = pathinfo($_FILES['mycode-file-upload']['name'], PATHINFO_FILENAME);
        
        if (  ! empty( $_FILES['mycode-file-upload'] ) ) {
        
            if(! is_dir($secure_dir) ){
                if (!mkdir($secure_dir, 0755, true)) {
                    // Force delete the attachment (bypasses the trash)
                    $deleted = wp_delete_attachment( $attachment_id, true );
                    wp_send_json_error('Failed to create directories...');
                    wp_die();
                } 
            }
            
            // check if file already exists
            $file_exists = check_file_already_exists($secure_dir, $base_file_name );

            if(! $file_exists){
                // upload file
                $attachment_id = media_handle_upload( 'mycode-file-upload', 0 );
                // move file to secure directory
                $moved = rename(get_attached_file( $attachment_id ), $secure_dir . DIRECTORY_SEPARATOR . $base_file_name);
                if($moved){
                    chmod($secure_dir . DIRECTORY_SEPARATOR . $base_file_name, octdec(755));
                }
            }else {
                // get existing file id
                $upload_file = $_FILES['mycode-file-upload'];
                $file_data = array(
                    'name'     => $upload_file['name'],
                    'type'     => $upload_file['type'],
                    'tmp_name' => $upload_file['tmp_name'],
                    'error'    => $upload_file['error'],
                    'size'     => $upload_file['size']
                );
                $arr = wp_handle_upload($file_data, array('test_form' => false));
                $moved = rename($arr['file'],   $secure_dir . DIRECTORY_SEPARATOR . $base_file_name);
                if($moved){
                    chmod($secure_dir . DIRECTORY_SEPARATOR . $base_file_name, octdec(755));
                }
                $attachment_id = get_existing_file_id($school_board, $report_type, $report_name_no_ext);
            }
            
            set_post_type( $attachment_id, 'mycode-secured-file' );
            // set attachment details
            $args = array(
                'ID'                => $attachment_id,
                'post_title'        => $report_nice_name,
                'post_content'      => $report_nice_name,
                'comment_status'    => 'closed',
                'post_date'         => date('Y-m-d H:i:s', strtotime($file_date)),
                'post_date_gmt'     => date('Y-m-d H:i:s', strtotime($file_date)),
                'post_status'       => 'secured',
            );
            // update post
            wp_update_post($args);
            
            
            // update meta value - > by attachment id _wp_attached_file
            update_post_meta( $attachment_id, '_wp_attached_file' , $secure_dir . DIRECTORY_SEPARATOR . $base_file_name);
            // add school board and report type post meta
            update_post_meta( $attachment_id, 'mycode_school_board', $school_board);
            update_post_meta( $attachment_id, 'mycode_report_type', $report_type);
            update_post_meta( $attachment_id, 'mycode_file_date', date('Y-m-d H:i:s', strtotime($file_date)));
            update_post_meta( $attachment_id, 'mycode_report_path', $secure_dir . DIRECTORY_SEPARATOR . $base_file_name);
            wp_send_json_success('File Uploaded Successfully!');
        }else{
            wp_send_json_error('File is Empty!');
        }
        
        //remove the upload directory create process
        remove_filter( 'upload_dir', 'mycode_support_custom_upload_dir' );
        wp_die();
    }

    Any advice is welcome, but I mainly want to stop the second record from being generated.

    Thanks

    • This topic was modified 3 months, 3 weeks ago by jester48.
Viewing 3 replies - 1 through 3 (of 3 total)
  • Moderator threadi

    (@threadi)

    3 months, 3 weeks ago

    I can’t see anything in the code that would cause the problem you described. However, it may be related to the rest of the code, which apparently still exists. You are welcome to show us the entire code, but preferably not here in a forum post, but rather on gists or github, for example.

    Nevertheless, a few comments on the code:

    • At the top, you check whether a directory has been created. Then you execute a function to delete an attachment with an $attachment_id that does not exist at all at that point.
    • If you are already using wp_send_json_*() as feedback to the user, you do not need wp_die() afterwards. The remove_filter() at the very bottom will therefore not be executed at all.
    • You are not securing the inputs properly. Although you are using sanitize_* functions, wp_unslash() is still missing in all places. This is not mentioned in the manual, but it is best practice for WordPress coding standards.
    • The variable $report_name is not used at all.
    • Instead of mkdir(), rename(), or chmod(), I would recommend using the WP_Filesystem functions provided by WordPress. This is also part of the WordPress coding standards and is highly recommended. See: https://developer.wordpress.org/reference/functions/wp_filesystem/

    Here’s an additional tip: if you’re not sure why two data records are being created, perform a debugging by using var_dump(); and exit; from start to finish to see what exists and what is being done where in the process. Since it appears to be an AJAX request, it’s worth checking this out in the developer console in your browser.

    • This reply was modified 3 months, 3 weeks ago by threadi.
    Thread Starter jester48

    (@jester48)

    3 months, 3 weeks ago

    thanks for the feedback, much appreciated, i’ve been going in circles and needed a fresh set of eyes.

    Some of the wp_die blocks were removed after posting, I was still testing and forgot to take them out., the file cleanup block I am still trying to figure out, I rearranged the code a few times trying to determine the cause for the double post creation.

    I did var_dumps throughout the code before posting, the duplicate record is created right at the following code block, a var_dump here and and exit then a check of the posts table shows two records created

    // upload file
                $attachment_id = media_handle_upload( 'mycode-file-upload', 0 );
                // move file to secure directory
                $moved = rename(get_attached_file( $attachment_id ), $secure_dir . DIRECTORY_SEPARATOR . $base_file_name);
                if($moved){
                    chmod($secure_dir . DIRECTORY_SEPARATOR . $base_file_name, octdec(755));
                }

    I think i have made all the changes you recommended, code can be seen here: https://gist.github.com/james-gerard-rodgers/34a69cb47f81a5208dfa9043d5cbdc08

    Moderator bcworkz

    (@bcworkz)

    3 months, 3 weeks ago

    You’ve answered your own question without realizing it 🙂 Calling media_handle_upload() creates the default instance regardless of what your code is doing as well.

    I would suggest an entirely different approach. I think you’ll still be able to use some of your existing code, so it’s not a complete start over proposition. I recommend utilizing the “pre_move_uploaded_file” filter. In your callback for this, you’d verify if it’s a file needing secure placement. If not, return null and default processing proceeds normally. If it is a secure placement file, your callback is passed enough information to get the file from the server’s temp directory and to move it to where you desire. You can then return a non-null value to prevent the normal moving of the file to default /uploads/.

    That’ll get the file where you want without duplication, however, the related attachment post will not have proper meta data. In order to correct the meta data, use the “wp_handle_upload” filter to alter path data as necessary.

    BTW, regarding your use of remove_filter(). It is indeed a good technique to ensure your callback is not called more than once per request. That’s fine, but don’t wait until the end of your callback to call this function because it’ll cause a race condition that could render it ineffective. You should call it first thing upon entry into your callback. The rest of the callback will still execute. Doing this early gives the function time to remove your filter hook before it’s called again.

Viewing 3 replies - 1 through 3 (of 3 total)

You must be logged in to reply to this topic.