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

Use original filename for cache key #53

Merged
merged 1 commit into from
Aug 4, 2020

Conversation

lthurston
Copy link
Contributor

See issue #52

@lthurston
Copy link
Contributor Author

This is a pretty straight forward change. Let me know if you want to see any modifications.

@lthurston
Copy link
Contributor Author

@toughdeveloper Is this project being managed anymore?

@chocolata
Copy link
Collaborator

I haven't seen a response from @toughdeveloper in months. I'm not feeling very confident about this. Maybe someone should fork the project, so we can get on with it?

@lthurston
Copy link
Contributor Author

Yeah, it would be pretty ridiculous for us all to maintain our own forks. I can start a new issue and mention everyone who has an open PR to see who would be willing to take this on. I would be willing to manage the fork, but can't promise I'll always have enough time to deal with it.

@matt-pawley
Copy link
Owner

@lthurston @maartenmachiels - Hi, Sorry for not responding sooner. I don't have time to maintain this anymore.

Feel free to fork, I will update the readme to point to a maintained version.

@lthurston
Copy link
Contributor Author

Thanks, @toughdeveloper, for letting us know and also for this useful plugin.

@lthurston
Copy link
Contributor Author

See #55 for further discussion of project maintenance.

@matteotrubini
Copy link
Collaborator

I think that this enhancement will have high priority given the SEO best practices.

@lthurston this PR seems to be incomplete, maybe some commit are missing?

@maartenmachiels did you have check this PR before?

@chocolata
Copy link
Collaborator

Hi matteo, no I didn't check this before...

@lthurston
Copy link
Contributor Author

Sorry @matteotrubini , little behind on email. This PR is complete and running in production environments (saving GB of disk space). It's a simple change that just uses the relative path to an original images rather than the complete path in generating the cache key.

Copy link
Collaborator

@matteotrubini matteotrubini left a comment

Choose a reason for hiding this comment

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

Can you explain how changes let's us to use original's relative path please?

@matteotrubini
Copy link
Collaborator

@lthurston please also rebase your commit on top of current develop, thank you!

@lthurston
Copy link
Contributor Author

Actually, yeah, you're right. Something seems to be missing. I'll figure out what it is.

@lthurston
Copy link
Contributor Author

@matteotrubini I've finally pushed the right changes. Sorry about the delay.

@lthurston
Copy link
Contributor Author

Hi @matteotrubini, I'm just checking in and hoping this simple change can get merged in.

@lthurston
Copy link
Contributor Author

Does anyone on this thread have a moment to +1 this? I'm anxious to getting it merged in because as mentioned in #52, all images get regenerated using our deploy setup (Capistrano deploys new releases to a uniquely named folder each time, then creates a symlink). We've got Composer configured to use our fork of this, but it'd be great to wrap this up.

@chocolata
Copy link
Collaborator

I just reviewed it - looks good to me. Do we have everything to proceed now? I'm also a big fan of keeping the original filenames. SEO-wise it's a great idea to keep those names descriptive.

@lthurston
Copy link
Contributor Author

Thanks @maartenmachiels. @matteotrubini Any chance you can merge this bad boy in?

@chocolata
Copy link
Collaborator

@matteotrubini could you kindly do this?

@matteotrubini matteotrubini merged commit 3fe36e0 into matt-pawley:develop Aug 4, 2020
@chocolata
Copy link
Collaborator

Thank you Matteo.

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.

4 participants