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

fix url & remove OriginPath #321

Merged

Conversation

ayuki-joto
Copy link
Collaborator

🎩 What? Why?

#317 で修正したcloud formationと、url関数の修正
Staging 環境で動かした場合に、Origin Pathが邪魔して読み込めないものがあった。
また、decidimのデフォルトアイコンが表示されないため修正

📌 Related Issues

📋 Subtasks

  • Add CHANGELOG upgrade notes, if required
  • If there's a new public field, add it to GraphQL API
  • Add documentation regarding the feature
  • Add/modify seeds
  • Add tests
  • Another subtask

@ayuki-joto
Copy link
Collaborator Author

defaultのiconなどを返す場合は、pathがないものなので、既存のdefault_urlで対応
また、asset_hostがない場合にlocalで画像がなくなってしまうので、そのための分岐も追加しました

Copy link
Contributor

@komtaki komtaki left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@takahashim
Copy link
Collaborator

確かにこの修正がないと問題がありそう(ちなみに開発環境でも画像が壊れることがあるっぽいです…)のですが、やっぱりよく理解できないところがあるのでいまさらですが質問させてください。
このurlの定義ってcarrierwaveのCarrierWave::Uploader::Url由来のものを修正したもの、ということで合ってるでしょうか。
元の定義は以下のようになっていて、細かいところが違ってそうです。

      ##
      # === Parameters
      #
      # [Hash] optional, the query params (only AWS)
      #
      # === Returns
      #
      # [String] the location where this file is accessible via a url
      #
      def url(options = {})
        if file.respond_to?(:url) and not (tmp_url = file.url).blank?
          file.method(:url).arity == 0 ? tmp_url : file.url(options)
        elsif file.respond_to?(:path)
          path = encode_path(file.path.sub(File.expand_path(root), ''))

          if host = asset_host
            if host.respond_to? :call
              "#{host.call(file)}#{path}"
            else
              "#{host}#{path}"
            end
          else
            (base_path || "") + path
          end
        end
      end

これだと file.respond_to?(:path) の条件も合致しない場合にはそのまま何も返さず(= nilを返して)、それによりdefault_url()にfallbackする、ということになっていたようなのですが、そのへんは path.nil?の条件で間に合うのでしょうか?
fileを一切見ないようになっているのも理由がよく分かってないのでした。

あと、CarrierWave::Uploader::Urlで定義されているurl()は、CarrierWave::Uploader::VersionsCarrierWave::Uploader::DefaultUrlで上書きされていて、そのうちCarrierWave::Uploader::Versionsで上書きしている処理がなくなってしまっているのでは…?という懸念もあります(その辺りも敢えて上書きしたのかもしれませんが…)。

@takahashim
Copy link
Collaborator

↑上記についてはFogの対応でCarrierWave側にさらに上書きされているらしいのと、現状の挙動を確認するとこれで問題なさそうということなので、私の方もちゃんと把握できているわけではないのでいったんこれで様子を見てる方針で良さそうです

@ayuki-joto ayuki-joto merged commit e5a46a8 into codeforjapan:develop Jan 17, 2022
@ayuki-joto ayuki-joto mentioned this pull request Jan 17, 2022
6 tasks
@ayuki-joto ayuki-joto deleted the fix/clouf-formation-cloud-front branch April 25, 2022 02:31
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