gpml-acf-user-image-field.php: Fixed an issue with User Image getting removed on form resubmission.
#1091
Conversation
…ng removed on form resubmission.
WalkthroughThe update_user_image_field method in the GPML_ACF_User_Image_Field class was modified to handle cases where the ACF image field value is empty but uploaded files exist either in the $_POST data under 'gform_uploaded_files' or in the $_FILES superglobal. It extracts the uploaded filename, queries the WordPress database for the corresponding attachment ID, and sets the field value before checking if the value is empty and whether to remove it. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form (POST)
participant GPML_ACF_User_Image_Field
participant WordPress DB
User->>Form (POST): Submit form with/without image
Form (POST)->>GPML_ACF_User_Image_Field: Call update_user_image_field($value, $post_id, $field, $input_name)
alt $value is empty and 'gform_uploaded_files' exists or $_FILES contains file
GPML_ACF_User_Image_Field->>Form (POST): Extract uploaded filename from $_POST['gform_uploaded_files'] or $_FILES
GPML_ACF_User_Image_Field->>WordPress DB: Query for attachment post by filename
WordPress DB-->>GPML_ACF_User_Image_Field: Return attachment ID (if found)
GPML_ACF_User_Image_Field->>GPML_ACF_User_Image_Field: Set $value = attachment ID
end
GPML_ACF_User_Image_Field-->>Form (POST): Continue with updated $value
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
gp-media-library/gpml-acf-user-image-field.php (2)
47-70: Good fix for the image retention issue, with some security considerations.The added code effectively solves the issue by checking for uploaded files in the POST data when the ACF field value appears empty. This prevents the user's image from being lost during form resubmission.
However, there are some security considerations:
- The database query uses
LIKEwith wildcards, which could potentially match unintended files if filenames are similar.- Consider adding additional validation to ensure the attachment actually belongs to the user being updated.
- $attachment_id = $wpdb->get_var( $wpdb->prepare( - "SELECT ID FROM $wpdb->posts WHERE post_type = 'attachment' AND guid LIKE %s LIMIT 1", - '%' . $wpdb->esc_like( $filename ) . '%' - )); + // More precise query that also checks post_author matches the user being updated + $attachment_id = $wpdb->get_var( $wpdb->prepare( + "SELECT ID FROM $wpdb->posts WHERE post_type = 'attachment' AND guid LIKE %s AND (post_author = %d OR post_author = 0) LIMIT 1", + '%' . $wpdb->esc_like( $filename ) . '%', + $user_id + ));
48-53: Consider adding error handling for JSON parsing.The code assumes that
$clean_jsonwill always be valid JSON. If the JSON is malformed,json_decode()will returnnulland no error handling is in place.- $clean_json = stripslashes( $raw_json ); - - $uploaded_files_array = json_decode( $clean_json, true ); + $clean_json = stripslashes( $raw_json ); + $uploaded_files_array = json_decode( $clean_json, true ); + + // Add error handling for JSON parsing + if ( json_last_error() !== JSON_ERROR_NONE ) { + error_log( 'GPML_ACF_User_Image_Field: Error decoding uploaded files JSON: ' . json_last_error_msg() ); + $uploaded_files_array = array(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gp-media-library/gpml-acf-user-image-field.php(1 hunks)
🔇 Additional comments (1)
gp-media-library/gpml-acf-user-image-field.php (1)
72-74: The fix correctly preserves the existing logic.The original conditional that checks if the value is empty is maintained, with the new code improving detection of uploaded files before this check. This ensures backward compatibility with the existing behavior.
| $field_id = $this->_args['field_id']; | ||
| $key = 'input_' . $field_id; | ||
|
|
||
| if ( isset( $uploaded_files_array[ $key ][0]['uploaded_filename'] ) ) { |
There was a problem hiding this comment.
@saifsultanc does this need to work with single file upload fields as well?
If so, you'll likely need to do a check on $_FILES if it's a single file upload field. You can check the field type like so: $field->multipleFiles
Here's an example of $_FILES with a file for a single file upload field.
…ng removed on form resubmission.
…ng removed on form resubmission.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
gp-media-library/gpml-acf-user-image-field.php (1)
61-83: Address code duplication as mentioned in past review.The database query and value assignment logic is duplicated between the two conditional blocks. This violates the DRY principle and makes maintenance harder.
+private function try_set_value_from_filename( $filename ) { + if ( empty( $filename ) ) { + return null; + } + $attachment_id = $this->get_attachment_id_by_filename( $filename ); + return $attachment_id ? $attachment_id : null; +} if ( empty( $value ) && $raw_json ) { $uploaded_files_array = json_decode( stripslashes( $raw_json ), true ); if ( json_last_error() === JSON_ERROR_NONE && isset( $uploaded_files_array[ $key ][0]['uploaded_filename'] ) ) { - $filename = $uploaded_files_array[ $key ][0]['uploaded_filename']; - $attachment_id = $this->get_attachment_id_by_filename( $filename ); - if ( $attachment_id ) { - $value = $attachment_id; - } + $value = $this->try_set_value_from_filename( $uploaded_files_array[ $key ][0]['uploaded_filename'] ); } } $field = GFFormsModel::get_field( $form, $field_id ); if ( empty( $value ) && ! $field->multipleFiles && isset( $_FILES[ $key ] ) && ! empty( $_FILES[ $key ]['name'] ) ) { - $filename = $_FILES[ $key ]['name']; - $attachment_id = $this->get_attachment_id_by_filename( $filename ); - if ( $attachment_id ) { - $value = $attachment_id; - } + $value = $this->try_set_value_from_filename( $_FILES[ $key ]['name'] ); }
🧹 Nitpick comments (1)
gp-media-library/gpml-acf-user-image-field.php (1)
45-84: Consider edge cases and add documentation.The method handles multiple scenarios but lacks documentation explaining the fallback logic. Consider adding comments and handling edge cases like:
- What happens if multiple attachments have the same filename?
- Should there be logging when fallback methods are used?
- What about file extension mismatches?
Add method documentation:
+/** + * Updates the ACF user image field with uploaded file data. + * + * Tries multiple approaches to find the uploaded file: + * 1. Standard ACF field value retrieval + * 2. Fallback to gform_uploaded_files POST data + * 3. Fallback to $_FILES superglobal for single file uploads + * + * @param int $user_id User ID + * @param array $feed Gravity Forms feed data + * @param array $entry Form entry data + */ function update_user_image_field( $user_id, $feed, $entry ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gp-media-library/gpml-acf-user-image-field.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Danger JS
- GitHub Check: Danger JS
| $raw_json = $_POST['gform_uploaded_files'] ?? ''; | ||
| $field_id = $this->_args['field_id']; | ||
| $key = 'input_' . $field_id; | ||
|
|
||
| if ( empty( $value ) && $raw_json ) { | ||
| $uploaded_files_array = json_decode( stripslashes( $raw_json ), true ); | ||
| if ( isset( $uploaded_files_array[ $key ][0]['uploaded_filename'] ) ) { | ||
| $filename = $uploaded_files_array[ $key ][0]['uploaded_filename']; | ||
| $attachment_id = $this->get_attachment_id_by_filename( $filename ); | ||
| if ( $attachment_id ) { | ||
| $value = $attachment_id; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON parsing and array access.
The current code assumes the JSON structure and doesn't handle parsing errors or missing array keys, which could cause PHP warnings or unexpected behavior.
if ( empty( $value ) && $raw_json ) {
$uploaded_files_array = json_decode( stripslashes( $raw_json ), true );
- if ( isset( $uploaded_files_array[ $key ][0]['uploaded_filename'] ) ) {
+ if ( json_last_error() === JSON_ERROR_NONE &&
+ isset( $uploaded_files_array[ $key ][0]['uploaded_filename'] ) ) {
$filename = $uploaded_files_array[ $key ][0]['uploaded_filename'];
$attachment_id = $this->get_attachment_id_by_filename( $filename );
if ( $attachment_id ) {
$value = $attachment_id;
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $raw_json = $_POST['gform_uploaded_files'] ?? ''; | |
| $field_id = $this->_args['field_id']; | |
| $key = 'input_' . $field_id; | |
| if ( empty( $value ) && $raw_json ) { | |
| $uploaded_files_array = json_decode( stripslashes( $raw_json ), true ); | |
| if ( isset( $uploaded_files_array[ $key ][0]['uploaded_filename'] ) ) { | |
| $filename = $uploaded_files_array[ $key ][0]['uploaded_filename']; | |
| $attachment_id = $this->get_attachment_id_by_filename( $filename ); | |
| if ( $attachment_id ) { | |
| $value = $attachment_id; | |
| } | |
| } | |
| } | |
| $raw_json = $_POST['gform_uploaded_files'] ?? ''; | |
| $field_id = $this->_args['field_id']; | |
| $key = 'input_' . $field_id; | |
| if ( empty( $value ) && $raw_json ) { | |
| $uploaded_files_array = json_decode( stripslashes( $raw_json ), true ); | |
| if ( json_last_error() === JSON_ERROR_NONE && | |
| isset( $uploaded_files_array[ $key ][0]['uploaded_filename'] ) ) { | |
| $filename = $uploaded_files_array[ $key ][0]['uploaded_filename']; | |
| $attachment_id = $this->get_attachment_id_by_filename( $filename ); | |
| if ( $attachment_id ) { | |
| $value = $attachment_id; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In gp-media-library/gpml-acf-user-image-field.php around lines 61 to 74, the
code decodes JSON and accesses nested array keys without checking for JSON
parsing errors or verifying the existence of keys, which can cause warnings. Add
error handling by verifying json_decode did not return null and check that the
expected keys exist before accessing them to prevent warnings and ensure robust
behavior.
| if ( empty( $value ) && ! $field->multipleFiles && isset( $_FILES[ $key ] ) && ! empty( $_FILES[ $key ]['name'] ) ) { | ||
| $filename = $_FILES[ $key ]['name']; | ||
| $attachment_id = $this->get_attachment_id_by_filename( $filename ); | ||
| if ( $attachment_id ) { | ||
| $value = $attachment_id; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add filename validation for security.
The filename from $_FILES should be validated to prevent potential security issues or unexpected behavior.
if ( empty( $value ) && ! $field->multipleFiles && isset( $_FILES[ $key ] ) && ! empty( $_FILES[ $key ]['name'] ) ) {
$filename = $_FILES[ $key ]['name'];
+ // Validate filename
+ if ( ! preg_match( '/^[a-zA-Z0-9._-]+\.(jpg|jpeg|png|gif|webp)$/i', $filename ) ) {
+ return;
+ }
$attachment_id = $this->get_attachment_id_by_filename( $filename );
if ( $attachment_id ) {
$value = $attachment_id;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( empty( $value ) && ! $field->multipleFiles && isset( $_FILES[ $key ] ) && ! empty( $_FILES[ $key ]['name'] ) ) { | |
| $filename = $_FILES[ $key ]['name']; | |
| $attachment_id = $this->get_attachment_id_by_filename( $filename ); | |
| if ( $attachment_id ) { | |
| $value = $attachment_id; | |
| } | |
| } | |
| if ( empty( $value ) && ! $field->multipleFiles && isset( $_FILES[ $key ] ) && ! empty( $_FILES[ $key ]['name'] ) ) { | |
| $filename = $_FILES[ $key ]['name']; | |
| // Validate filename | |
| if ( ! preg_match( '/^[a-zA-Z0-9._-]+\.(jpg|jpeg|png|gif|webp)$/i', $filename ) ) { | |
| return; | |
| } | |
| $attachment_id = $this->get_attachment_id_by_filename( $filename ); | |
| if ( $attachment_id ) { | |
| $value = $attachment_id; | |
| } | |
| } |
🤖 Prompt for AI Agents
In gp-media-library/gpml-acf-user-image-field.php around lines 77 to 83, the
filename obtained from $_FILES is used directly without validation, which can
pose security risks. Add validation to the $filename variable to ensure it
contains only safe characters and does not include any path traversal sequences
or other malicious input before using it to get the attachment ID. This can be
done by sanitizing the filename or using a whitelist pattern to allow only
expected characters.
| protected function get_attachment_id_by_filename( $filename ) { | ||
| global $wpdb; | ||
|
|
||
| return $wpdb->get_var( $wpdb->prepare( | ||
| "SELECT ID FROM $wpdb->posts WHERE post_type = 'attachment' AND guid LIKE %s LIMIT 1", | ||
| '%' . $wpdb->esc_like( $filename ) . '%' | ||
| )); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the filename matching logic for better reliability.
The current approach of using LIKE '%filename%' on the GUID field may produce unreliable results since:
- WordPress attachment GUIDs typically contain full URLs, not just filenames
- The wildcard matching could match unintended attachments
- There's no validation of the input filename
Consider using WordPress's built-in functions or querying by post_title or _wp_attached_file meta instead.
protected function get_attachment_id_by_filename( $filename ) {
- global $wpdb;
-
- return $wpdb->get_var( $wpdb->prepare(
- "SELECT ID FROM $wpdb->posts WHERE post_type = 'attachment' AND guid LIKE %s LIMIT 1",
- '%' . $wpdb->esc_like( $filename ) . '%'
- ));
+ global $wpdb;
+
+ // First try exact match on _wp_attached_file meta
+ $attachment_id = $wpdb->get_var( $wpdb->prepare(
+ "SELECT post_id FROM $wpdb->postmeta WHERE meta_key = '_wp_attached_file' AND meta_value LIKE %s LIMIT 1",
+ '%' . $wpdb->esc_like( basename( $filename ) )
+ ));
+
+ if ( $attachment_id ) {
+ return $attachment_id;
+ }
+
+ // Fallback to post_title match
+ return $wpdb->get_var( $wpdb->prepare(
+ "SELECT ID FROM $wpdb->posts WHERE post_type = 'attachment' AND post_title = %s LIMIT 1",
+ pathinfo( $filename, PATHINFO_FILENAME )
+ ));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function get_attachment_id_by_filename( $filename ) { | |
| global $wpdb; | |
| return $wpdb->get_var( $wpdb->prepare( | |
| "SELECT ID FROM $wpdb->posts WHERE post_type = 'attachment' AND guid LIKE %s LIMIT 1", | |
| '%' . $wpdb->esc_like( $filename ) . '%' | |
| )); | |
| } | |
| protected function get_attachment_id_by_filename( $filename ) { | |
| global $wpdb; | |
| // First try exact match on the _wp_attached_file meta (stores the file path) | |
| $attachment_id = $wpdb->get_var( $wpdb->prepare( | |
| "SELECT post_id FROM $wpdb->postmeta WHERE meta_key = '_wp_attached_file' AND meta_value LIKE %s LIMIT 1", | |
| '%' . $wpdb->esc_like( basename( $filename ) ) | |
| )); | |
| if ( $attachment_id ) { | |
| return $attachment_id; | |
| } | |
| // Fallback to an exact post_title match (filename without extension) | |
| return $wpdb->get_var( $wpdb->prepare( | |
| "SELECT ID FROM $wpdb->posts WHERE post_type = 'attachment' AND post_title = %s LIMIT 1", | |
| pathinfo( $filename, PATHINFO_FILENAME ) | |
| )); | |
| } |
🤖 Prompt for AI Agents
In gp-media-library/gpml-acf-user-image-field.php around lines 36 to 43, the
current method uses a SQL LIKE query on the GUID field to find an attachment by
filename, which is unreliable because GUIDs contain full URLs and the wildcard
can match unintended results. To fix this, replace the query to search by the
'_wp_attached_file' meta key using a meta query or join with the postmeta table,
ensuring exact matching of the filename. Also, validate the input filename
before querying to avoid invalid or empty values.
Context
⛑️ Ticket(s): https://secure.helpscout.net/conversation/2921885802/82733
Summary
File uploaded to an ACF Image field on the user profile, when user profile is updated with GFUR is removed.
https://www.loom.com/share/029fcc517e50428d81800577d1106661?sid=ac7c7d82-17ae-4161-bda6-f4c3478d5e50