Bug(バグ) #3294
完了選択肢を持つプロフィール項目を作成し、ユーザーが選択肢を入力後に選択肢IDを削除すると、ユーザーのプロフィールを更新することができなくなる。
0%
説明
現象¶
プロフィール項目登録で選択肢一覧を作成し、ユーザーが選択肢を入力したあと、選択肢のIDを削除した時、ユーザーのプロフィールを更新することができなくなる。
再現環境¶
OpenPNE 3.8.3
再現手順¶
1.管理画面からプロフィール項目登録を選択し、フォームタイプを「複数選択」に設定した、任意のプロフィール項目を登録する。
2.プロフィール選択肢一覧で、任意のIDを登録する。
3.ユーザー画面のプロフィール編集画面から、上記IDを選択し、送信ボタンを押す。
4.管理画面で、プロフィール選択肢一覧から、上記IDを削除する。(プロフィール項目のIDを削除するのではない)
5.ユーザー画面のプロフィール編集画面を表示し、送信ボタンを押す。
→プロフィールの編集が完了せず、白い画面が表示される。
またdevでは下記のメッセージが表示される。
Fatal error: Call to a member function delete() on a non-object in /home/sns/openPNE38.pne.jp/lib/model/doctrine/MemberProfile.class.php on line 207
Yuya Watanabe さんがほぼ12年前に更新
- 説明 を更新 (差分)
- 担当者 を削除 (
Yuya Watanabe) - 3.6 で発生するか を Unknown (未調査) から Yes (はい) に変更
Chiharu Nakajima さんがほぼ12年前に更新
- 担当者 を Yuya Watanabe にセット
- 優先度 を Normal(通常) から High(高め) に変更
- 親チケット を削除 (
#2684) - 3.6 で発生するか を Yes (はい) から Unknown (未調査) に変更
Yuya Watanabe さんがほぼ12年前に更新
下記チケットのコミットが邪魔なため revert する.
#2478 「プロフィール項目で「単一選択」の状態で保存したあとに「複数選択」の状態で保存を行おうとしても保存されていないように見える」
$ git revert f84facbf5ea0f7433881b5ccfd6735763ecf61bc
Yuya Watanabe さんがほぼ12年前に更新
原因調査¶
mysql> select * from member_profile where profile_id = 5; +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | 20 | 3 | 5 | NULL | | NULL | NULL | 20 | 1 | 4 | 0 | 2013-01-29 22:38:47 | 2013-01-29 22:38:47 | | 21 | 3 | 5 | 2 | | NULL | NULL | 20 | 2 | 3 | 1 | 2013-01-29 22:38:47 | 2013-01-29 22:38:47 | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ 2 rows in set (0.00 sec)
下記のような構造となっている.
1----4 2--3
ここで proifle_option_id = 2 となるようなプロフィール項目を削除すると下記のようなデータになる.
mysql> select * from member_profile where profile_id = 5; +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | 20 | 3 | 5 | NULL | | NULL | NULL | 20 | 1 | 4 | 0 | 2013-01-29 22:38:47 | 2013-01-29 22:38:47 | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ 1 row in set (0.01 sec)
このとき,手順 5 を実行すると下記のような部分を通る.
lib/form/doctrine/MemberProfileForm.class.php 57 if (is_null($value['value'])) 58 { 59 if ($memberProfile) 60 { 61 if ($profile->isMultipleSelect()) 62 { 63 $memberProfile->clearChildren(); 64 }
lib/model/doctrine/MemberProfile.class.php 202 public function clearChildren() 203 { 204 if ($this->getTreeKey() && $this->getNode()->hasChildren()) 205 { 206 $children = $this->getNode()->getChildren(); 207 $children->delete(); 208 } 209 }
lib/vendor/symfony/lib/plugins/sfDoctrinePlugin/lib/vendor/doctrine/Doctrine/Node/NestedSet.php 56 /** 57 * test if node has children 58 * 59 * @return bool 60 */ 61 public function hasChildren() 62 { 63 return (($this->getRightValue() - $this->getLeftValue()) > 1); 64 }
ここで $this->getRightValue() - $this->getLeftValue() の値は 4 - 1 = 3 > 1 となり, hasChildren() は true を返すが,実際には子ノードは存在しない.そして getChildren() ではオブジェクトが返ってこないため例外が発生する.
この問題自体の懸念は https://redmine.openpne.jp/issues/2684#note-5 で述べていたが,具体的にどのような問題が発生するかはこの時点では不明だった.
取らなければならない対処としては以下の2つがある.
- 子ノードが削除された時に NestedSet における正しい削除を行う.
- すでに NestedSet の構造が壊れているデータに対して修正のマイグレーションを行う
Yuya Watanabe さんがほぼ12年前に更新
管理画面から「プロフィール選択肢」を削除した場合の処理の流れ¶
apps/pc_backend/modules/profile/actions/actions.class.php 159 public function executeDeleteOption($request) 160 { 161 $this->profileOption = Doctrine::getTable('ProfileOption')->find($request->getParameter('id')); 162 $this->forward404Unless($this->profileOption); 163 164 if ($request->isMethod('post')) 165 { 166 $request->checkCSRFProtection(); 167 $this->profileOption->delete(); 168 } 169 $this->redirect('profile/list'); 170 }
ProfileOption 自体は単純にレコードを削除しているが,MemberProfile 側も on delete が cascade になっているため単純にをレコードを削除している.
これは NestedSet に対応した削除方法ではないため NestedSet として扱う場合に問題が生じる.
143 MemberProfile: ... 160 ProfileOption: { local: profile_option_id, foreign: id, onDelete: cascade } <?pre>
Yuya Watanabe さんがほぼ12年前に更新
修正例¶
diff --git a/apps/pc_backend/modules/profile/actions/actions.class.php b/apps/pc_backend/modules/profile/actions/actions.class.php index f1f0003..6663243 100644 --- a/apps/pc_backend/modules/profile/actions/actions.class.php +++ b/apps/pc_backend/modules/profile/actions/actions.class.php @@ -158,12 +158,19 @@ class profileActions extends sfActions */ public function executeDeleteOption($request) { - $this->profileOption = Doctrine::getTable('ProfileOption')->find($request->getParameter('id')); + $optionId = $request->getParameter('id'); + $this->profileOption = Doctrine::getTable('ProfileOption')->find($optionId); $this->forward404Unless($this->profileOption); if ($request->isMethod('post')) { $request->checkCSRFProtection(); + $memberProfiles = Doctrine::getTable('MemberProfile')->findByProfileOptionId($optionId); + foreach ($memberProfiles as $memberProfile) + { + $memberProfile->deleteNode(); + } + $this->profileOption->delete(); } $this->redirect('profile/list');
実験1¶
「プロフィール選択肢」で deleteNode() を用いるようにした場合
初期状態
mysql> select * from member_profile where profile_id = 5; +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------ ---------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------ ---------+ | 22 | 3 | 5 | NULL | | NULL | NULL | 22 | 1 | 4 | 0 | 2013-01-30 19:18:36 | 2013-01-30 19:18:36 | | 23 | 3 | 5 | 4 | | NULL | NULL | 22 | 2 | 3 | 1 | 2013-01-30 19:18:36 | 2013-01-30 19:18:36 | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------ ---------+
削除
mysql> select * from member_profile where profile_id = 5; +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+-----------$ ---------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+-----------$ ---------+ | 22 | 3 | 5 | NULL | | NULL | NULL | 22 | 1 | 2 | 0 | 2013-01-30 19:18:36 | 2013-01-30 19:18:43 | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+-----------$ ---------+ 1 row in set (0.00 sec)
一見成功しているように見える.
実験2¶
複数選択を複数回更新した場合.下記のような状態になりうる.
mysql> select * from member_profile where profile_id = 5; +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+-----------$ ---------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+-----------$ ---------+ | 22 | 3 | 5 | NULL | | NULL | NULL | 22 | 1 | 8 | 0 | 2013-01-30 19:18:36 | 2013-01-30 19:19:23 | | 26 | 3 | 5 | 6 | | NULL | NULL | 22 | 6 | 7 | 1 | 2013-01-30 19:19:23 | 2013-01-30 19:19:23 | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------ ---------+ 2 rows in set (0.00 sec)
削除
mysql> select * from member_profile where profile_id = 5; +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------ ---------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------ ---------+ | 22 | 3 | 5 | NULL | | NULL | NULL | 22 | 1 | 6 | 0 | 2013-01-30 19:18:36 | 2013-01-30 19:19:29 | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+------------ ---------+ 1 row in set (0.00 sec)
この挙動自体は NestedSet の仕様のため,対応しなければならない内容としては複数選択を複数回保存した場合でも正しいデータになっているようにすることと思われる.
Yuya Watanabe さんがほぼ12年前に更新
修正例2¶
diff --git a/lib/model/doctrine/MemberProfile.class.php b/lib/model/doctrine/MemberProfile.class.php index a085613..16bed25 100644 --- a/lib/model/doctrine/MemberProfile.class.php +++ b/lib/model/doctrine/MemberProfile.class.php @@ -204,7 +204,10 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn if ($this->getTreeKey() && $this->getNode()->hasChildren()) { $children = $this->getNode()->getChildren(); - $children->delete(); + foreach ($children as $child) + { + $child->getNode()->delete(); + } } }
実験¶
2つを選択して保存.
mysql> select * from member_profile where profile_id = 5; +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | 82 | 3 | 5 | NULL | | NULL | NULL | 82 | 1 | 6 | 0 | 2013-01-30 20:13:07 | 2013-01-30 20:13:07 | | 83 | 3 | 5 | 7 | | NULL | NULL | 82 | 2 | 3 | 1 | 2013-01-30 20:13:07 | 2013-01-30 20:13:07 | | 84 | 3 | 5 | 8 | | NULL | NULL | 82 | 4 | 5 | 1 | 2013-01-30 20:13:07 | 2013-01-30 20:13:07 | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ 3 rows in set (0.00 sec)
そのまま保存し直す.
mysql> select * from member_profile where profile_id = 5; +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | 86 | 3 | 5 | NULL | | NULL | NULL | 86 | 1 | 6 | 0 | 2013-01-30 20:14:08 | 2013-01-30 20:14:28 | | 88 | 3 | 5 | 8 | | NULL | NULL | 86 | 2 | 3 | 1 | 2013-01-30 20:14:08 | 2013-01-30 20:14:28 | | 89 | 3 | 5 | 7 | | NULL | NULL | 86 | 8 | 9 | 1 | 2013-01-30 20:14:28 | 2013-01-30 20:14:28 | | 90 | 3 | 5 | 8 | | NULL | NULL | 86 | 4 | 5 | 1 | 2013-01-30 20:14:28 | 2013-01-30 20:14:28 | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ 4 rows in set (0.00 sec)
原因推測¶
最初に getChildren() をしたときには正しい値をそれぞれ持っているが, 子ノードが削除されるたびに DB 上の rgt, lft が更新されていくがメモリ上の子ノードが持つ rgt, lft の値が更新されず,期待された子ノードが削除されない.
解決策としては下記のようなことが考えられる
- getChildren() を lft の降順で消してゆけば rgt, lft が変な値を持たずに削除される.
- 根ノードごと消して作成し直す.
getChildren() は lft の昇順で取得されるため,単純にリストの逆順が得られれば何とかなる.
lib/vendor/symfony/lib/plugins/sfDoctrinePlugin/lib/vendor/doctrine/Doctrine/Node/NestedSet.php 205 public function getChildren() 206 { 207 return $this->getDescendants(1); 208 } ... 215 public function getDescendants($depth = null, $includeNode = false) ... 224 $q->addWhere("$baseAlias.lft > ? AND $baseAlias.rgt < ?", $params)->addOrderBy("$baseAlias.lft asc");
Yuya Watanabe さんがほぼ12年前に更新
修正例¶
逆順で子ノードを削除する.
diff --git a/lib/model/doctrine/MemberProfile.class.php b/lib/model/doctrine/MemberProfile.class.php index a085613..9bdf498 100644 --- a/lib/model/doctrine/MemberProfile.class.php +++ b/lib/model/doctrine/MemberProfile.class.php @@ -204,7 +204,9 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn if ($this->getTreeKey() && $this->getNode()->hasChildren()) { $children = $this->getNode()->getChildren(); - $children->delete(); + for ($i = $children->count(); $i-- > 0;){ + $children[$i]->getNode()->delete(); + } } }
実験¶
2つを選択して保存.
mysql> select * from member_profile where profile_id = 5; +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | 97 | 3 | 5 | NULL | | NULL | NULL | 97 | 1 | 6 | 0 | 2013-01-30 20:55:35 | 2013-01-30 20:55:35 | | 98 | 3 | 5 | 7 | | NULL | NULL | 97 | 2 | 3 | 1 | 2013-01-30 20:55:35 | 2013-01-30 20:55:35 | | 99 | 3 | 5 | 8 | | NULL | NULL | 97 | 4 | 5 | 1 | 2013-01-30 20:55:35 | 2013-01-30 20:55:35 | +----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ 3 rows in set (0.00 sec)
保存し直す.
mysql> select * from member_profile where profile_id = 5; +-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | 97 | 3 | 5 | NULL | | NULL | NULL | 97 | 1 | 4 | 0 | 2013-01-30 20:55:35 | 2013-01-30 20:55:56 | | 100 | 3 | 5 | 7 | | NULL | NULL | 97 | 8 | 9 | 1 | 2013-01-30 20:55:56 | 2013-01-30 20:55:56 | | 101 | 3 | 5 | 8 | | NULL | NULL | 97 | 2 | 3 | 1 | 2013-01-30 20:55:56 | 2013-01-30 20:55:56 | +-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ 3 rows in set (0.00 sec)
子ノードは正しく削除されているが, lft, rgt が変な値になっている.
原因推測¶
$parent->cleaerChildren() で正しく削除されているが, $parent の lft, rgt の値自体が新しい状態に更新されていないため,それを起点にしたデータが作成される.
Yuya Watanabe さんがほぼ12年前に更新
diff --git a/lib/model/doctrine/MemberProfile.class.php b/lib/model/doctrine/MemberProfile.class.php index a085613..3af9436 100644 --- a/lib/model/doctrine/MemberProfile.class.php +++ b/lib/model/doctrine/MemberProfile.class.php @@ -204,7 +204,10 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn if ($this->getTreeKey() && $this->getNode()->hasChildren()) { $children = $this->getNode()->getChildren(); - $children->delete(); + for ($i = $children->count(); $i-- > 0;){ + $children[$i]->getNode()->delete(); + } + $this->refresh(); } }
実験¶
2つを選択して保存.
mysql> select * from member_profile where profile_id = 5; +-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | 114 | 3 | 5 | NULL | | NULL | NULL | 114 | 1 | 6 | 0 | 2013-01-30 21:06:34 | 2013-01-30 21:12:21 | | 121 | 3 | 5 | 7 | | NULL | NULL | 114 | 2 | 3 | 1 | 2013-01-30 21:12:21 | 2013-01-30 21:12:21 | | 122 | 3 | 5 | 8 | | NULL | NULL | 114 | 4 | 5 | 1 | 2013-01-30 21:12:21 | 2013-01-30 21:12:21 | +-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ 3 rows in set (0.01 sec)
そのまま保存し直す.
mysql> select * from member_profile where profile_id = 5; +-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | id | member_id | profile_id | profile_option_id | value | value_datetime | public_flag | tree_key | lft | rgt | level | created_at | updated_at | +-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ | 114 | 3 | 5 | NULL | | NULL | NULL | 114 | 1 | 6 | 0 | 2013-01-30 21:06:34 | 2013-01-30 21:12:33 | | 123 | 3 | 5 | 7 | | NULL | NULL | 114 | 2 | 3 | 1 | 2013-01-30 21:12:33 | 2013-01-30 21:12:33 | | 124 | 3 | 5 | 8 | | NULL | NULL | 114 | 4 | 5 | 1 | 2013-01-30 21:12:33 | 2013-01-30 21:12:33 | +-----+-----------+------------+-------------------+-------+----------------+-------------+----------+------+------+-------+---------------------+---------------------+ 3 rows in set (0.00 sec)
元々の子ノードが削除され,新しい状態のノードの状態も正しいため,正しい挙動をしていると思われる.
Yuya Watanabe さんがほぼ12年前に更新
- ステータス を New(新規) から Accepted(着手) に変更
【残りTODO】
- すでに正しくないデータとなっている MemberProfile について修正を行うマイグレーションスクリプトを作成する.
Yuya Watanabe さんがほぼ12年前に更新
note-12 のマイグレーションについては #3321 で対応するため,本チケットでは note-11 の内容をコミットします.
Yuya Watanabe さんがほぼ12年前に更新
- ステータス を Accepted(着手) から Pending Review(レビュー待ち) に変更
- 進捗率 を 0 から 50 に変更
更新履歴 b42f5b5b82375f40054dbb7b54aaaa2066b30187 で適用されました。
Rimpei Ogawa さんがほぼ12年前に更新
逆順 node->delete() + refresh の修正で問題ないことは確認しました。
別解を2つ考えたのでコメントしておきます。
node の delete 時に lft, rgt が最新の状態である必要があることを明示する方針で、
以下のように毎回 refresh するのもありかなと思います。(以下のコードはループ初回が冗長ですが)
--- a/lib/model/doctrine/MemberProfile.class.php +++ b/lib/model/doctrine/MemberProfile.class.php @@ -204,7 +204,12 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn if ($this->getTreeKey() && $this->getNode()->hasChildren()) { $children = $this->getNode()->getChildren(); - $children->delete(); + foreach ($children as $child) + { + $child->refresh(); + $child->getNode()->delete(); + } + $this->refresh(); } }
ただ、refresh() は内部でクエリ発行するためループの中で使うのは効率がよいとは言えず、
効率重視でいくのであればいっそ node->delete() はまったく呼ばずに Collection->delete() 後に rgt を上書きしてしまうのも手です。以下
--- a/lib/model/doctrine/MemberProfile.class.php +++ b/lib/model/doctrine/MemberProfile.class.php @@ -204,7 +204,13 @@ class MemberProfile extends BaseMemberProfile implements opAccessControlRecordIn if ($this->getTreeKey() && $this->getNode()->hasChildren()) { $children = $this->getNode()->getChildren(); - $children->delete(); + if ($children) + { + $children->delete(); + } + + $this->getNode()->setRightValue($this->getNode()->getLeftValue() + 1); + $this->save(); } }
Doctrine_Collection::delete() もループで1レコードずつ削除するので、効率という点ではそこも改善の余地はあります。(preDelete() や postDelete() が呼ばれるメリットはありますが、MemberProfile では使ってなさそう)
Rimpei Ogawa さんがほぼ12年前に更新
- ステータス を Pending Review(レビュー待ち) から Pending Testing(テスト待ち) に変更
- 進捗率 を 50 から 70 に変更
b42f5b5b の修正は問題ないためレビュー済みとします。