BASEプロダクトチームブログ

ネットショップ作成サービス「BASE ( https://thebase.in )」、ショッピングアプリ「BASE ( https://thebase.in/sp )」のプロダクトチームによるブログです。

BASE で使っているPHPフレームワークにプルリクエストを送ろうとしたら、先を越された話

PHP CakePHP

f:id:tenkoma:20181205113831j:plain

この記事は、「BASE Advent Calendar 2018」の5日目の記事です。

devblog.thebase.in

Backend Engineer の田中(@tenkoma)です。

「BASE」の裏側で動いているアプリケーションはCakePHP 2を使っています。

そのCakePHP 2にPHP7.3対応のプルリクエストを送ろうとしたけど先を越されてしまった話をします。

CakePHPのPHP7.3対応状況

PHPはPHP7.0以降、大きな機能追加のあるバージョンが年1回リリース*1されるようになりました。

リリースサイクルは公開されており(PHP: todo:php73)、毎年9月中までにはRC版が公開されているようです。活発なPHP製OSSプロジェクトなら、ビルドスクリプトにPHPの新しいバージョンを追加して、互換性が確認できているか確かめられるようになっているかもしれません。

CakePHP 3はテストスイートがすべてパスしたブランチを2018年10月2日に master マージしていたようです。

ではCakePHP 2の状況は？

PHP 7.2でだいたい動作することは自分で確かめていたので、PHP 7.3 RC3が出た頃に状況を確認しました。

すると、PHP 7.3互換性対応のプルリクエストがマージされたことがあるが、全テストケースがパスしているかはわからない状況みたいでした。

PHP 7.3の勉強にもなるし、やってみるか〜ということで作業することにしました。

ほぼ同じ修正をコミットしてたのに…悔

まずは、テストケースが失敗するとしたら、どれが失敗するか把握する必要があるので、PHP 7.3でテストを実行するためのプルリクエストをつくり、マージしてもらいました。

マージ後のビルドログを見ると、270 Errorsと表示されましたが、ほぼ同種のエラーがいろいろなテストに発生しているだけで、修正はたいしたことありません。

自分のアカウントにフォークしたリポジトリもTravisの設定を追加していたので、一つ一つ修正してはプッシュしてエラーを確認していったところ、残り1箇所まで減らすことができました。

残る一つは mb_strtoupper() のPolyfillのテストでした。

CakePHP 2には、mbstring拡張が利用できない場合でも mb_strtoupper(), mb_strtolower() が使えるよう、関数が用意されています。(実際のコードは Multibyteクラスで実装されています。)

mb_strtoupper() のテストも用意されていますが、Travis CIでmbstring拡張が有効になっており、組み込み関数が呼び出されて、そこでテストが失敗していました。

There was 1 failure:
1) MultibyteTest::testUsingMbStrtoupper
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'ԱԲԳԴԵԶԷԸԹԺԻԼԽԾԿՀՁՂՃՄՅՆՇՈՉՊՋՌՍՎՏՐՑՒՓՔՕՖև'
+'ԱԲԳԴԵԶԷԸԹԺԻԼԽԾԿՀՁՂՃՄՅՆՇՈՉՊՋՌՍՎՏՐՑՒՓՔՕՖԵՒ'

PHP 7.3 RC版の不具合かとも思いましたが、これは文字ケース変換の機能拡張のようです。

PHP 7.3で仕様が変わることはわかりましたが、どう直すか悩みました。いまさら Multibyte クラスの仕様をPHPにあわせて拡張するのもどうかと思いましたし(実装難しそう…)互換性を諦めて、テストを削除してしまえばいいか、とも考えました。

それから何もせず1日経ったところで次のプルリクエストを見つけました。

コミットの内容はほぼ一緒で、mb_strtoupper()のテスト失敗はそのままでしたが、見事に先を越されてしまいました。

先にプルリクエストを出して、スレッドで相談しながら決めれば良かったんですよね。 行動指針「Move Fast」にしたがって、早く出せば良かったな、と思いました。

補足: PHP 7.3対応で修正されたCakePHP 2の互換性

実装コードの修正を2点紹介します。

compact() で未定義の変数を配列にしようとするとNotice Errorになる

7.2 まで、 compact() では未定義変数名を含めることができました。(戻り値にキーは無い)

$defined = 'defined';
$arr = compact('defined', 'undefined');
var_dump($arr);
// output: 
// array(1) {
//   'defined' =>
//   string(7) "defined"
// }

7.3からは以下のようなNotice Errorになります。

PHP Notice:  compact(): Undefined variable: path in /home/travis/build/tenkoma/cakephp/lib/Cake/Utility/Debugger.php on line 255

switch文の break; の代わりに continue; を使うとWarning Errorになる

7.2 まで switch文ではbreak文の代わりにcontinue文を使うことができましたが、switch文の外側のループを対象にした命令のつもりでも、switchへのbreak文になる問題がありました。

foreach($items as $item) {
    switch($item['type']) {
        case 'A':
            continue; // foreachループの先頭に戻らず、switch文を抜けるだけ
    }
}

PHP 7.3ではWarning Errorとなります。PHP 8では構文エラーになるそうです。

参考: PHP 7.3 曖昧なcontinue文の禁止 – yohgaki's blog

まとめ

プルリクエストはある程度できたら出しちゃったほうがいいこともあるよ！

明日はid:pigooosukeです。お楽しみに！

「BASE Advent Calendar」の記事一覧はこちら。

devblog.thebase.in

*1:毎年12月初旬頃