空DELETEを抑制する

[nobuhiko]

Fixes #630

[codecov]

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 51.64%. Comparing base (2fe0c3a) to head (dbc6926).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
data/class/SC_Query.php	50.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1097   +/-   ##
=======================================
  Coverage   51.63%   51.64%           
=======================================
  Files          80       80           
  Lines       10223    10224    +1     
=======================================
+ Hits         5279     5280    +1     
  Misses       4944     4944           

Flag	Coverage Δ
tests	51.64% <60.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nanasess]

LGTM

[bbkids]

data/class/SC_Query.php
// 空deleteは実行しない
if ($this->count($table, $where, $arrWhereVal) == 0) {
return;
}

このコード入れるとどうも管理画面にログイン出来なくなります。

[nobuhiko]

@bbkids
PHP 7.4.33
MySQL 5.7.44
ではログイン出来ました。

[bbkids]

本当に申し訳御座いません。
まだきちんと解析出来でいないのですが、古い商品ランキングのプラグインが原因のようです。

管理画面にログインするタイミングでランキングが更新されるのですが、
SC_Query.phpにコード適用後にそのプラグインが原因で以下のエラーが出ている事がわかりました。

プラグイン側に問題があるのだと思います。すみませんでした。

---

[/admin/home.php] Fatal error(E_USER_ERROR): DB処理でエラーが発生しました。
SQL: [SELECT COUNT() FROM dtb_ranking ORDER BY total DESC ]
MDB2 Error: no such field
_doQuery: [Error message: Could not execute statement]
[Last executed query: PREPARE mdb2_statement_mysqli_10a0b49bd0566f968c4d56aedeff4eb9bbeac88c64 FROM 'SELECT COUNT() FROM dtb_ranking ORDER BY total DESC ']
[Native code: 1054]
[Native message: Unknown column 'total' in 'order clause']

---

[seasoftjapan]

MySQL は DELETE で ORDER BY を使えるので、カスタマイズでは支障となるパターンがありそうですね。(@bbkids 様の報告されたケースがそれに該当するかは未検証です。)

SC_Query::count()、SC_Query::exists() 側でケアしても良いのかも。
根本的には、SC_Query::setOrder()、SC_Query::setLimit() あたりが微妙だと常々思いますが、手出ししにくいですね...

[seasoftjapan]

MySQL は DELETE で ORDER BY を使えるので、カスタマイズでは支障となるパターンがありそうですね。

ざっと確認しましたが、delete() は、setOrder() や setLimit() の影響を受けないようなので、カスタマイズの観点では問題無さそうでした。

setOrder() など → select() など → delete() の流れを同一 SC_Query インスタンスで実行するとコケる問題のようです。

個人的には、上で言う「select() など」の時点で、setOrder() の効力を失効される方が良い気がするのですが、どうでしょう？→ #1116 に展開しました。

多分、@bbkids 様の報告ケースは、それで回避されそうに思います。また、EC-CUBE 本体の実装でも LC_Page_Mypage_Favorite::lfGetFavoriteProduct() のように同一メソッド内で何度も SC_Query_Ex::getSingletonInstance() する必要が無くなると思います。
ちなみに、setLimit() は現にそうなっています。setOrder() と setLimit() で有効範囲が異なる現状は、もはやバグレベルのようにも思います。

気持ち悪い例

        $objQuery = SC_Query_Ex::getSingletonInstance();
        $objQuery->begin();
        $objQuery->delete('dtb_bkup');
        $objQuery->insert('dtb_bkup', ['bkup_name' => 'example1']);
        $objQuery->insert('dtb_bkup', ['bkup_name' => 'example2']);
        $objQuery->insert('dtb_bkup', ['bkup_name' => 'example3']);
        var_export($objQuery->select('bkup_name', 'dtb_bkup'));
/*
array (
  0 => 
  array (
    'bkup_name' => 'example1',
  ),
  1 => 
  array (
    'bkup_name' => 'example2',
  ),
  2 => 
  array (
    'bkup_name' => 'example3',
  ),
)
*/
        $objQuery->setLimit(1);
        $objQuery->setOrder('bkup_name DESC');
        var_export($objQuery->select('bkup_name', 'dtb_bkup'));
/*
array (
  0 => 
  array (
    'bkup_name' => 'example3',
  ),
)
*/
        var_export($objQuery->select('bkup_name', 'dtb_bkup'));
/*
array (
  0 => 
  array (
    'bkup_name' => 'example3',
  ),
  1 => 
  array (
    'bkup_name' => 'example2',
  ),
  2 => 
  array (
    'bkup_name' => 'example1',
  ),
)
*/
        $objQuery->rollback();

[bbkids]

setOrder() など → select() など → delete() の流れを同一 SC_Query インスタンスで実行するとコケる問題のようです。

仰る通りで、導入していた古いプラグインが同一 SC_Query インスタンスでsetOrder() → select() → delete() の流れを実行している事が原因でした。
取り急ぎ現状は同一メソッド内で何度かSC_Query_Ex::getSingletonInstance() を実行し回避する事にしました。