はじめに
「リファクタリングしたい」
エンジニアなら誰もが一度は思ったことがあるはず。
でも、今回の話は 「リファクタリングしなかった」 話です。
PHP5.6で動いていた10年以上前のレガシーシステムをPHP8.2に移行する案件を担当しました。
バージョン管理の痕跡すらない、コピペの嵐、SQLインジェクションだらけ・・・
そんなコードを前にして、私は リファクタリングしない 決断をしました。
※記事内のコードは事実を元に再構成したものです。
なぜリファクタリングしなかったのか
✅ 動いているコードは正義
10年以上動いてきた実績がある。
バグがあるかもしれないけど、それも含めて「仕様」として運用されてきた。
下手に触ると、その「仕様化されたバグ」に依存した運用が壊れる。
🏢 内部システムだから
外部からのアクセスがない社内システム。
SQLインジェクションがあっても、攻撃者がいない。
セキュリティリスクは認識しつつも、優先度は下がる。
まあ、外部システムだったらこんなやべえコードにはなってなかったんだろうけど・・・
🧪 テストがない
ユニットテストもE2Eテストもない。
リファクタリングしても、正しく動いているか確認する手段がない。
「動いてるからOK」を証明できない。
📄 ドキュメントがない
仕様書がない。コメントもない。
なぜそう書いたのか、誰にもわからない。
意図を推測しながらリファクタリングするのは危険すぎる。
💰 予算と納期
リファクタリングには時間がかかる。
クライアントは「PHP8で動くようにしてほしい」としか言っていない。
美しいコードを求めているわけじゃない。
⚖️ 責任の所在
リファクタリングして壊したら、私の責任。
そのまま移行して壊れたら、元のコードの責任。
この違いは大きい。
👥 次に触る人のため
リファクタリングすると、元の構造が見えなくなる。
レガシーコードのまま残しておけば、次の人も「これがレガシーだ」とわかる。
中途半端にリファクタリングすると、どこまでが元でどこからが新しいのかわからなくなる。
⏰ 全箇所修正は工数的に不可能
同じ問題が何十箇所、何百箇所とある。
全部直すのは現実的じゃない。
「動くようにする」が目的であって「綺麗にする」が目的じゃない。
遭遇したやべえコード
やべえレベルが高い順に上位8つを紹介。
1位: SQLインジェクション
// ユーザー入力をそのままクエリに結合
$sql = "SELECT * FROM users WHERE id = " . $_GET['id'];
$result = mysql_query($sql);
?id=1 OR 1=1 でDBの中身が丸見え。
?id=1; DROP TABLE users;-- で全データ消失。
教科書に載ってるレベルの脆弱性がそのまま本番環境に。
2位: GETパラメータで認証バイパス
// 各画面でGETパラメータからユーザーIDを取得
$Staff_No = $_GET['Staff_No'];
// このIDでデータを取得・表示
$query = "SELECT * FROM user_data WHERE Staff_No = '".$Staff_No."'";
URLの?Staff_No=1を?Staff_No=2に変えるだけで他人のデータにアクセス可能。
認証の意味がない。パスワードなんて飾り。
3位: パスワード平文保存
// データベースにそのまま保存
$sql = "INSERT INTO users (password) VALUES ('" . $_POST['password'] . "')";
ハッシュ化なし。ソルトなし。
DBが漏洩したら全ユーザーのパスワードがそのまま流出。
4位: DB認証情報がソースコードに直書き
// 変数すら使わず直接ハードコード
$conn = mysql_connect("192.168.1.100", "admin_user", "SecretPass123!");
mysql_select_db("production_db", $conn);
// 別ファイルでも同じ認証情報がコピペ
$conn = mysql_connect("192.168.1.100", "admin_user", "SecretPass123!");
本番IPアドレスもパスワードも丸見え。
ソースコードが漏洩したらDBに直接アクセス可能。
5位: 謎のフラグ管理
if ($flag == 1) {
// 処理A
} elseif ($flag == 2) {
// 処理B
} elseif ($flag == 3) {
// 処理A + 一部処理B
} elseif ($flag == 4) {
// 処理Bの亜種
} elseif ($flag == 5) {
// 処理A + 処理B + 特殊処理
}
// ... flag == 20 まで続く
フラグの意味がどこにも書いてない。
「3」が何を表すのか、神のみぞ知る。
6位: コピペの嵐
// file_a.php
function GetDetailVal($office_no, $name) {
// 実装A（50行）
}
// file_b.php
function GetDetailVal($office_no, $name) {
// 実装B（微妙に違う50行）
}
// file_c.php
function GetDtailVal($office_no, $name) { // typoバージョン
// 実装C
}
同じ名前の関数が複数ファイルに存在。しかもtypoバージョンまである。
どれが正しいか不明。修正しても全ファイルに反映されない。
7位: グローバル変数の乱用
$glbLoginID; // グローバルだがundefined
function doSomething() {
global $LogStaffNo; // 関数内でglobal宣言
global $TSPassword;
// ...
}
// セッションとグローバル変数が混在
$_SESSION["Login_ID"] = $result[0]['Login_ID']; // セッション
$LogStaffNo = $result[0]['Staff_No']; // グローバル変数
変数のスコープが追えない。どこで書き換わるか不明。
8位: HTMLとPHPのスパゲッティ
<td class="<?php echo $bgcolor1;?>">
<input type="text"
value="<?php echo ($_POST['field_'.$j.'']!=""
?$_POST['field_'.$j.'']
:(($data[$j]!=""
?$data[$j]
:$error!="")
?$defaultVal[$j][$l]
:$fallback))?>" />
</td>
三項演算子のネストが深すぎて解読不能。
これを読める人、天才か変態。
まだまだやべえコードたくさんあったけど、長くなっちゃうのでこの辺で・・・
PHP8移行で遭遇したエラー
「なんでこれ今まで許されてた？画面にエラー出なかったの？」
と思ったら、php.iniに・・・
error_reporting = E_ERROR | E_PARSE | E_CORE_ERROR | E_COMPILE_ERROR | E_RECOVERABLE_ERROR
Warning/Noticeは非表示かよ！
PHPって結構許容範囲が広くて、変数未定義くらいじゃErrorじゃなくてWarning扱い。
そんなPHPでWarningを非表示にしちゃう危険性・・・
表面上は動いてるけど、裏では大量のWarningが出てたってこと。
以下、PHP8で動かなくなった代表的なやつ。
1. count(null) が Fatal Error に
// PHP7以前: Warning出してたけど動いてた
// PHP8: Fatal error: Uncaught TypeError
$count = count($maybeNull);
対処法:
$count = is_countable($maybeNull) ? count($maybeNull) : 0;
2. NULL との算術演算が TypeError に
// PHP7以前: NULLを0として計算してた
// PHP8: Uncaught TypeError
$total = $price + $tax; // $tax が NULL の場合
対処法:
$total = $price + ($tax ?? 0);
3. mysql_* 関数が完全削除
// PHP7で非推奨、PHP8で削除
mysql_connect($host, $user, $pass);
mysql_query($sql);
対処法:
PDOで共通クラスを作成し、削除されたmysql_queryと同名の関数を定義。
既存コードはそのまま動く。これが一番賢かった。
バージョン管理もない
その痕跡がこちら。
バックアップファイルの存在
index.php
index.php.bak
index.php.bak2
index.php.old
index.php.20190401
index_new.php
index_test.php
どれが本番で使われてるのか、ファイル名だけじゃわからない。
コメントによる履歴管理
// 2019/04/01 田中 修正
// 2019/05/15 鈴木 上記を戻した
// 2019/06/01 田中 やっぱり修正
// 2020/01/10 佐藤 暫定対応
// ↑いつか消す
消されることなく5年以上経過。
「暫定」という名の永続
// TODO: 暫定対応。後で直す
// ↓このコード、7年前から「暫定」
暫定は永遠に暫定のまま。
移行作業でやったこと
1. 動作確認環境の構築
まずPHP5.6とPHP8.2の両方で動く環境を用意。
Dockerで切り替えられるようにした。
2. エラーログの収集
PHP8で動かして、出てくるエラーを片っ端から記録。
エラーの種類と発生箇所をリスト化。
3. 最小限の修正
リファクタリングはしない。
- 削除された
mysql_*と同名の関数をPDOベースで定義 -
count(null)→is_countable()でガード -
NULL算術→?? 0でデフォルト値設定
元のロジックには触らない。
4. 動作確認
修正前と修正後で同じ挙動になることを確認。
「美しくなったか」ではなく「同じように動くか」が基準。
学んだこと
📜 レガシーコードは歴史の産物
「なんでこんな書き方？」と思うコードにも理由がある。
当時のPHPバージョン、当時のベストプラクティス、当時の制約。
それを理解せずに「ダメなコード」と断じるのは傲慢。
🛡️ 動いているものを壊すな
「きれいにしたい」は開発者のエゴ。
クライアントは「動くこと」を求めている。
美しさより安定性。
💸 リファクタリングは別予算で
移行とリファクタリングは別の作業。
一緒にやろうとすると、どっちも中途半端になる。
リファクタリングしたいなら、別途提案して予算を取る。
🤝 次の人への配慮
中途半端に手を入れると、次の人が混乱する。
「全部レガシー」か「全部リファクタリング済み」のどちらかが理想。
まとめ
リファクタリングしたい気持ちはわかる。
でも、時にはその衝動を抑える勇気も必要。
- 動いているコードは尊重する
- リファクタリングと移行は分ける
- 最小限の修正で目的を達成する
- 次の人のことを考える
レガシーコードと向き合うのは大変だけど、それも立派なエンジニアリング。
美しいコードを書くことだけがエンジニアの仕事じゃない。
動くものを、動くままに、次の世代へ繋ぐ。
それもまた、大切な仕事だと思います。
・・・でも、最初からちゃんと綺麗なコード書け！！！