-
Notifications
You must be signed in to change notification settings - Fork 31
Feature/public id sync #105
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good refactor overall, with solid changes, but there's some unclarity that should be addressed before this PR moves forward.
| if ( false !== strpos( $public_id, '/' ) ) { | ||
| // Split the public_id into path and filename to allow filtering just the ID and not giving access to the path. | ||
| $public_id_info = pathinfo( $public_id ); | ||
| $public_id_folder = trailingslashit( $public_id_info['dirname'] ); | ||
| $public_id_file = $public_id_info['filename']; | ||
| } else { | ||
| // File is in the root of cloudinary. | ||
| $public_id_folder = ''; | ||
| $public_id_file = $public_id; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can switch to this in order to satisfy linters:
| if ( false !== strpos( $public_id, '/' ) ) { | |
| // Split the public_id into path and filename to allow filtering just the ID and not giving access to the path. | |
| $public_id_info = pathinfo( $public_id ); | |
| $public_id_folder = trailingslashit( $public_id_info['dirname'] ); | |
| $public_id_file = $public_id_info['filename']; | |
| } else { | |
| // File is in the root of cloudinary. | |
| $public_id_folder = ''; | |
| $public_id_file = $public_id; | |
| } | |
| // File is in the root of cloudinary. | |
| $public_id_folder = ''; | |
| $public_id_file = $public_id; | |
| if ( false !== strpos( $public_id, '/' ) ) { | |
| // Split the public_id into path and filename to allow filtering just the ID and not giving access to the path. | |
| $public_id_info = pathinfo( $public_id ); | |
| $public_id_folder = trailingslashit( $public_id_info['dirname'] ); | |
| $public_id_file = $public_id_info['filename']; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dugajean Nice idea. I usually do prefer to have a default, and if it's not what's expected then change.
|
LGTM |
pereirinha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @DavidCramer for the hard work on this one.
Allows for being able to alter the public_id for uploading as well as correcting the folder changing sync.
Testing notes.
This allows for changing the public ID of assets.
Below is an example of using the filter
cloudinary_upload_optionsto use the assets slug as it'spublic_idAdd the snippet in the
functions.phpin your theme.go to the media library page. If all assets slug is the same as the ID (it should be), then the cloud should be green.
Edit an asset, and change the slug (you may need to select Screen Options, and enable it).
Update and go back to the media library. The image should now be in the syncing state (orange cloud).
After a moment, it should be green and the public ID should be the same as the slug.