Project

General

Profile

Bug(バグ) #2003

アルバム編集画面で、表紙が設定されていない場合に壊れた画像が表示されている

Added by Minoru Takai over 8 years ago. Updated over 8 years ago.

Status:
Fixed(完了)
Priority:
High(高め)
Target version:
Start date:
2011-04-12
Due date:
% Done:

100%

3.6 で発生するか:
Yes
[QA]バグ通知済:
No
3.8 で発生するか:
Unknown (未調査)

Description

概要

アルバムを作成して、アルバム(表紙)の編集画面( album/edit/:id )にアクセスする。

  • アルバム作成時に表紙画像を設定しない
  • アルバム作成時に設定した表紙画像を削除する

上記のいずれかの場合(アルバム表紙が "NO IMAGE" のとき)に、編集画面の「表紙画像」という項目欄に、 src 属性値のない img 要素がHTML上で出力されており、ブラウザによっては壊れた画像が表示されている(*1)。

(*1) 無論、問題なのは、壊れた画像が表示されていること自体ではなく、不適切な img 要素がHTML上に出力されていることである。

  • OpenPNE-3.4.10 で設置したSNSのアルバム編集ページのソースコード
    <tr>
      <th><label for="album_file_id">表紙画像</label></th>
      <td><img size="40" class="input_file" /><br /><input size="40" type="file" name="album[file_id]" class="input_file" id="album_file_id" /><br /> </td>
    </tr>
    

この問題が生じるバージョン

OpenPNE-3.4.10 および OpenPNE-3.6beta9 にバンドルされる、 opAlbumPlugin 0.9.3beta および 0.9.4beta では少なくとも生じている。恐らく全てのバージョンで生じる問題である。

この問題への対応方針

次の2つの方法が少なくとも考えられる。

  • (1) "NO IMAGE" のときは、画像を表示しない( img 要素が出力されないようにする)
  • (2) "NO IMAGE" のときは、NO IMAGE画像を表示する( img 要素の src 属性値に NO IMAGE画像のパスを与えるようにする)

このフォームの利便性と、"NO IMAGE"が使われる他の箇所(My Photo など)との統一性を考えると、(2) の対応が好ましいと考えられるが、異なる見解があればこの限りではない。

具体的な修正内容

このチケットを作成している時点では調査中です。

album_edit.png View - Mac, Firefox4 で -moz-force-broken-image-icon プロパティを指定して表示した際のキャプチャ (35.5 KB) Minoru Takai, 2011-04-12 16:16


Related issues

Related to opAlbumPlugin - Bug(バグ) #2466: mobile_frontendのアルバム編集画面で、表紙が設定されていない場合にno_image画像が表示されない New(新規) 2011-10-04

Associated revisions

Revision 92018af6 (diff)
Added by Maki Takahashi over 8 years ago

"No Image" is displayed instead of the cover image in the album edit when the cover image of the album is not set (refs #2003)

Revision d83416d2 (diff)
Added by Maki Takahashi over 8 years ago

Fit the coding style to other (refs #2003)

Revision 5e06c452
Added by Shogo Kawahara over 8 years ago

Merge pull request #7 from martini2002jp/ticket-2003

(fixes #2003)

History

#1 Updated by Shingo Yamada over 8 years ago

  • Priority changed from Normal(通常) to High(高め)

#2 Updated by Shingo Yamada over 8 years ago

本チケットは、高橋さんに依頼しました。(高橋さんのassign権限を要求中)

#3 Updated by Maki Takahashi over 8 years ago

  • Status changed from New(新規) to Accepted(着手)
  • Assignee set to Maki Takahashi

#4 Updated by Maki Takahashi over 8 years ago

  • Status changed from Accepted(着手) to Pending Review(レビュー待ち)
  • % Done changed from 0 to 50

https://github.com/martini2002jp/opAlbumPlugin/commit/92018af63ec80b93e7eb70e603a0580394752481
にて修正いたしました。

アルバム表紙画像が設定されていない場合、編集画面において「NO IMAGE」画像が表示され、*「このファイルを削除する」チェックボックスが表示されないこと*
アルバム表紙画像が設定されている場合は、編集画面において設定された画像が表示され、「このファイルを削除する」チェックボックスが表示されること
を確認しました。
(新規作成の場合は表紙画像はもともと表示されない)

モバイルについては表紙画像の設定変更ができないため、この問題は起きておりません。

#5 Updated by Naoya Tozuka over 8 years ago

変更点確認しました。動作上とくに問題ありませんが、テンプレート (_formEditImage.php) 内でのPHPコード部分につきまして、

<?php
$imageTag = image_tag_sf_image($image->getCoverImage(), array('size' => '120x120'));
if ($image->getFileId()): ?>
...
else:
...
endif;
これはOpenPNEコーディング規約では規定されていませんが、
<?php $imageTag = image_tag_sf_image($image->getCoverImage(), array('size' => '120x120')); ?>
<?php if ($image->getFileId()): ?>
...
<?php else: ?>
...
<?php endif;
のように1行1行を <?php ... ?> で囲むスタイルに合わせられることを推奨します。

#6 Updated by Minoru Takai over 8 years ago

序にですが、テンプレートファイルであれば、ファイル全体がPHP処理命令であることは想定していないため、最終行がPHP処理記述であってもPHP処理命令終了区切り子(つまり ?> )を記述した方が見栄えがよいかもしれません。参考までに関連するコーディング規約を示しておきます。 http://www.openpne.jp/coding-standards-ja/#id3

<?php $imageTag = image_tag_sf_image($image->getCoverImage(), array('size' => '120x120')); ?>
<?php if ($image->getFileId()): ?>
...
<?php else: ?>
...
<?php endif; ?>

なお、以下のような記述よりも note-5 で指摘されている記述の方が好ましい理由を個人的な観点から示します。

1: <?php
2: else:
3:   echo $imageTag; ?><br />
4: %input%<br />

ここで示した 3 行目について、例えば $imageTag の直前にHTML記述を追加する修正を行いたい場合、 else 文全体に差分が生じるような修正となってしまいます。また $imageTag を出力しない修正を行いたい場合、 1 行目で改行してしまっているのが不自然になる可能性があります。

PHP処理記述の目的が異なる場合は、処理が連続していたとしても処理命令区間を独立させたほうがよいと考えています。以下の例は極端かもしれませんが、個人的にはテンプレートに於いては以下のような書き方をしています(もちろん、場所によってはPHP処理の中で sprintf() などを用いて出力した方が見栄えがよい場面もあると思います)。

<?php
// テンプレート内で用いる一時的な変数は冒頭に書く
$var1 = ...;
$var2 = ...;
?>
<?php foreach ($vals as $val): ?>
<?php
// foreach 文ブロック内で一時的に用いる変数を書く
$val_x = ...;
$val_y = ...;
?>
<li><?php echo $val; ?>(<?php echo $val_x; ?>, <?php echo $val_y; ?>)</li>
<?php endforeach; ?>

上記はコーディング規約で定められているわけではないためあくまで参考程度にしていただければと思います。

#7 Updated by Maki Takahashi over 8 years ago

お二人のご指摘を元に
https://github.com/martini2002jp/opAlbumPlugin/commit/d83416d24cdc09606c23cc9c0528be2ac6f6a8ed
にてテンプレートを再度修正いたしました。

note-4 で行った動作確認を再度おこない、正しく動作していることを確認いたしました。

#8 Updated by Naoya Tozuka over 8 years ago

  • Status changed from Pending Review(レビュー待ち) to Pending Testing(テスト待ち)
  • % Done changed from 50 to 70

修正どうもありがとうございます。OKです。

// ところで、OpenPNEのコードの中では <?php echo $hogehoge ?> ではセミコロンが用いられず、<?php endif; ?> ではセミコロンが用いられる例が多いですが、完全に統一が取れているわけではなさそうです。

#9 Updated by Shogo Kawahara over 8 years ago

  • Target version set to 0.9.5

#10 Updated by Yuma Sakata over 8 years ago

  • Status changed from Pending Testing(テスト待ち) to Fixed(完了)
  • % Done changed from 70 to 100

テストOKです。

Also available in: Atom PDF