CMS Page - CMS Page - Force validate layout update xml in production mode when saving CMS Page - Handle layout update xml validation exceptions

[adrian-martinez-interactiv4]

When saving custom layout update xml in CMS page, in production mode, xml is not validated against schema file, due to \Magento\Framework\App\Arguments\ValidationState::isValidationRequiredmethod:

    /**
     * Retrieve current validation state
     *
     * @return boolean
     */
    public function isValidationRequired()
    {
        return $this->_appMode == \Magento\Framework\App\State::MODE_DEVELOPER;
    }

Actual result:

Desired behaviour would be check the layout update xml against the schema, and show error message to the user if it is invalid:

Also, when layout update is validated, validation exceptions are unhandled, showing a report error in production mode, and showing this error in developer mode:

For example, uncomment demo layout handle in home page:

Desired behaviour would be handle the exception properly instead of crash, and show error message to the user:

Description

• According to doc block, \Magento\Cms\Controller\Adminhtml\Page\PostDataProcessor::validate should always return a boolean value. It uses \Magento\Framework\View\Model\Layout\Update\Validator::isValid method, that may throw an exception if xml is not valid, so \Magento\Cms\Controller\Adminhtml\Page\PostDataProcessor::validate must handle possible exceptions thrown by xml validator, in order to always return a boolean value.
• \Magento\Cms\Model\Page\DomValidationState has been added, and it is injected from post data processor to force layout update xml validation, only upon cms page save.

This PR include changes of PR #11865

Manual testing scenarios

1. Enable developer mode

2. Edit or create a CMS page, enter some invalid xml in Layout Update XML textarea

3. Try to save the page, application crashes

4. Enable production mode

5. Edit or create a CMS page, enter some invalid xml in Layout Update XML textarea

6. Try to save the page, xml does not get validated

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[dmanners]

Just a few suggestions on the exceptions and validation methods.

[dmanners]

I would consider just a return true here

[dmanners]

I would consider just a return true here

[dmanners]

I would consider a return false; and then we could remove the $hasError variable here.

[dmanners]

I understand that you want to catch all other exceptions here but from a validation point I think there is only the ValidationException and ValidationSchemaException unless I am mistaken. In that case I would prefer the following.

try {
    $hasError = $this->validateData($data, $validatorCustomLayout);
} catch (ValidationException $e || ValidationSchemaException $e) {
    $validatorMessages = $validatorCustomLayout->getMessages();
    $hasError = $hasError || (bool) count($validatorMessages);
    foreach ($validatorMessages as $message) {
        $this->messageManager->addErrorMessage($message);
    }
} catch (\Exception $e) {
    $this->messageManager->addExceptionMessage($e);
}

What do you think?

[adrian-martinez-interactiv4]

I hadn't seen that before, I mean, I didn't know I could catch several exceptions in a single catch block, I used finally to avoid logic duplication, but also that code must be executed if no exception is caught, so it can't be just there in the catch block. We have to find an intermediate solution.

[adrian-martinez-interactiv4]

Is it really allowed to do that?

If not, may I use instanceof a generic exception catch?