PHP-CS-Fixer の導入 #996

nanasess · 2024-09-05T08:58:01Z

~~strlen($val) で null チェックしているのを修正するのが主目的~~
- 未定義変数や連想配列での扱いに副作用があるため除外
その他、統一した方が良さそうなルールを適用

PHP8.3で以下のコマンドを実行

PHP_CS_FIXER_IGNORE_ENV=1 data/vendor/bin/php-cs-fixer fix  --allow-risky=yes

codecov · 2024-09-05T09:03:25Z

Codecov Report

Attention: Patch coverage is 46.02122% with 814 lines in your changes missing coverage. Please review.

Project coverage is 56.23%. Comparing base (74eaa0a) to head (5c6ecce).

Files with missing lines	Patch %	Lines
data/class/helper/SC_Helper_Customer.php	7.19%	129 Missing ⚠️
data/class/helper/SC_Helper_CSV.php	0.00%	105 Missing ⚠️
data/class/helper/SC_Helper_Transform.php	0.00%	59 Missing ⚠️
data/class/SC_Image.php	2.50%	39 Missing ⚠️
data/class/SC_Fpdf.php	11.76%	30 Missing ⚠️
data/class/api/SC_Api_Operation.php	0.00%	30 Missing ⚠️
data/class/helper/SC_Helper_PageLayout.php	25.64%	29 Missing ⚠️
data/class/api/SC_Api_Utils.php	0.00%	28 Missing ⚠️
data/class/api/operations/ItemLookup.php	0.00%	28 Missing ⚠️
data/class/SC_Initial.php	0.00%	27 Missing ⚠️
... and 44 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #996      +/-   ##
==========================================
- Coverage   56.29%   56.23%   -0.07%     
==========================================
  Files          75       75              
  Lines        8917     9046     +129     
==========================================
+ Hits         5020     5087      +67     
- Misses       3897     3959      +62

Flag	Coverage Δ
tests	`56.23% <46.02%> (-0.07%)`	⬇️

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.

seasoftjapan · 2024-09-05T09:49:30Z

6f20c7d は、全て PHP-CS-Fixer による変更ですか?

ローカルで bfc9f5d から実行を試したのですが、結果が異なる様子でして。

$ data/vendor/bin/php-cs-fixer fix --allow-risky=yes --dry-run
Loaded config default from "****/.php-cs-fixer.dist.php".
   1) data/class/SC_Customer.php
   2) data/class/helper/SC_Helper_TaxRule.php
   3) data/class/helper/SC_Helper_CSV.php
   4) data/class/SC_Fpdf.php
   5) data/class/pages/guide/LC_Page_Guide_Kiyaku.php
   6) data/class/pages/shopping/LC_Page_Shopping.php
   7) data/class/pages/products/LC_Page_Products_List.php
   8) data/class/pages/admin/contents/LC_Page_Admin_Contents_Recommend.php
   9) data/class/pages/admin/contents/LC_Page_Admin_Contents_RecommendSearch.php
  10) data/class/pages/admin/mail/LC_Page_Admin_Mail_Preview.php
  11) data/class/pages/admin/design/LC_Page_Admin_Design.php
  12) data/class/pages/admin/design/LC_Page_Admin_Design_MainEdit.php
  13) data/class/pages/admin/order/LC_Page_Admin_Order_Mail.php
  14) data/class/pages/admin/order/LC_Page_Admin_Order_Edit.php
  15) data/class/pages/admin/order/LC_Page_Admin_Order_Disp.php
  16) data/class/pages/admin/order/LC_Page_Admin_Order_ProductSelect.php
  17) data/class/pages/admin/products/LC_Page_Admin_Products_Product.php
  18) data/class/pages/mypage/LC_Page_Mypage_Favorite.php
  19) data/module/HTTP/Request.php

Checked all files in 20.817 seconds, 24.000 MB memory used

正しい実行手順を教えていただけますと幸いです。

nanasess · 2024-09-05T12:34:07Z

@seasoftjapan
そのままだと composer.json の config.platform.php: 7.4.0 が有効になっていて、PHP8.0以降のルールが適用されないようです。
以下でいかがでしょうか？

PHP_CS_FIXER_IGNORE_ENV=1 data/vendor/bin/php-cs-fixer fix --allow-risky=yes

また、 --diff オプションをつけていただくと、原因がわかりやすいです

nanasess · 2024-09-05T12:34:24Z

テストが落ちているようなので、一旦 Draft に戻します

nanasess · 2024-09-06T00:59:39Z

未定義変数、連想配列の扱いに難がありそう

- string_length_to_empty - ternary_to_elvis_operator

seasoftjapan · 2024-09-06T09:59:32Z

data/class/SC_CartSession.php

@@ -741,12 +751,12 @@ public function isDelivFree($product_type_id)
     * @param SC_Customer   $objCustomer   ログイン中の SC_Customer インスタンス
     * @param integer       $use_point     今回使用ポイント
     * @param integer|array $deliv_pref    配送先都道府県ID.
-                                        複数に配送する場合は都道府県IDの配列


消失しているようです。

seasoftjapan · 2024-09-06T10:13:33Z

data/class/api/operations/CartAdd.php

@@ -41,7 +40,7 @@ class API_CartAdd extends SC_Api_Abstract_Ex
    public function doAction($arrParam)
    {
        $this->arrResponse = array(
-            'Version' => ECCUBE_VERSION);
+            'Version' => ECCUBE_VERSION, );


カンマの後、改行してくれるオプションとか無いでしょうか?
いくつか、気持ちの悪い(中途半端な)印象の整形がありました。

trailing_comma_in_multiline が有効になっていると、複数行の配列で、末尾は改行しないパターンでこのようになってしまうようです。
複数行を強制するルールは実装されていないようなので、 trailing_comma_in_multiline => false にしたいと思います
https://cs.symfony.com/doc/rules/control_structure/trailing_comma_in_multiline.html

seasoftjapan · 2024-09-06T10:17:06Z

data/class/api/operations/ItemSearch.php

@@ -125,27 +125,27 @@ protected function getProductsList($searchCondition, $disp_number, $startno, $li
            case 'price':
                $objProduct->setProductsOrder('price02', 'dtb_products_class', 'ASC');
                break;
-            // 販売価格が高い順
+                // 販売価格が高い順


これ、救えないですかね。case のコメントで、case の真上に欲しいです。

'statement_indentation' => ['stick_comment_to_next_continuous_control_statement' => true] とすることで対応できそうです。
https://cs.symfony.com/doc/rules/whitespace/statement_indentation.html
が、 composer の依存関係の問題で、現在 php-cs-fixer 3.9.5 がインストールされています。
このバージョンでは上記オプションは未実装のため、利用できないようです。
PHPUnit のバージョンを上げることで、上記オプションも利用できるようになりそうなので、別途対応しようと思います。

seasoftjapan · 2024-09-06T10:26:17Z

.php-cs-fixer.dist.php

+    'is_null' => false,
+    'concat_space' => ['spacing' => 'one'],
+    'native_constant_invocation' => false,
+    'array_syntax' => false,


デフォルト short だと思いますが、不都合ありそうでしたか?
個人的には、少なくとも混在するのは嫌で、統一するなら short 一択かなといった印象です。

変更が多くなりすぎるため、2.13からバージョンアップする際のコンフリクト解消が辛くなるんじゃないかと思って変換しないようにしてました。

2.13 の状態で、一旦 php-cs-fixer のルールを適用

2.17 のブランチをマージ
とすれば、少しはコンフリクトもマシになりそうですかね

array_syntax のみの影響度合いは把握できていませんが、他の項目でスペースを変更されるのもマージ時には辛いかもとは思いました。私も対処方法として、2.13 側にも PHP-CS-Fixer を同じ設定で適用するのが良さそうに思いました。

seasoftjapan · 2024-09-06T10:52:01Z

.php-cs-fixer.dist.php

+    '@Symfony:risky' => true,
+    // '@PHP83Migration' => true,
+    'psr_autoloading' => false,
+    'single_quote' => false,


デフォルトでなかなか良い感じの置換をする様子ですが、目に付くパターンありましたか?

'single_quote' ですかね。こちらも array_syntax と同じ理由で false にしていました。

2.13 に php-cs-fixer を適用する手順で問題なさそうであれば、 true にしたいと思います

'single_quote' ですかね。

はい。1行を指定してコメントつけたつもりですが、上3行も自動引用されたようで、分かりにくくなり恐縮です。

seasoftjapan · 2024-09-06T10:57:43Z

.php-cs-fixer.dist.php

+    'single_quote' => false,
+    'is_null' => false,
+    'concat_space' => ['spacing' => 'one'],
+    'native_constant_invocation' => false,


これが適用されていなそうな置換が見受けられます。

例

- trigger_error('最大階層制限到達', E_USER_ERROR); + trigger_error('最大階層制限到達', \E_USER_ERROR);

当方環境 (PHP 8.3.9 / FROM ghcr.io/ec-cube/ec-cube2-php/8.3-apache) では、置換されないようです。

手元の環境の問題っぽいです。確認してみます

native_constant_invocation が適用されていたようですので対応します

seasoftjapan · 2024-09-06T11:12:11Z

.php-cs-fixer.dist.php

+    throw new \LogicException();
+}
+
+$rules = [


設定理由をソースコメントに残していただけると、参考になります。
特に無効化している項目ですが、具体的に不都合があったのか、何となく避けたかといった辺り分かると、今回は対応を見送っても、今後の対応拡大に繋げやすいと思います。
下に、何項目かコメントつけましたが、無理に今回対応しなくても、後々で良いのかなと思う部分もあります。

seasoftjapan · 2024-09-06T11:25:05Z

data/class/SC_Batch.php

@@ -24,8 +24,8 @@
 /**
 * バッチ処理用 の基底クラス.
 *
- * @package Page


@Package は不要との判断でしょうか?

以前に、phpDocumentor で出力したときに「あー分類されるんだ」と思ったくらいで、さほど必要になることは無いと思いますが。

rules 的には対象でない気がして、気になりました。

@Symfony で phpdoc_no_package が適用されているようです。
https://cs.symfony.com/doc/rules/phpdoc/phpdoc_no_package.html
phpdoc_no_package => true で良いと思います

nanasess · 2024-09-09T00:11:13Z

@seasoftjapan ありがとうございます！おおむね対応可能だと思いますので、順次対応していきますね

nanasess · 2024-09-29T15:14:03Z

2.13.x のソースに対しても同様のルールを適用できそうなので、これを活用すれば旧バージョンからのマージや PHP8 対応も楽にできそう

- コメント追記 - `single_quote => true` に変更(4系にあわせる) - `concat_space => ['spacing' => 'none']` に変更(4系にあわせる) - `array_syntax => true` に変更(4系にあわせる) - `phpdoc_scalar => true` に変更(phpdoc の int や bool を統一したい) - `data/smarty_extends` 及び `tests` にも適用

…low-risky=yes

nanasess · 2024-09-30T17:11:13Z

@seasoftjapan
以下のルールを見直してみました

single_quote => true に変更(4系にあわせる)
concat_space => ['spacing' => 'none'] に変更(4系にあわせる)
array_syntax => true に変更(4系にあわせる)
phpdoc_scalar => true に変更(phpdoc の int や bool を統一したい)
data/smarty_extends 及び tests にも適用

各ルールにコメントも入れておきましたのでご確認お願いいたします🙇‍♂️
94d493c

PHP-CS-Fixer の導入

bfc9f5d

nanasess requested review from seasoftjapan and nobuhiko September 5, 2024 09:00

nanasess marked this pull request as draft September 5, 2024 12:34

nanasess added 2 commits September 6, 2024 10:36

Merge remote-tracking branch 'upstream/master' into php-cs-fixer

4381058

副作用のありそうなルールを除外

9326ac0

- string_length_to_empty - ternary_to_elvis_operator

nanasess force-pushed the php-cs-fixer branch from a796588 to dee413e Compare September 6, 2024 02:23

nanasess marked this pull request as ready for review September 6, 2024 02:43

nanasess enabled auto-merge September 6, 2024 02:43

seasoftjapan mentioned this pull request Sep 6, 2024

strlen($val) をどうするか #999

Open

seasoftjapan reviewed Sep 6, 2024

View reviewed changes

nanasess added 2 commits October 1, 2024 00:43

php-cs-fixer を適用すると削除されるため1行に修正

e96e7fe

nanasess force-pushed the php-cs-fixer branch from dee413e to 872e0eb Compare September 30, 2024 17:01

nanasess added 2 commits October 1, 2024 02:03

Merge remote-tracking branch 'upstream/master' into php-cs-fixer

a9a7e00

Apply PHP_CS_FIXER_IGNORE_ENV=1 data/vendor/bin/php-cs-fixer fix --al…

5c6ecce

…low-risky=yes

nanasess force-pushed the php-cs-fixer branch from 872e0eb to 5c6ecce Compare September 30, 2024 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP-CS-Fixer の導入 #996

PHP-CS-Fixer の導入 #996

nanasess commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

seasoftjapan commented Sep 5, 2024

nanasess commented Sep 5, 2024 •

edited

Loading

nanasess commented Sep 5, 2024

nanasess commented Sep 6, 2024

seasoftjapan Sep 6, 2024

seasoftjapan Sep 6, 2024

nanasess Sep 30, 2024 •

edited

Loading

seasoftjapan Sep 6, 2024

nanasess Sep 30, 2024

seasoftjapan Sep 6, 2024

nanasess Sep 20, 2024

seasoftjapan Sep 21, 2024

seasoftjapan Sep 6, 2024

nanasess Sep 20, 2024

seasoftjapan Sep 21, 2024

seasoftjapan Sep 6, 2024

nanasess Sep 20, 2024

nanasess Sep 30, 2024

seasoftjapan Sep 6, 2024

seasoftjapan Sep 6, 2024

nanasess Sep 30, 2024

nanasess commented Sep 9, 2024

nanasess commented Sep 29, 2024

nanasess commented Sep 30, 2024

PHP-CS-Fixer の導入 #996

Are you sure you want to change the base?

PHP-CS-Fixer の導入 #996

Conversation

nanasess commented Sep 5, 2024 • edited Loading

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

seasoftjapan commented Sep 5, 2024

nanasess commented Sep 5, 2024 • edited Loading

nanasess commented Sep 5, 2024

nanasess commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanasess Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanasess commented Sep 9, 2024

nanasess commented Sep 29, 2024

nanasess commented Sep 30, 2024

nanasess commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading

nanasess commented Sep 5, 2024 •

edited

Loading

nanasess Sep 30, 2024 •

edited

Loading