Allow integration testing of secure post requests with a query string. #9103
Conversation
The current integration test secure token generation does not take query strings into account, while the security component does. This address that inconsistency.
Current coverage is 94.94%@@ master #9103 diff @@
==========================================
Files 368 368
Lines 26988 26991 +3
Methods 3232 3232
Messages 0 0
Branches 0 0
==========================================
+ Hits 25623 25626 +3
Misses 1365 1365
Partials 0 0
|
| @@ -434,10 +434,15 @@ protected function _buildRequest($url, $method, $data) | |||
| $session = Session::create($sessionConfig); | |||
| $session->write($this->_session); | |||
| list ($url, $query) = $this->_url($url); | |||
| $tokenUrl = $url; | |||
|
|
|||
| if (!empty($query)) { | |||
There was a problem hiding this comment.
As per our guidelines to not cloak variable checks.
Prevents bugs in the future and reads exactly the same.
=> http://book.cakephp.org/3.0/en/contributing/cakephp-coding-conventions.html#careful-when-using-empty-isset
There was a problem hiding this comment.
If that's what the guidelines say 👍
There was a problem hiding this comment.
That's a weird guideline... why trigger a type conversion for no reason?
There was a problem hiding this comment.
Check the discussion a while back where not doing this actually led to existing bugs in the core framework.
E.g. refactoring to
if (!empty($guery)) { // now dead code }
by accident - note the almost invisible guery vs query.
The alternative would be to use if (count(...) > 0).
The current integration test secure token generation does not take query strings into account, while the security component does. This address that inconsistency.