Skip to content

Attachment default_url should allow nil#55

Open
johnnyshields wants to merge 3 commits into
kreeti:masterfrom
johnnyshields:patch-2
Open

Attachment default_url should allow nil#55
johnnyshields wants to merge 3 commits into
kreeti:masterfrom
johnnyshields:patch-2

Conversation

@johnnyshields

Copy link
Copy Markdown

If I set Paperclip::Attachment.default_options[:default_url] = nil (instead of missing.png) Paperclip gives an error. This PR allows setting to nil, without changing any existing default behavior.

johnnyshields and others added 3 commits March 29, 2021 13:50
If I set `Paperclip::Attachment.default_options[:default_url] = nil` (instead of `missing.png`) Paperclip gives an error. This PR allows setting to nil, without changing any existing behavior.
@ssinghi
ssinghi requested a review from sbhawsingka May 27, 2021 17:21
@ssinghi ssinghi self-assigned this May 27, 2021
@sbhawsingka

Copy link
Copy Markdown

Hi @johnnyshields, thanks for the PR!

Your fix looks good, however, one small concern

image_tag user.avatar.url(:medium)

will lead to the following error - Nil location provided. Can't build URI.

So this change will hamper the ability to follow the common pattern of using the image_tag as shown in the above example.

If the purpose of this PR is only to render nothing in place of a missing image, maybe, we should avoid returning nil from the library so that the common code pattern shown above works safely.

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