Improve error messaging when merged XML is invalid #36239

fredden · 2022-10-04T15:13:41Z

Description

When dealing with XML in Magento, the error messages are not always most helpful. When each XML document is individually valid, but the result of merging these isn't, the error message does not help a developer identify the issue.

This pull request adds the filename causing a problem to the error message output.

Before and after pictures

The following two pictures show the output from bin/magento setup:upgrade where the problem shows up.

Before:

After:

Manual testing scenarios

Note that if you fix/merge #36240 before this, then you will need to find alternative steps to reproduce.

Ensure Magento is in "developer" mode with bin/magento deploy:mode:set developer
Create a new local module in app/code/

Create a valid etc/db_schema.xml file changing the 'type' of an existing column from 'int' (with 'padding') to 'varchar'. For example,

magento2/app/code/Magento/AsynchronousOperations/etc/db_schema.xml

Lines 20 to 21 in 447c613

    
           <column xsi:type="int" name="operation_count" padding="10" unsigned="true" nullable="false" identity="false" 
        
                   comment="Total number of operations scheduled within this bulk"/>

and

<?xml version="1.0"?>
<schema xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="urn:magento:framework:Setup/Declaration/Schema/etc/schema.xsd">
    <table name="magento_bulk">
        <column xsi:type="varchar" name="operation_count" />
    </table>
</schema>

Run bin/magento setup:upgrade
Witness error message

Before this change, it is very difficult to identify the location of the error.

Questions or comments

All unit tests pass both before and after this change, suggesting that the code coverage isn't ideal. Perhaps we need a separate task to improve code coverage of this class.

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)
README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
All automated tests passed successfully (all builds are green)

m2-assistant · 2022-10-04T15:13:44Z

Hi @fredden. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here

ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

magento-automated-testing · 2022-10-04T15:15:33Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

magento-automated-testing · 2022-10-04T16:05:00Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

magento-automated-testing · 2022-10-04T17:00:07Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

ihor-sviziev · 2022-10-17T11:16:21Z

@fredden Great job! Do you feel it's possible to cover with any type of automated tests?

magento-automated-testing · 2022-10-17T11:32:10Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

magento-automated-testing · 2022-10-17T11:32:13Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

magento-automated-testing · 2022-10-17T11:32:15Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

fredden · 2022-10-17T14:47:01Z

Do you feel it's possible to cover with any type of automated tests?

I don't know how to reliably cover this with a test. We could write a test using the given example, but the underlying problem is being fixed in #36240, so we'd need another test case that will reliably break. I'm open to suggestions.

ihor-sviziev · 2022-10-18T12:44:49Z

Yeah, looks like it will be too hard to cover it with test, let's skip it

magento-automated-testing · 2022-10-20T08:19:53Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

engcom-Lima · 2022-10-26T10:25:19Z

@magento run Functional Tests EE

fredden · 2022-11-22T07:10:25Z

@engcom-Delta thanks for testing. I can see that you have successfully verified the change. What changes would you like to be made to this pull request? Perhaps you meant to confirm/approve this and picked the wrong option?

engcom-Delta · 2022-11-22T09:15:51Z

@fredden My apologies for that. I have updated and will move it to the appropriate column.

engcom-Delta · 2022-11-22T09:22:50Z

@magento run Functional Tests B2B

magento-automated-testing · 2022-11-22T09:22:59Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

engcom-Delta · 2022-11-22T11:39:02Z

@magento run Functional Tests EE

magento-automated-testing · 2022-11-22T11:39:06Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

magento-automated-testing · 2022-11-22T12:06:43Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

engcom-Delta · 2022-11-22T14:29:36Z

@magento run Functional Tests CE

magento-automated-testing · 2022-11-22T14:29:46Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

engcom-Echo · 2022-11-23T06:16:31Z

@magento run Functional Tests CE, Functional Tests B2B

magento-automated-testing · 2022-11-23T06:16:39Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

magento-automated-testing · 2022-11-23T07:55:47Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

engcom-Echo · 2022-11-23T08:33:38Z

@magento run Functional Tests B2B

magento-automated-testing · 2022-11-23T08:33:44Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

magento-automated-testing · 2022-11-23T10:39:35Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

engcom-Echo · 2022-11-23T12:37:31Z

@magento run Functional Tests B2B

magento-automated-testing · 2022-11-23T12:37:40Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

Improve error messaging when merged XML is invalid

2f9de7b

m2-github-services added Partner: Fisheye partners-contribution Pull Request is created by Magento Partner labels Oct 4, 2022

fredden added 2 commits October 4, 2022 16:55

Correct grammar in comment

a8d440e

Correct copyright block syntax

6b1eb68

fredden mentioned this pull request Oct 4, 2022

Allow changing column type in etc/db_schema.xml #36240

Open

5 tasks

Update comments to appease linter

3dfe2c4

sdzhepa added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Oct 6, 2022

m2-community-project bot added the Progress: pending review label Oct 6, 2022

ishakhsuvarov approved these changes Oct 13, 2022

View reviewed changes

m2-community-project bot assigned ishakhsuvarov Oct 13, 2022

m2-community-project bot added Progress: ready for testing and removed Progress: pending review labels Oct 13, 2022

ihor-sviziev added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Oct 18, 2022

ihor-sviziev approved these changes Oct 18, 2022

View reviewed changes

m2-community-project bot assigned ihor-sviziev Oct 18, 2022

m2-community-project bot added Progress: testing in progress and removed Progress: ready for testing labels Oct 26, 2022

m2-community-project bot added Progress: ready for testing Progress: needs update and removed Progress: ready for testing labels Nov 22, 2022

m2-community-project bot added Progress: testing in progress and removed Progress: needs update labels Nov 22, 2022

Merge branch '2.4-develop' into xml-validation/better-messaging

0aa7ebd

m2-community-project bot added Progress: extended testing and removed Progress: testing in progress labels Nov 22, 2022

m2-community-project bot added Progress: accept and removed Progress: extended testing labels Nov 24, 2022

magento-devops-reposync-svc merged commit fd798cc into magento:2.4-develop Dec 22, 2022

fredden deleted the xml-validation/better-messaging branch December 23, 2022 08:36

hostep mentioned this pull request Feb 9, 2023

feat: more informative error message on xml validation errors #36836

Merged

6 tasks

	<column xsi:type="int" name="operation_count" padding="10" unsigned="true" nullable="false" identity="false"
	comment="Total number of operations scheduled within this bulk"/>

Improve error messaging when merged XML is invalid #36239

Improve error messaging when merged XML is invalid #36239

Uh oh!

Conversation

fredden commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Manual testing scenarios

Questions or comments

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Oct 4, 2022

Uh oh!

magento-automated-testing bot commented Oct 4, 2022

Uh oh!

magento-automated-testing bot commented Oct 4, 2022

Uh oh!

magento-automated-testing bot commented Oct 4, 2022

Uh oh!

ihor-sviziev commented Oct 17, 2022

Uh oh!

magento-automated-testing bot commented Oct 17, 2022

Uh oh!

magento-automated-testing bot commented Oct 17, 2022

Uh oh!

magento-automated-testing bot commented Oct 17, 2022

Uh oh!

fredden commented Oct 17, 2022

Uh oh!

ihor-sviziev commented Oct 18, 2022

Uh oh!

magento-automated-testing bot commented Oct 20, 2022

Uh oh!

engcom-Lima commented Oct 26, 2022

Uh oh!

fredden commented Nov 22, 2022

Uh oh!

engcom-Delta commented Nov 22, 2022

Uh oh!

engcom-Delta commented Nov 22, 2022

Uh oh!

magento-automated-testing bot commented Nov 22, 2022

Uh oh!

engcom-Delta commented Nov 22, 2022

Uh oh!

magento-automated-testing bot commented Nov 22, 2022

Uh oh!

magento-automated-testing bot commented Nov 22, 2022

Uh oh!

engcom-Delta commented Nov 22, 2022

Uh oh!

magento-automated-testing bot commented Nov 22, 2022

Uh oh!

engcom-Echo commented Nov 23, 2022

Uh oh!

magento-automated-testing bot commented Nov 23, 2022

Uh oh!

magento-automated-testing bot commented Nov 23, 2022

Uh oh!

engcom-Echo commented Nov 23, 2022

Uh oh!

magento-automated-testing bot commented Nov 23, 2022

Uh oh!

magento-automated-testing bot commented Nov 23, 2022

Uh oh!

engcom-Echo commented Nov 23, 2022

Uh oh!

magento-automated-testing bot commented Nov 23, 2022

Uh oh!

Uh oh!

fredden commented Oct 4, 2022 •

edited

Loading