CakePHP に issue を立てて、修正を pull request してみた

概要

CakePHP 公式の github に pull request してみたら、たくさん良い指摘をしてもらえたので、その内容を（翻訳もして）共有してみます。
どんな英語が使われてるのかも参考になると思うので、英文と訳文の両方を掲載しますね。

簡単な経緯

ある日、CakePHP3 のアドベントカレンダー で tomzohさん が CakePHP3でコントローラのテストを書く という記事を掲載し、ここでハマりポイントと回避方法を丁寧に紹介しておられました。
その内容が誰しもハマりそうな内容だったので、 tomzohさん に許可を頂いて、issue を立ててみたという次第です。

立てた issue

IntegrationTestCase::cookie() doesn't encrypt #7862

長いので要約します

（詳細見たい人は #7862 をご覧ください。）

CakePHP3 には IntegrationTestCase という CakePHP2 の ControllerTestCase にあたるクラスがあります。
IntegrationTestCase には cookie() というメソッドがあり、簡単にクッキーを仕込めるのですが、
CookieComponent はデフォルトで値を暗号化するのに IntegrationTestCase::cookie() は暗号化しないので、多くのユーザはテストケースで下記のいずれかが必要になってしまいます:

• EventManager でコールバックを仕込み、CookieComponent の暗号化を強制的に OFF にする
• CookieComponent と同じ仕様で暗号化してから cookie() に渡す

で、コメントが追加される

markstory:

暗号化した cookie をセットできる方法があったとしたらどう？ もう少し実際の利用に近いテストケースって出せる？

What if there was a way to set encrypted cookies? Wouldn't that create a more realistic test scenario?

実際のテストケース作る

※この場合に「実際のテストケース」として要求しているものが、テスト駆動的に理想の仕様を示すためのテストケースなのか、それとも、この問題を理解するためのテストケースなのか、この時点では分かりませんでした。なので、とりあえず、後者で書いてみました。今後、理解に曖昧なところがあると困ると思って。仮に前者だったとしても、１行変えるだけだし、違ったら言ってくれると思うし。

で、具体的な再現シナリオ を記載する。
手動で暗号化して通過するケースと、暗号化せずに失敗するケースを記載する。

すると、コメント。

markstory:

OK。通過する方の例では、cookie の値を暗号化するようにセットできるメソッドがあったほうが良さそう。手動で暗号化の処理をしなければならないってのは最善とは思えない。そういうメソッドがあれば、CookieComponent 経由で読ませたい cookie についてはそれを使ってセットできるから良いよね。

Ok, in the passing example, it seems like we should probably have a method
to set encrypted cookie values. Having to do that encryption process by hand seems sub-optimal. If such a method existed, you could use it to set cookies that you want to read via CookieComponent.

ここで pull request を作りました。

[3.2] Can encrypt with IntegrationTestCase::cookie() #7873

僕:

IntegrationTestCase::cookie() に CookieComponent で暗号化するための第３引数を追加。#7862 を参照してください。

Add the 3rd argument to IntegrationTestCase::cookie() to encrypt with the CookieComponent. See: #7862

antograssiot:

• ヘッダーコメントのバージョン番号が 3.0.0 になってるが 3.2.0 だという指摘
• travis-ci でコーディングルールの違反が指摘されてるのでその指摘
  • use のクラス名はアルファベット順であるべき
  • クラス/関数開始の { の前には改行が必要
  • 関数の Doc は必須
  • ファイルの末尾には改行が必要

僕:

• Fix the version number in the comment.
• Add comments.
• Apply the coding standard.
• Apply the coding standard.
• Apply the coding standard. (Fix that "Expected docblock to be aligned with code.")

markstory:

• インスタンスを入れる変数名は小文字で始めるという指摘

ここで、CookieComponent が持つ暗号化/復号化ロジックをどこに置くべきかとう論議に。

僕の当初案では、CookieComponent の protected な暗号化メソッドを継承して public に引き上げたクラス CookieEncrypter を作っていました。
それに対して、切り出すべきではとの指摘。

markstory:

サブクラスじゃなくて、たぶん暗号化/復号化ロジックを Component からもっと再利用しやすいコード内へと切り出したほうがいいな。

Instead of subclassing perhaps we should extract the encryption/decryption logic out of the Component into a more re-usable piece of code?

僕:

Cake\Utility\CookieCrypt に切りだすのはどうでしょう？ ただ、 protected なメソッド/プロパティについての互換性が失われるかもしれません。ユーザの中には CookieComponent を継承している人もいるでしょう。どう思います？

Trait として切りだすのどうでしょう？ Trait なら互換性が保てる気がします。

How about extracting the logic to the class named as Cake\Utility\CookieCrypt? However it might break the compatibility relevant to protected methods/properties. Some users might extend CookieComponent. How do you think?

How about extracting as Trait? I think the compatibility may be able to be kept if Trait.

markstory:

Trait はサブクラスより良さそうだね。

A trait would be nicer than subclassing.

こうして、Trait として切り出すことに。

引数で false を渡すと暗号化が off されるという仕様は良くないという指摘。

僕の当初案では、

• $this->cookie($key, $value, 'aes') なら暗号化される。 $value が配列の場合、CookieComponent と同様に json_encode もされる。
• $this->cookie($key, $value, false) なら暗号化されないが、$value が配列なら json_encode だけはされる。
• $this->cookie($key, $value) なら今までどおり、暗号化も json_encode もされない。

という仕様にしてました。
これが解りづらいという指摘です。

markstory:

どうして false なのに暗号化される？

Why does false turn on encryption?

僕:

CookieComponent::write() には配列を渡すことも可能で、配列は json にエンコードされます。初心者の中には $this->cookie('anyKey', ['key' => 'value']); をしようとする人もいると思うんです。しかし、cookie() メソッドは CookieComponent と違ってデフォルトで json_encode しませんから、これは動きません。cookie() のデフォルトの挙動は変えるべきではないので、CookieComponent の encryption と同じように false を導入すれば良いのではと思ったのです。

CookieComponent::write() can accept an array which will encode to json. I think some beginners try to do $this->cookie('anyKey', ['key' => 'value']);. But, it doesn't work because cookie() method does not json_encode by default, unlike CookieComponent. The default works of cookie() should not be changed, so I think it is better way to introduce false as same as the encryption of CookieComponent.

markstory:

違うメソッドで暗号化された cookie をセットできればいいんじゃない？ API は読みやすくなるなる気がするよ。Boolean のフラグは固有のメソッド名に比べて把握しづらいよ。

What if encrypted cookies were set through a different method? I think that might result in an easier to read API. Boolean flags are harder to grok than distinct method names.

こうして、メソッド分けることに。

コントローラを使ってテストすべきという指摘

僕の単体テストは関連をすべてモックしたもので本当に単体でのテストになっていたため、Controller を実際に使うべきというものでした。Controller を使うとなると、テストで作成するクラスも増え、他のテストにも影響しそうで躊躇していたのですが、まあ、指摘されてみると、そうですよね、という感じです。
今回の問題は単体でテストしていては発見できない類のものなので。

markstory:

コントローラのアクションで cookie が同様にデコードできるか確認するのが良いんじゃないかな。

It would be good to make sure a controller action can decode the cookie as well.

引数で CookieComponent の config 渡せるようにしては？ という提案が来る

jeremyharris:

第３引数で component が受け取るような config 配列を渡せるようにすれば、より意味深いものになるのでは？ この方法ならユーザは cookie 設定のすべてを制御できるようになる。(salt がオプションで書ければ便利になる気がする。)

Would it make more sense to have the third option be a config array, similar to what the component accepts? That way users have full control over the cookie settings. (salt being an option I could see as useful.)

僕:

@jeremyharris CookieComponent はデフォルトで値を暗号化するので、多くのユーザは CookieComponent の config を意識することもなく使ってると思います。ですので、['encryption' => ***] は無しでこれが使える方が良いと思うのです。ただ、暗号化キーを渡さなければならないというケースもありえます。ですので、第４引数でこれができるようにしました。どうでしょう？

@jeremyharris The CookieComponent encrypt values by default, so I think most users use it without the conscious of the configuration of CookieComponent. Therefore, I think it is nicer to use it without ['encryption' => ***]. However, the case that you need to pass the encryption key can be possible. So, I have added the forth argument to do this. How about this?

jeremyharris:

@waterada テスト中に他のオプションが必要な理由も思い当たらないので、良さそうに思えるね。シグネチャがより将来性のあるものになるとか、ビルダークラスが受け入れられるとかも考えたけど、皆さんが引数増やすのでOKなら私は :)

jeremyharris: @waterada I think that will work since I can't see why we'd need the other options during tests. I was aiming for the signature to be a bit more future proof or even accept a builder class, but if everyone's okay with more args, I am :)

次バージョンでなく現バージョンに入れようという提案。

lorenzo:

@waterada この pull request を代わりに 3.0 ブランチに submit できる？

@waterada Would you like to submit this pull request to the 3.0 branch instead?

dereuromark:

@lorenzo 3.1 のこと？

@lorenzo You mean 3.1?

僕:

@lorenzo これを 3.0 に submit できますよ。3.0 は正しいですか？ それとも 3.1 ？

@lorenzo I can submit to this to the 3.0. 3.0 is correct? Or, 3.1?

lorenzo:

申し訳ない、言いたかったのは master ブランチだよ。にっこり

sorry, I meant the master branch :)

こうして、pull request を master から出し直すことに。
新しい pull request を作り、今までの指摘も整理する。

master をベースにした pull request に切り替える。

新しい pull request を作って、元の pull request には下記を書いておく。

僕:

master へと pull request し直しました。#7880 を参照してください。

I have re-pull-requested to the master. Please see: #7880

新しく作ったプルリクに書き込む。

僕:

[3.1] IntegrationTestCase::cookieEncrypted() 追加。 #7880

CookieComponent を使って暗号化できるように IntegrationTestCase::cookieEncrypted() 追加。
参照: issue #7862
これも参照: 前の pull request #7873 (まもなく close されます)

• @lorenzo のアドバイス に基づき、master をベースにして再 pull request しています。
• @markstory のアドバイス に基づき、CookieComponent から trait として暗号化ロジックを切り出しています。
• @markstory のアドバイス に基づき、新たなメソッド cookieEncrypted() を cookie() とは別に追加しています。
• @markstory のアドバイス に基づき、コントローラのアクションが cookie をデコードできるのか確認するテストを追加しています。
• @jeremyharris のアドバイス に基づき、cookieEncrypted() の第４引数で暗号化キーを指定できるようにしています。

[3.1] Add IntegrationTestCase::cookieEncrypted(). #7880

Add IntegrationTestCase::cookieEncrypted() to encrypt with the CookieComponent.
See: issue #7862
See also: the previous pull request #7873 (will be closed soon)

• This is the re-pull-request which is based on master on @lorenzo's advice.
• Extracting the crypt logic from CookieComponent to the trait on @markstory's advice.
• Add the new method cookieEncrypted() separately from cookie() on @markstory's advice.
• Add the test to make sure a controller action can decode the cookie on @markstory's advice.
• Add the forth argument to specify the encryption key to cookieEncrypted() on @jeremyharris' advice.

さらにいくつか指摘されましたが

ながくなったので、一旦ここで切ることにします。
反響があったら続き書きますね。