Skip to content
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.

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

Fix Validation Exception #81

Closed
maghamed opened this issue Sep 5, 2017 · 1 comment
Closed

Fix Validation Exception #81

maghamed opened this issue Sep 5, 2017 · 1 comment
Assignees

Comments

@maghamed
Copy link
Contributor

maghamed commented Sep 5, 2017

Working on Reservation Validation we found out next:

I've added validation for ReservationBuilder but I think there is a general issue.
The __construct() signature of \Magento\Framework\Exception\AbstractAggregateException class and the one of its descendant \Magento\Framework\Validation\ValidationException are different.
This causes an issue in the addError() method when more than a validation error occurs because $this->originalPhrase is not properly initialized in ValidationException and we have render() called on a null object.

you are totally correct. The ValidationException

namespace Magento\Framework\Validation;

use Magento\Framework\Exception\AbstractAggregateException;

/**
 * Add possibility to set several messages to exception
 *
 * @api
 */
class ValidationException extends AbstractAggregateException
{
    /**
     * @param ValidationResult $validationResult
     * @param \Exception $cause
     * @param int $code
     */
    public function __construct(ValidationResult $validationResult, \Exception $cause = null, $code = 0)
    {
        foreach ($validationResult->getErrors() as $error) {
            $this->addError($error);
        }
        parent::__construct($this->phrase, $cause, $code);
    }
}

violates 4 main rules which should be followed by Magento constructors:

  1. It's forbidden to have business logic in the Magento Constructors, just assignments allowed
  2. As a consequence of 1. the signature of the child doesn't correspond to parent's signature (which is not recommended).
  3. As a consequence of 1. parent constructor not called as a first line in child constructor. (Following the rule of calling parent's constructor in the very beginning of child constructor is a save way to be sure that you have a correctly instantiated base implementation. For example, in Java super constructor MUST be called at the first line of the child).
  4. As a consequence of all of the above, call $this->addError($error); which uses parent's contract relies on $this->phrase which is not initialized at this moment.

So, the problem appeared as an outcome of item 4.
But before that 3 items were violated, which made it possible to make a logical error in item 4,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants