ローカル環境やイントラネットに置かれた古いシステムなどは、サポートがとっくに切れて何年も経過しているPHPのまま動いているものも世の中に少なくありません。ですが、そんなシステムもサーバーのアップデートや換装などで動作のほころびが発生してくるので、改修などが必要になってくる場合があります。
ですが、そういうシステムに限って、過去に数社、何人ものエンジニアが継ぎ足し継ぎ足し改修をかけているので、中身が完全に入り組んだソースが書かれたスパゲッティコードになっていることも珍しくありません。
本案件のWEBシステム(以下システムと表記)はPHP4.xから稼働し、何度か改修の痕跡があり、PHP5.5時代を最後にマイナー改修されたまま半放置状態だったもので、一部機能が完全に動作していませんでした。それを本システムに対し、PHP7.4まで対応させ、完全に動作するように修正した際に、どのようなことを行ったかという実務に基づいた記録です。
まずは現状把握
いきなり改修作業に入らず、まずは、どの機能が働いていて、またどの機能が働いていないのかを確認する必要があります。そして、それをページごとに記録した上で、プログラムにメスを入れていくことになります。
本案件では一番大きな問題としてCRUDが全く機能していませんでした。
エンコードを統一
古いファイルはEUC-JPやSJISで記述されていることも少なくありませんが、よほどのことがない限り、昨今UTF-8以外は推奨されておりません。したがって、まずは全ファイルのエンコードを全部UTF-8に変更してから、その言語が読み取れるか確認しておきましょう。DBの関係でエンコードを施している場合もあるかと思われますが、その場合もエンコードを合わせておく必要があります。
システムをブラウザで確認してみた際に、一定の値だけ文字化けしているケースは、DBテーブルのデータ部分が文字化けしていることが多いです。
改修作業に入る前に
これで準備できたら、いよいよプログラムの中身を見ていくことになります。ですがPHP開発者の間では、エラー隠蔽という悪習が蔓延しており、スパゲッティー化したコードは、これが改修の邪魔をしていることが多いです。
隠蔽していたエラーを表示
PHPでは設定一つでエラーを隠蔽できてしまうというエラー制御設定が実装されています。php.iniというPHPの設定ファイルに
display_errors = off
こう書いてしまうだけで、warningはおろか、noticeまで非表示になってしまうので、悪魔の囁きでついついやってしまいがちです。
ですが、これこそ原因の発見、改修の妨げの元凶にもなるバッドノウハウの極致です。noticeぐらい問題ないだろう、そんな軽い気持ちが後々の大変な事態を招きかねません。また、自分がシステムを開発するときは必ず、display_errors=Onにして、完成品でもそのままdisplay_errors=Offにせず納品するようにしています。そうしないと、動作中に不具合が発生しても、その原因を突き止められないからです。
そして本案件ですが、エラーを表示してみたところ全数十ファイルで2,000件以上検出されました。ですが、そのエラーメッセージを追っていくと、だいたい以下の4つが9割以上を占めていました。
undefined variable hoge
未定義変数によって起きる軽度エラー（notice）です。そして、この変数定義が面倒だからといってdisplay_errorsをoffにして納品していたというケースが跡を絶ちません。ですが、こんな悪習は絶対にやめましょう。きちんと変数は定義しておくべきです。
だからといって、このエラー対応も気をつけないと落とし穴にハマります。本案件ではこんな例がありました。最初のエラー表示で$hogeが未定義だったので、以下のように定義しておいたのですが、
require_once($_SERVER['DOCUMENT_ROOT']."/func/func_hoge.php");
$hoge = "";
これで処理を行うと、処理がうまく動作しませんでした。原因を調べるとrequre_onceで呼び出された外部ファイルの中に変数$hogeの値を取得するような記述があったためです。ですので、この場合は外部ファイル呼び出し前に変数を定義しておく必要がありました。
$hoge = "";
require_once($_SERVER['DOCUMENT_ROOT']."/func/func_hoge.php");
これで問題なく動作、かと思えばこれも正しく動作しません。更に分析してみたら$hogeに代入されていたのは数値でした。ですので、この場合はNULLあるいは0を初期値に設定しておく必要がありました。
$hoge = NULL;
require_once($_SERVER['DOCUMENT_ROOT']."/func/func_hoge.php");
このように未定義変数を放置しておくと、システムのどこでIO(入出力)が実行されているか不明確になってしまうことが往々にしてあるので、絶対に変数は定義し、上記のエラーは表示させないようにしましょう。
undefined index hoge
未定義のキーを含んだ配列を呼び出した場合に発生する軽度エラー（notice）です。むしろ、こっちの対応の方が面倒だから、後でdisplay_errorsをoffにしていることが多いです。たしかに、上記よりこっちの方が後々発生するバグの頻度は低いのですが、配列に対して値を記入しなかった場合とnullの場合などの判別が曖昧になり、後々の処理で困難を招くことが起きたりします。
if(isset($ar_hoge['fuga'])) $fuga = $ar_hoge['fuga'];
このように、isset関数を使って配列キーの有無を確認しておきましょう。ただ、isset関数はNULLでもtrueを返してしまうので、そこの対応もしっかりしておく必要があります。
ちなみに、PHP7.4だとNULL合体演算子を使えます（メールにて助言頂きました、ありがとうございます）
$fuga = $ar_hoge['fuga'] ?? NULL;
古い記述に見られるエラー
時代の古いファイルを処理していると必ずといっていいほど直面するエラーです。こういうのも隠蔽体質で放置しておくのは得策ではありません。
Warning: Use of undefined constant hoge - assumed hoge
そのまま読めば、未定義の定数が使用されているというエラーですが、そのエラー発生源は配列のキー設定で、古いPHPは配列のキーに対してクォートで囲まなくても問題なかったのですが、PHP5以降は非推奨になりました。なので、そのキー表記を放置しておくと上記のエラーが発生します。キーはきちんとクォートで囲みましょう。
$hoge[fuga] = $piyo; //昔は許容された書き方
$hoge['fuga'] = $piyo; //このようにクォートで囲むこと
ただし、inputタグなどのnameプロパティの場合はそのままにしておきましょう。クォートで囲むとクォートもキー名の一部にみなされてしまい、困った事態が発生します。
<input type="text" name="hoge[fuga]"><!-- うっかりクォートで囲まないこと -->
Delimiter must not be alphanumeric or backslash
preg_matchなど正規表現でマッチングを行う関数に際し、デリミタを設定しない場合に起きるエラーで、かつては未使用でも動作が保証されていましたが、これも非推奨の記述になりました。なので、/を用いるか、あるいは/でエスケープを記述したくない場合は{}などで囲む必要があります。
preg_match("hoge/fuga",$uri); //デリミタで囲んでいないと警告が表示される
preg_match("/hoge\/fuga/u",$uri); //スラッシュをエスケープ
preg_match("{hoge/fuga}u",$uri); //スラッシュをエスケープしない
廃止（予定含む）関数を使っている場合
display_errors=Offは、このように廃止されているにもかかわらず使用されたままの関数使用を見逃す危険があり、動作が動かなくなった原因の一つはこれであることも多く、それこそエラー隠蔽によって気づかないままになってしまうことも少なくありません。
廃止された関数は未定義関数とみなされ使用不可になっているため、undefined function hogeと表示され、これは致命的なエラー（未定義関数エラー）なので動作が停止します。書き換え対象として多いのが正規表現のマッチングを用いたereg_hoge（ereg_match、ereg_replaceなど）という関数で、これはPHP7以降はpreg_hoge関数（PCRE関数）しか対応していません。また、each関数などもPHP7.2で非推奨、8以降で廃止されており、これは複数の配列をforeachでループするときに用いたりしたので、適宜書き換える必要があります。
実践的な改修作業のために
これで最低限動くようになったら、改修を円滑に進めるために可読性を高めておきましょう。
変数を明示的に書き換える
スパゲッティ化したコードにはある共通点があります。それは、意味が曖昧な変数を使用していることで、変数が乱立し、それによって無駄な処理が増えていることです。今回の案件ではテーブルの件数を表示させるのに、3回も別の変数で制御していた、という例がありました。そんな変数は名前を書き換えておきましょう。
自分の場合、変数には法則性を持たせるようにしています。最低限、配列を格納する変数と文字列を格納する変数は分類しておきましょう。
//変数の名前の例
$ar_hoge = []; //arはarray（配列）を表す
$tbl_hoge = ""; //出力用のテーブル
$list_hoge = "" ; //出力用のリスト
$cnt_hoge = ""; //数値、個数のカウント用
$hoge_flg = true; //判定用フラグ、hogeは何の判定なのかを記述
このような感じでルールを設けています。なぜ、これを実施するかですが、改修作業において一番大事なのはデータの流れを把握することだからです。どこから入力されて、どう加工されて、どう出力されていくのか、その変数の出入り口を把握することこそが改修作業の鍵なので、だからこそ可読性の高い変数に書き換えておく作業が重要だと自分は考えているからです。
if文でプログラムの分岐を追う
どんなに煩雑なスパゲッティコードでも今まで動作していたということは、最低限の処理の分岐はできているわけです。なので、変数を可読性の高いものに書き換えたなら、次は具体的な処理より、表面的なプログラムの分岐を追っていきましょう。そして、コメントも分岐のところに書いておくといいです。
そうすることで、無駄な分岐や無駄な処理を発見することもできます。本案件では２割ぐらい無駄な処理をカットすることができました。
htmlは極力出力だけにする
PHPの特徴として、簡単にhtml部分に変数どころか処理も埋め込めるというメリットがありますが、これが同時に悪弊を広めています。それは何かというと処理部分もhtml側に記述したりして、データの流れを追いにくい支離滅裂な状態になってしまっている場合が多いからです。なので、html部分は極力変数を出力するだけにしておいた方がシステム内の変数の流れも追いやすく、デバッグも円滑に進めることができます。
繰り返す処理は関数化
データの流れを把握したら、次はプログラムが実際どんな処理を行っているかを具体的に見ていきましょう。だいたいは分岐用のフラグがあって、そのフラグにしたがって、似たようで異なる制御が施されている、そんなコードが多いはずです。
そしてスパゲッティコードほど、同じ処理を記述していることが多かったりもします（前に改修した人がきちんとデータを読み切れていないので）が、そこはどんどん外部関数化していった方がいいでしょう。時折、やっていることが同じなのに別の関数を使ったりしていた、なんて場合もあります。
それが一定の工数や流れを持っている場合は、外部ファイル化、クラス化するのもありです。
本案件では、５割以上は冗長なプログラムを省力化できました。
ただし、それらはデータの入出力、加工の流れを完全に把握している場合にとどめてください。きちんと把握していない状態で関数化をしたりすると、微妙な処理の差異を放置し、致命的な動作エラーに直結することがあります（削除にかかわるフラグを放置していたなど）。
再構築は奥の手
このリファクタリング（改修）作業ですが、根本から書き換える再構築は行いませんでした。原因の第一の理由は予算の問題ですが、それを置いといても、根本を書き換えるということは、いわば一から作成することになるので、仕様書なども紛失している（本案件では制作会社は消滅済）システム案件で、それを実施するのはとんでもないリスクを伴う（正常に動作している複雑な処理部分まで制御不能にしてしまう恐れがある）からです。
スクリプト部分は手を入れない
逆にJavaScriptで制御している部分は後方互換性を持っていることが多いので、そのままにしておいた方がいいでしょう。下手にいじると、どこで問題が発生したのかが追跡できなくなりますし、それこそ動作に不具合が生じる原因はだいたい、後方互換性を持っていないPHP側にあることが大半だからです。
セキュリティ対策は最後の最後に
もっとも目まぐるしく対策が入れ替わっているのはセキュリティ対策の部分で、昔だとXSSとSQLインジェクションすら対応していない場合もあります。ですが、それらに手を付けるのは、完全に動作を確認してからにしましょう。