Handle type errors for objects creation #15895
Conversation
|
|
magento-engcom-team
added
Partner: Atwix
ishakhsuvarov
left a comment
ishakhsuvarov
left a comment
There was a problem hiding this comment.
It's best to cover this kind of changes with a test
| try { | ||
| return new $type(...array_values($args)); | ||
| } catch (\Throwable $e) { | ||
| throw new \Exception($e->getMessage(), null, $e); |
There was a problem hiding this comment.
I would suggest throwing \Magento\Framework\Exception\RuntimeException here instead, as \Exception is too generic.
| try { | ||
| return new $type(...array_values($args)); | ||
| } catch (\Throwable $e) { | ||
| throw new \Exception($e->getMessage(), null, $e); |
There was a problem hiding this comment.
It would be nice to add a log entry right here
| try { | ||
| return new $type(...array_values($args)); | ||
| } catch (\Throwable $e) { | ||
| $this->getLogger()->critical(__( |
There was a problem hiding this comment.
Since framework must remain independent of other parts of the system __() call can not be used here. It's advised to use Magento\Framework\Phrase instead.
| $e->getMessage() | ||
| ] | ||
| )); | ||
| throw new RuntimeException(__($e->getMessage())); |
There was a problem hiding this comment.
Original exception message is already logged at this point. It makes sense to throw more generic message here.
|
@yuriyDne Thanks for the update. This issue is apparently more complicated then it looks. |
|
Sorry for delay - you are right - it's much more complicated than I expected when we try to use logger instance |
|
@yuriyDne Sure, we are happy to help. We'll take a closer look and let you know |
| try { | ||
| return new $type(...array_values($args)); | ||
| } catch (\Throwable $e) { | ||
| $this->getLogger()->critical(__( |
There was a problem hiding this comment.
It may be better, to replace getLogger method with a static object manager call, getting LoggerInterface right here, for example:
use Magento\Framework\App\ObjectManager;
use Psr\Log\LoggerInterface;
$logger = ObjectManager::getInstance()->get(LoggerInterface::class);This way it's possible not to rely on $this->objectManager which may or may not be set, making the code slightly more future-proof.
|
Hi @yuriyDne I've updated the code review with possible solution. Please let me know if you have any questions. |
|
@ishakhsuvarov - Thanks for help - I updated code according to your recommendations |
|
@yuriyDne I would suggest covering this with an integration test, which would be much more effective in this case. |
|
@ishakhsuvarov integration test is implemented. Please check. Thanks |
| return new $type(...array_values($args)); | ||
| try { | ||
| return new $type(...array_values($args)); | ||
| } catch (\Throwable $e) { |
There was a problem hiding this comment.
I have executed full suite of integration tests on the internal CICD and found out that, catching \Throwable here yields undesired results.
This primarily happens, when some classes throw different exceptions right from the constructor. While this violates Magento Technical Guidelines, this case still has to be supported, for example:
\Magento\Framework\Filesystem\File\Readwould throwFileSystemExceptionfrom the constructor when file does not exist.AbstractFactorywould then catch it, as\Throwablecatches all possible exception types, and rethrow it differently.- FileSystem logic broken as a result
To bypass this case I would suggest catching only \TypeError here, but not \Throwable.
I am attaching list of integration tests, which fail with Throwable and pass with TypeError:
| ); | ||
|
|
||
| throw new RuntimeException( | ||
| new Phrase('Create object error') |
There was a problem hiding this comment.
I would suggest to make error message a bit more detailed, something in the lines of original TypeError
| // Call parent constructor without parameters to generate error | ||
| parent::__construct(); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Please add an empty line at the end of the file, as it is required by the coding standard.
|
|
||
| class ConstructorWithThrowable extends \Magento\Framework\ObjectManager\TestAsset\ConstructorOneArgument | ||
| { | ||
| public function __construct(Basic $one) |
There was a problem hiding this comment.
Static tests would fail here, as $one is never used.
I'd suggest adding
/**
* @SuppressWarnings(PHPMD.UnusedLocalVariable)`
*/| public function testNewInstanceWithThrowableError() | ||
| { | ||
| try { | ||
| $testObject = self::$_objectManager->create(self::TEST_CLASS_WITH_THROWABLE); |
There was a problem hiding this comment.
There is no need to use $testObject variable. You may just do an ObjectManager call to get the exception.
| /** | ||
| * Test create instance with throwable error | ||
| */ | ||
| public function testNewInstanceWithThrowableError() |
There was a problem hiding this comment.
It's best to avoid doing try/catch in the test.
You may use
/**
* @expectedException \TypeError
*/It would assert that correct exception is thrown automatically.
|
Hi @yuriyDne Looks like this PR is nearly complete now! :) |
magento-engcom-team
added
Progress: needs update
and removed
Progress: needs update
labels
|
@ishakhsuvarov - made fixes after your code review. Please check changes. Thanks |
|
@yuriyDne Thank you for the update |
…o handle-type-error-for-object-manager
- Updated test
- Added declaration of strict types
|
Hi @yuriyDne. Thank you for your contribution. |
4 participants
Handle type errors when object is creating by ObjectManager
Description
When object creates with type error on production mode with display_errors off it's very difficult to catch this error because Magento doesn't handle it in log files (only apache log which is not always public accessed). It can be actual for background processes when there is no possibility for manual testing. So my suggestion is catch object creating TypeError (as example) and throw it as simple Exception.
Fixed Issues (if relevant)
Manual testing scenarios
Contribution checklist