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

Add a new helper to return the contents of an asset file #4149

Merged
merged 1 commit into from
Aug 9, 2017

Conversation

lizconlan
Copy link

@lizconlan lizconlan commented Aug 8, 2017

Connects to #4148

Fix inspired by rails/sprockets-rails#237 (comment)
(and the immediate followups).

Placed in a helper wrapper to allow us to force UTF-8 and handle the minor faff of checking whether the Enumerator actually contains anything without a bunch of extra stuff to the template.

After deploying, we may need to clear zip file caches for attempted downloads as I suspect it's cached an HTML file containing the File Not Found message

@lizconlan lizconlan force-pushed the 4148-fix-sprockets-error branch 3 times, most recently from 116d1f9 to 6e1dc6b Compare August 8, 2017 15:21
@@ -78,6 +78,11 @@ def admin_date(date)
return "#{exact_date} (#{ago_text})"
end

def read_asset_file(asset_name)
assets = Rails.application.assets_manifest.find_sources(asset_name)
assets.first.force_encoding('utf-8') if assets.first
Copy link
Member

Choose a reason for hiding this comment

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

I think it should raise an exception if it can't be found, otherwise this would silently fail.

Copy link
Member

@garethrees garethrees 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 👍 Lets get this deployed as soon as possible.

You're right about needing to clear caches.

if assets.first
assets.first.force_encoding('utf-8')
else
raise(Sprockets::FileNotFound,
Copy link
Member

Choose a reason for hiding this comment

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

Always omit parentheses for Methods that have "keyword" status in Ruby:

https://github.com/bbatsov/ruby-style-guide#method-invocation-parens

With Sprockets 3.x and Rails 4.2 and up, the default setting in
production for `config.assets.compile` is false which means that
`application.assets` is not populated when we attempt to access it to
read the asset file contents.

* Adds a new helper method to:
  -  handle the call to the sprockets method
     `applications.assets_manifest.find_sources()` which returns an
     Enumerator of the contents of found files instead of the original
     method's String or Nil
  - forces the content to UTF-8 to avoid possible string concatenation
    errors
@lizconlan lizconlan merged commit 082ffdd into develop Aug 9, 2017
@lizconlan lizconlan deleted the 4148-fix-sprockets-error branch January 29, 2018 12:05
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.

None yet

2 participants