-
Notifications
You must be signed in to change notification settings - Fork 3
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
adding custom WP CLI command and optional admin screen #3
base: master
Are you sure you want to change the base?
Conversation
@ryanshoover I know you are swamped, so there is zero rush to this, but I'd like your thoughts on this enhancement to the plugin. It's also adding an admin screen section. |
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.
Don't you need to instantiate these classes in the fastly-io.php file to get them to add their hooks?
|
||
--- | ||
|
||
## **Features** |
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.
Add a requirements section? You have to be routing your site through the Fastly CDN and you have to have Fastly's Image IO turned on. Anything else?
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.
Probably making sure any other image optimization plugins are turned off, like EWWW, Smush, and the like. Yeah I'll add that.
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.
Hunh. Do you have to turn off smush or ewww? What if you want to minimize the first image you upload? Worth testing?
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.
I mean you don't...but wouldn't that be redundant, if the minification and optimization takes place via Fastly? Yeah, it's worth testing. Do we have a Smush and/or EWWW license to test with?
} else { | ||
$command = 'wp fastlyio media regenerate'; | ||
$command .= ' 2>&1'; | ||
$output = shell_exec($command); |
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.
Could we paginate or stream the response? If memory serves, this can take a long time to complete, given all the DB queries. It can look like nothing happened after you clicked the button
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.
Do we need to run this through shell_exec? Is there a way to not have to rely on that? (I'm not sure it'll work reliably on app servers as the requests might time out or fail mysteriously)
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.
I was using shell_exec because it was conking out on me, but that could have just been me. I'll see what I can do to paginate/stream this, and not use shell_exec for sure.
@@ -0,0 +1,116 @@ | |||
<?php |
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.
Why do we need this rather than just wp media regenerate
or wp media regenerate --url=...
?
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.
Mainly because when I was using this on Boone, I often ran into issues where it was trying to regenerate a file that couldn't be regenerated, like a PDF. I'd need to do a deep dive into the CLI command itself, but this is an attempt to make sure only items with image meta gets handled.
Happy to switch this back to the regular command tho if you think that's a poop idea!
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.
Ah weird. media regenerate specifically supports PDFs because of their image thumbnails. I wonder if that breaks something we're doing.
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.
Huh. First, holy 7 year old find. And two, maybe Boone just had corrupt files.
This isn't a hill I'm going to die on. I'll remove this and adjust to go back to the standard. :)
Yes, yes you do. I didn't copy the right file over from my local testing env. 🤦🏻♀️ Should be there now (for the admin anyway) |
bc0c104
to
0511833
Compare
This PR adds a custom hook into the
wp media regenerate
command. This will only regenerate attachments that have image metadata, thus skipping PDFs.Still is a WIP, and am working on batching and multisite support. Also, deleting variants without resizing for non-image attachments.