Skip to content

Comments

Ticket 62119: Fixes invalid url output#131

Merged
heyitsbryanm merged 4 commits intoimgix:mainfrom
anthony0030:ticket-62119
Apr 7, 2025
Merged

Ticket 62119: Fixes invalid url output#131
heyitsbryanm merged 4 commits intoimgix:mainfrom
anthony0030:ticket-62119

Conversation

@anthony0030
Copy link
Contributor

@anthony0030 anthony0030 commented Feb 4, 2025

Description

When making a URL with a blank parameter, it would outpoint an unneeded =

Before this PR...
https://demo.imgix.net/images/demo.png?mark=&h=200&w=200&s=XXXXXXXXXXXXXX
https://demo.imgix.net/images/demo.png?mark=

After this PR...
https://demo.imgix.net/images/demo.png?mark&h=200&w=200&s=XXXXXXXXXXXXXX
https://demo.imgix.net/images/demo.png?mark

Checklist

We have a default mark parameter defined within our Imgix source settings, and we need to generate an image for use with an OG tag WITHOUT the mark parameter. We deliberately pass in a blank "mark" parameter to override the default mark parameter defined in Imgix. Sending in a blank mark parameter removes the default watermark.

The SDK generates a URL with "mark=" with the correct signature, and this image is accessible; however, when Facebook tries to download this image, it goes to the image URL with "mark" (notice without the = sign). This makes the image URL invalid because the signature is for "mark=" and not "mark," and no image loads.

When somebody shares a link to their website, the card Facebook produces has no image.

This PR solves this problem as the generated URL is missing the "=" and Facebook visits the url correctly.

PS: This commit also fixes the tests not running due to the change in Minitest, MiniTest -> Minitest

@anthony0030 anthony0030 requested a review from a team as a code owner February 4, 2025 10:16
@anthony0030 anthony0030 changed the title Ticket 62119: Fixes ivalid url output Ticket 62119: Fixes invalid url output Feb 4, 2025
Copy link
Collaborator

@heyitsbryanm heyitsbryanm left a comment

Choose a reason for hiding this comment

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

Looks good - thanks!

Copy link

@AbuBoo AbuBoo left a comment

Choose a reason for hiding this comment

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

From testing the files I still get mark= with the equation set.

I tested it for both the signed URLs and not signed and for mark64 and regular mark.

On that, nothing is erroring, so if this patch works for the client's specific domain then its okay.

Outputs from my commands on branch Ticket 62119
Ahmed@imgixs-MBP imgix-rb % ruby generate-imgix-url-signed.rb Generated Imgix signed URL: https://s3testingdemosource.imgix.net/img/test/FELV-cat.jpg?ixlib=rb-4.1.0&w=500&h=700&mark64=&s=d9b732839820f0fba4d3071618aa532a

Ahmed@imgixs-MBP imgix-rb % ruby generate_imgix_url.rb Generated Imgix URL: https://assetmanager.imgix.net/test/FELV-cat.jpg?ixlib=rb-4.1.0&w=500&h=700&mark=

@anthony0030
Copy link
Contributor Author

Hi @AbuBoo, thank you for looking into this!

I would like to know where I can find the files you are referencing.

  • generate-imgix-url-signed.rb
  • generate_imgix_url.rb

I would like to have this PR perfect.

@AbuBoo
Copy link

AbuBoo commented Mar 26, 2025

Hi @anthony0030, I was actually able to update my Ruby and fix my testing scripts; I replicated your steps and got the same output.

Additionally, we tested with a test Block agaisnt our library to ensure no conflicting errors. From my end, this looks good to go.

Copy link

@AbuBoo AbuBoo left a comment

Choose a reason for hiding this comment

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

LGTM after testing it works well with no errors.

@anthony0030 anthony0030 requested a review from AbuBoo March 26, 2025 19:36
@anthony0030
Copy link
Contributor Author

Sorry pressed by mistake

@anthony0030
Copy link
Contributor Author

Good morning,

Is there any update on getting this merged @heyitsbryanm, @AbuBoo ?

I hope you have a great week.

@heyitsbryanm heyitsbryanm merged commit 7fafbc1 into imgix:main Apr 7, 2025
@heyitsbryanm
Copy link
Collaborator

Sorry, took me a moment to get to this.

I merged it and released a new version. Thanks again for writing the fix!

Let me know if you need us to do anything else to make sure the fix is released.

@anthony0030
Copy link
Contributor Author

Good morning @heyitsbryanm,

I appreciate your help so far!

Could you also release the gem so it makes the installation easy?
I don't see the new version here https://rubygems.org/gems/imgix

Have a great day!

@anthony0030 anthony0030 deleted the ticket-62119 branch April 14, 2025 13:29
@anthony0030
Copy link
Contributor Author

Hi!
Is there any update on getting version 4.1.1 published @heyitsbryanm, @AbuBoo?

When I install the gem according to the instructions, I get version 4.1.0, not 4.1.1.
It seems the update has not been pushed to https://rubygems.org/gems/imgix

This guide can be helpful: https://guides.rubygems.org/publishing

Have a great week!

@AbuBoo
Copy link

AbuBoo commented Apr 17, 2025

Hi! Is there any update on getting version 4.1.1 published @heyitsbryanm, @AbuBoo?

When I install the gem according to the instructions, I get version 4.1.0, not 4.1.1. It seems the update has not been pushed to https://rubygems.org/gems/imgix

This guide can be helpful: https://guides.rubygems.org/publishing

Have a great week!

@anthony0030 I'm looking into this this week and will follow up with you soon. Apologies for the delay.

@AbuBoo
Copy link

AbuBoo commented May 2, 2025

@anthony0030 This has been completed, and the version is now available for use. Sorry for the delay had to get the correct permissions to update this SDK.

Let me know if you need anything else!

@anthony0030
Copy link
Contributor Author

@AbuBoo, thanks very much! I understand the problem. I will check it when I am back at the office. Have a great weekend!

@anthony0030
Copy link
Contributor Author

I checked it and I was able to install it with no problem! Thanks again @AbuBoo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants