Skip to content

removal of Throwables through _sendRequest from get(), post(), patch(), put(), delete, head(), & options() #14193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
travisfont wants to merge 8 commits into from
Closed

removal of Throwables through _sendRequest from get(), post(), patch(), put(), delete, head(), & options() #14193

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f604264
removal of Throwables through _sendRequest from get(), post(), patch(…
travisfont Jan 20, 2020
58a128f
Removed the double LogicException in _handleError() annotation
travisfont Apr 24, 2020
1020a90
Stickler CI - fully qualified names
travisfont Apr 24, 2020
062f7a3
Now allows null if no throwable is found
travisfont Apr 24, 2020
66ee846
Stickler-ci: Update to fully qualified name on Exceptions
travisfont Apr 24, 2020
4eaf4f5
Update to _sendRequest() to send only Throwable Exceptions to _handle…
travisfont Apr 24, 2020
6b08f2b
PHP CodeSniffer - Style Update
travisfont Apr 24, 2020
b36b076
src/TestSuite/IntegrationTestTrait.php#L561
travisfont Apr 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 12 additions & 19 deletions src/TestSuite/IntegrationTestTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

use Cake\Controller\Controller;
use Cake\Core\Configure;
use Cake\Database\Exception as DatabaseException;
use Cake\Error\ExceptionRenderer;
use Cake\Event\EventInterface;
use Cake\Form\FormProtector;
Expand Down Expand Up @@ -60,7 +59,6 @@
use Exception;
use Laminas\Diactoros\Uri;
use LogicException;
use PHPUnit\Framework\Error\Error as PhpUnitError;
use Throwable;

/**
Expand Down Expand Up @@ -378,7 +376,6 @@ public function cookieEncrypted(string $name, $value, $encrypt = 'aes', $key = n
*
* @param string|array $url The URL to request.
* @return void
* @throws \PHPUnit\Framework\Error\Error|\Throwable

@dereuromark dereuromark Jan 20, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arent those true if you disable error handler middleware?

@dereuromark dereuromark Mar 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like if any business logic or in the controller throws exceptions, and we disable the error catching then this would still be true, right?

@dereuromark dereuromark Apr 25, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are still not answered here. I still think those should be kept as those can through exceptions if error handler middleware is disabled.

*/
public function get($url): void
{
Expand All @@ -395,7 +392,6 @@ public function get($url): void
* @param string|array $url The URL to request.
* @param string|array $data The data for the request.
* @return void
* @throws \PHPUnit\Framework\Error\Error|\Throwable
*/
public function post($url, $data = []): void
{
Expand All @@ -412,7 +408,6 @@ public function post($url, $data = []): void
* @param string|array $url The URL to request.
* @param string|array $data The data for the request.
* @return void
* @throws \PHPUnit\Framework\Error\Error|\Throwable
*/
public function patch($url, $data = []): void
{
Expand All @@ -429,7 +424,6 @@ public function patch($url, $data = []): void
* @param string|array $url The URL to request.
* @param string|array $data The data for the request.
* @return void
* @throws \PHPUnit\Framework\Error\Error|\Throwable
*/
public function put($url, $data = []): void
{
Expand All @@ -445,7 +439,6 @@ public function put($url, $data = []): void
*
* @param string|array $url The URL to request.
* @return void
* @throws \PHPUnit\Framework\Error\Error|\Throwable
*/
public function delete($url): void
{
Expand All @@ -461,7 +454,6 @@ public function delete($url): void
*
* @param string|array $url The URL to request.
* @return void
* @throws \PHPUnit\Framework\Error\Error|\Throwable
*/
public function head($url): void
{
Expand All @@ -477,7 +469,6 @@ public function head($url): void
*
* @param string|array $url The URL to request.
* @return void
* @throws \PHPUnit\Framework\Error\Error|\Throwable
*/
public function options($url): void
{
Expand All @@ -493,7 +484,6 @@ public function options($url): void
* @param string $method The HTTP method
* @param string|array $data The request data.
* @return void
* @throws \PHPUnit\Framework\Error\Error|\Throwable
*/
protected function _sendRequest($url, $method, $data = []): void
{
Expand All @@ -508,16 +498,19 @@ protected function _sendRequest($url, $method, $data = []): void
$this->_requestSession->write('Flash', $this->_flashMessages);
}
$this->_response = $response;
} catch (PhpUnitError $e) {
throw $e;
} catch (DatabaseException $e) {
throw $e;
} catch (\PHPUnit\Framework\Error\Error $e) {
$this->_exception = $e;
Comment on lines +501 to +502

@garas garas Apr 25, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least PHPUnit exceptions should be rethrown. Normal applications shouldn't use PHPUnit classes.

However it could be widened to \PHPUnit\Framework\Exception class or \PHPUnit\Exception interface.

All PHPUnit asserts should immediately abort test, but not rendered with ExceptionRenderer. Otherwise it makes testing hard.

} catch (\Cake\Database\Exception $e) {
$this->_exception = $e;
} catch (LogicException $e) {
throw $e;
$this->_exception = $e;
} catch (Throwable $e) {
$this->_exception = $e;
// Simulate the global exception handler being invoked.
$this->_handleError($e);
}

// Simulate the global exception handler being invoked.
if ($this->_exception instanceof Throwable) {

@dereuromark dereuromark Apr 25, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isnt this a behavior change?
They all are instance of this class afaik. Before only in the last catch block this was invoked, now in every case, right?

@travisfont travisfont Apr 25, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, previously it was:

try {
   // ...
   $this->_response = $response;
   } catch (PhpUnitError $e) {
      throw $e;
   } catch (DatabaseException $e) {
      throw $e;
   } catch (LogicException $e) {
      throw $e;
   } catch (Throwable $e) {
      $this->_exception = $e;
      // Simulate the global exception handler being invoked.
      $this->_handleError($e);
}

I've updated all throwables to be added to the exception property and then passed to the handleError() to handle the exception. Thoughts?

$this->_handleError($this->_exception);
}
}

Expand Down Expand Up @@ -566,10 +559,10 @@ public function controllerSpy(EventInterface $event, ?Controller $controller = n
* This method will attempt to use the configured exception renderer.
* If that class does not exist, the built-in renderer will be used.
*
* @param \Throwable $exception Exception to handle.
* @param \Throwable|\LogicException|\Cake\Database\Exception|\PHPUnit\Framework\Error\Error $exception Exception to handle.

@dereuromark dereuromark Apr 25, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't \LogicException|\Cake\Database\Exception|\PHPUnit\Framework\Error\Error all extending Throwable?

@travisfont travisfont Apr 25, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Is it more ideal to remove them all and use the inheritable Throwable instead?

@dereuromark dereuromark Apr 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the point in outlining them all here, if they are subclasses of it.
Also, why removing the typehint?

* @return void
*/
protected function _handleError(Throwable $exception): void
protected function _handleError($exception): void
{
$class = Configure::read('Error.exceptionRenderer');
if (empty($class) || !class_exists($class)) {
Expand Down