Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add filters for file uploads (prepare and pre upload), update PHPDoc #747

Merged
merged 2 commits into from
Aug 24, 2016

Conversation

tripflex
Copy link
Collaborator

@tripflex tripflex commented Aug 23, 2016

This PR adds two new filters in the file upload handling process.

The first filter job_manager_prepare_uploaded_files is added to the return value from the core function job_manager_prepare_uploaded_files (this I added so it's available if needed later)

The second filter and the most important one is the job_manager_upload_file_pre_upload filter added before the core wp_handle_upload function is called.

This allows us to check the file for certain conditions (file size, dimensions, etc), or anything else that may be required before actually uploading the file, and return our own WP_Error that would then be output on the frontend (in error notice, or message box for Ajax uploads).

The most important part of this is that it prevents the file from actually being uploaded to the server, as the only method I can use right now is in the validation process and return an error, but by that point the file has already been uploaded to the server.

@tripflex
Copy link
Collaborator Author

IMO this is a really important filter that is missing as I could use the submit_job_wp_handle_upload_overrides which I actually added in a PR a long time ago, but that was before the upload handling was changed, and we need to be able to return our own WP_Error to specify to the user on frontend what the problem was

@tripflex
Copy link
Collaborator Author

Also updated PHPDoc as return value is going to be stdClass or WP_Error object

@@ -723,6 +723,10 @@ function job_manager_upload_file( $file, $args = array() ) {
$allowed_mime_types = $args['allowed_mime_types'];
}

if( is_wp_error( $file = apply_filters( 'job_manager_upload_file_pre_upload', $file, $args, $allowed_mime_types ) ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incredibly picky, but can you add the space after the if?

I'm not crazy about the readability of the line, but not a blocker for me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say pull out the whole $file var.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spencerfinnell nah this way by adding one filter we add two use case scenarios ... one for modifying the $file data before being uploaded, and the other for returning a WP_Error to prevent upload and return that error

@kraftbj
Copy link
Contributor

kraftbj commented Aug 24, 2016

Noted very nit-picky code standard fix, but otherwise, 👍 from me.

@tripflex
Copy link
Collaborator Author

tripflex commented Aug 24, 2016

@kraftbj Would you prefer something like this instead?

/**
 * Filter file configuration before upload
 *
 * This filter can be used to modify the file arguments before being uploaded, or return a WP_Error
 * object to prevent the file from being uploaded, and return the error.
 *
 * @since 1.25.2
 *
 * @param array $file Array of $_FILE data to upload.
 * @param array $args Optional file arguments
 * @param array $allowed_mime_types Array of allowed mime types from field config or defaults
 */
$file = apply_filters( 'job_manager_upload_file_pre_upload', $file, $args, $allowed_mime_types );

if ( is_wp_error( $file ) ) {
    return $file;
}

@tripflex
Copy link
Collaborator Author

I used the method in the PR to kind of mimic this:

// Validate required
if ( is_wp_error( ( $return = $this->validate_fields( $values ) ) ) ) {
    throw new Exception( $return->get_error_message() );
}

https://github.com/Automattic/WP-Job-Manager/blob/master/includes/forms/class-wp-job-manager-form-submit-job.php#L426-L429

@kraftbj
Copy link
Contributor

kraftbj commented Aug 24, 2016

Yeah, that docblock is good. Commit it and I'll merge this in.

@tripflex
Copy link
Collaborator Author

@kraftbj okay updated

@kraftbj kraftbj merged commit 81a0bd4 into Automattic:master Aug 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants