Validate XML mapping against XSD file #6728

greg0ire · 2017-09-25T22:18:05Z

TODO

fix all the tests I broke
make it optional and disabled by default, and trigger deprecation if disabled?
establish what is the right kind of exception to throw (there is a test that asserts a mapping exception from doctrine/common should be thrown, I don't know why yet)

greg0ire · 2017-09-25T22:21:07Z

lib/Doctrine/ORM/Mapping/MappingException.php

+     */
+    public static function fromLibXmlErrors(array $errors) : self
+    {
+        return new self(array_reduce(array_map(function (\LibXMLError $error) : string {


First time ever playing with array_reduce, this is fun! But I should probably use implode instead, right?

Yeah, that looks like it could perfectly be implemented with implode(PHP_EOL, array_map(...);.

Will change

greg0ire · 2017-09-26T18:55:05Z

Regarding the exception issue, I found that the xml driver extends a class from another package (doctrine/common). One of the parent methods throws an exception from that package if the result from loadMappingFile is missing a $className key.

Would you consider throwing an exception from this package a BC-break or not? Also, if we forget about this PR for a moment, wouldn't it be better to do this? Asking because ExceptionFromThisPackage does not extend ExceptionFromAnotherPackage

<?php
try {
   // method call from another package
} catch (ExceptionFromAnotherPackage $e) {
    throw new ExceptionFromThisPackage('message', 0, $e);
}

Majkl578

I think this would be a nice addition to 3.0 and have it enabled by default.

Note that this introduces a dependency on ext-dom.

Majkl578 · 2017-10-01T18:28:26Z

lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php

@@ -861,6 +862,21 @@ protected function loadMappingFile($file)
        return $result;
    }

+    private function assertValid(string $file): void


How about call it validateMapping?

Could be less misleading since I don't use actual assertions here

Majkl578 · 2017-10-01T18:29:21Z

lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php

+    {
+        $backedUpErrorSetting = libxml_use_internal_errors(true);
+        $document = new \DOMDocument();
+        $document->load($file);


This will cause duplicated IO, better pass already fetched XML string to this function and use DOMDocument::loadXML().

Yeah but it gives worse error messages

See #6728 (comment)

Majkl578 · 2017-10-01T18:30:54Z

lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php

+        if (!$document->schemaValidate(__DIR__ . '/../../../../../doctrine-mapping.xsd')) {
+            $errors = libxml_get_errors();
+            libxml_clear_errors();
+            libxml_use_internal_errors(false);


These two directives could be put into a more DRY finally block, and use $backedUpErrorSetting.

I lost my train of thought it seems

Majkl578 · 2017-10-01T18:31:09Z

lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php

+        $backedUpErrorSetting = libxml_use_internal_errors(true);
+        $document = new \DOMDocument();
+        $document->load($file);
+        if (!$document->schemaValidate(__DIR__ . '/../../../../../doctrine-mapping.xsd')) {


Space around ! is needed.

Majkl578 · 2017-10-01T18:44:29Z

Would you consider throwing an exception from this package a BC-break or not?

In 3.0: No.
In 2.x: No, because this behavior would not be enabled by default (if accepted for 2.x - not sure about it).

Please add some tests to illustrate how the error propagation from libxml looks like.
It'd also be interesting to know how many (if any) tests fail with this enabled.

greg0ire · 2017-10-01T19:10:16Z

Right now it's not optional so... congrats, no failures!

greg0ire · 2017-10-01T21:03:56Z

Right now it's not optional so... congrats, no failures!

Uuuuh what am I saying, there is one failure that was not intended (I think):

1) Doctrine\Tests\ORM\Tools\Export\XmlClassMetadataExporterTest::testExportDirectoryAndFilesAreCreated
Doctrine\ORM\Mapping\MappingException: libxml error: Element '{http://doctrine-project.org/schemas/orm/doctrine-mapping}join-column', attribute 'on-update': The attribute 'on-update' is not allowed.
/home/greg/dev/doctrine2/tests/Doctrine/Tests/ORM/Tools/Export/xml/Doctrine.Tests.ORM.Tools.Export.User.dcm.xml:35
libxml error: Element '{http://doctrine-project.org/schemas/orm/doctrine-mapping}cascade-persist': This element is not expected. Expected is one of ( {http://doctrine-project.org/schemas/orm/doctrine-mapping}cascade-detach, ##other{http://doctrine-project.org/schemas/orm/doctrine-mapping}* ).
/home/greg/dev/doctrine2/tests/Doctrine/Tests/ORM/Tools/Export/xml/Doctrine.Tests.ORM.Tools.Export.User.dcm.xml:59

/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/MappingException.php:829
/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php:873
/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php:840
/home/greg/dev/doctrine2/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/Driver/FileDriver.php:115
/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php:59
/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:151
/home/greg/dev/doctrine2/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:333
/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:78
/home/greg/dev/doctrine2/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:226
/home/greg/dev/doctrine2/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:115
/home/greg/dev/doctrine2/tests/Doctrine/Tests/ORM/Tools/Export/AbstractClassMetadataExporterTest.php:91

BTW, here is the error message when loading a string instead of a file:

1) Doctrine\Tests\ORM\Tools\Export\XmlClassMetadataExporterTest::testExportDirectoryAndFilesAreCreated
Doctrine\ORM\Mapping\MappingException: libxml error: Element '{http://doctrine-project.org/schemas/orm/doctrine-mapping}join-column', attribute 'on-update': The attribute 'on-update' is not allowed.
/home/greg/dev/doctrine2/:35
libxml error: Element '{http://doctrine-project.org/schemas/orm/doctrine-mapping}cascade-persist': This element is not expected. Expected is one of ( {http://doctrine-project.org/schemas/orm/doctrine-mapping}cascade-detach, ##other{http://doctrine-project.org/schemas/orm/doctrine-mapping}* ).
/home/greg/dev/doctrine2/:59

/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/MappingException.php:829
/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php:873
/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php:840
/home/greg/dev/doctrine2/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/Driver/FileDriver.php:115
/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php:59
/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:151
/home/greg/dev/doctrine2/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:333
/home/greg/dev/doctrine2/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php:78
/home/greg/dev/doctrine2/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:226
/home/greg/dev/doctrine2/vendor/doctrine/common/lib/Doctrine/Common/Persistence/Mapping/AbstractClassMetadataFactory.php:115
/home/greg/dev/doctrine2/tests/Doctrine/Tests/ORM/Tools/Export/AbstractClassMetadataExporterTest.php:91

greg0ire · 2017-10-01T21:08:38Z

Note that this introduces a dependency on ext-dom.

Made that explicit

SenseException · 2017-10-02T07:58:05Z

composer.json

@@ -15,6 +15,7 @@
    "minimum-stability": "dev",
    "require": {
        "php": "^7.1",
+        "ext-dom": "*",


I have a question: Isn't this extension optional? The usage of XML in Doctrine is optional too for a user, isn't it?

Technically, it's an optional dependency. However, making it an explicit dependency is the defensive way here: we prevent the code from blowing up in the users face with a fatal error if they use XML mappings.

Remember that other bundles (e.g. the FOSUserBundle) specify XML mappings which are automatically imported, so the user won't know that he's using XML mappings and thus should add the ext-dom dependency.

One more thought: moving this to suggest would require all bundles that specify XML mappings to add a require entry for ext-dom, which would be a BC break towards the bundles. IMO it's safer to add it as an explicit dependency.

Talking about "Bundles" sounds more like Symfony instead of Doctrine. This repository is independent from frameworks. Should in this case the require be added in DoctrineBundle? What are "all bundles"?

@SenseException replace "bundles" with "anything that provides XML mappings". Suggesting ext-dom instead of requiring it will break third party software if they already use XML mappings. That's a BC break that would move this feature back to 3.0. The way it is now it's possible to add this to the 2.x line.

@alcaeus I rebased on 3.x yesterday 😅

Oh, didn't see that, my bad. IMO it's better to have the explicit dependency anyways. 👍

I have mixed feelings about this. One one hand, it's true that user would need explicit ext-dom dependency. On the other one, we should check at runtime for DOM presence, it just feels wrong to force people with annotations install DOM. (Well, we can say the same about doctrine/annotations, but that's slightly different story).
Since this PR is now 3.0 only, it should be fine fine to do either way, but question is which one is better.

On the other one, we should check at runtime for DOM presence, it just feels wrong to force people with annotations install DOM.

https://webmozart.io/blog/2014/10/24/defining-php-annotations-in-xml/

Majkl578 · 2017-12-08T22:17:27Z

Needs a rebase.

This means that a class implementing a doctrine/common interface or extending a class from that package can throw a specialized exception that will still be caught by code in the parent package. Blocks doctrine#6728

greg0ire · 2018-04-17T21:00:07Z

establish what is the right kind of exception to throw (there is a test that asserts a mapping exception from doctrine/common should be thrown, I don't know why yet)

#7199 should solve this

beberlei

An unrelated follow up could be to get rid of SimpleXML in the driver and use only ext-dom

greg0ire · 2022-04-24T09:55:08Z

@lcobucci regarding the OCP, I read the link you gave and it says (rightly IMO) that the OCP is something that should be taken into account when designing a class. XMLDriver does not seem closed against adding validation functionality, as there is no way to inject a validator in it. Also, I agree that decoration is a great way to add functionality, and use it a lot in my projects, but here the method we would want to decorate is loadMappingFile(), which is protected. That means we cannot use decoration, but would be forced to use inheritance instead to "decorate" the method. Even if it was public and we could do proper decoration, the interface of XMLDriver is not exactly small, which means we would have to proxify a lot of methods to achieve that.

I think a sensible way to violate the OCP as little as possible could be to do the following:

interface XMLValidator
{
    public function __invoke(string $file);
}

class DomXMLValidator implements XMLValidator
{
    // … implementation goes here
}

class XMLDriver
{
    // inject validator in constructor and call it
}

But given the size of the added piece of code, and given how unlikely I think it is people would want to inject their own implementation of the XMLValidator, I'm not sure it's worth it.

Do you still have concerns about runtime deprecations, now that we have switched to https://github.com/doctrine/deprecations/ ? That lib wasn't discussed at all in doctrine/doctrine-website#200

lcobucci · 2022-04-25T20:12:19Z

@lcobucci regarding the OCP, I read the link you gave and it says (rightly IMO) that the OCP is something that should be taken into account when designing a class. XMLDriver does not seem closed against adding validation functionality, as there is no way to inject a validator in it. (...)

The great thing about "principles" that version of me from 4 years ago hadn't learned well is that they aren't absolute rules 😆. I (now) know that understanding the context and allowing some flexibility is important - even though I tend to stick to them as much as possible.

If I was making this feature, I'd still aim to add a new component instead of modifying the existing one to conditionally change its behaviour based on a boolean flag. I believe that adding a new (final) class (SchemaEnforcingXmlDriver, whateva) that - initially - inherits the XmlDriver makes things more explicit in the long run and gives more flexibility on refactoring than the boolean flag approach 😄

With that said, I understand your arguments and agree that a completely different design would make things much better buuuuut 🤣

Do you still have concerns about runtime deprecations, now that we have switched to https://github.com/doctrine/deprecations/ ? That lib wasn't discussed at all in doctrine/doctrine-website#200

Doctrine has fully embraced that deprecations library so I believe that's a moot point now 👍

lcobucci

Just some minor improvements on the SA side of things =)

tests/Doctrine/Tests/ORM/Mapping/XmlMappingDriverTest.php

Co-Authored-By: Axel Venet <avenet@wamiz.com> Co-authored-by: Luís Cobucci <lcobucci@gmail.com>

greg0ire · 2022-04-25T21:05:51Z

Thanks for reviving this @Kern046 ! And thanks for your help @lcobucci ! 5 years after the start, let's finally merge this :)

User Deprecated: Using XML mapping driver with XSD validation disabled is deprecated and will not be supported in Doctrine ORM 3.0. (XmlDriver.php:60 called by SimplifiedXmlDriver.php:23, doctrine/orm#6728, package doctrine/orm)

greg0ire commented Sep 25, 2017

View reviewed changes

greg0ire mentioned this pull request Sep 25, 2017

Weird XML associations error #6444

Closed

greg0ire force-pushed the validate_xml_against_xsd branch from e71713a to 1d91644 Compare September 26, 2017 09:49

Majkl578 added Improvement WIP labels Oct 1, 2017

Majkl578 suggested changes Oct 1, 2017

View reviewed changes

greg0ire force-pushed the validate_xml_against_xsd branch 3 times, most recently from 6dd616c to e3985e3 Compare October 1, 2017 21:08

greg0ire changed the base branch from master to develop October 1, 2017 21:17

greg0ire force-pushed the validate_xml_against_xsd branch from e3985e3 to 0a1bca7 Compare October 1, 2017 21:18

SenseException reviewed Oct 2, 2017

View reviewed changes

greg0ire mentioned this pull request Oct 13, 2017

Disallow orphan removal attribute on many-to-one #6772

Merged

alextech mentioned this pull request Nov 26, 2017

Supply default values for optional values in sequence mapping if omitted by driver. #6683

Merged

guilhermeblanco force-pushed the develop branch from 9f5fd37 to e7d61c2 Compare December 4, 2017 00:36

lcobucci added this to the 3.0 milestone Dec 17, 2017

lcobucci force-pushed the develop branch from 5c2ff27 to cebe300 Compare December 31, 2017 18:27

Majkl578 added the Rebase needed label Jan 1, 2018

Majkl578 changed the base branch from develop to master January 2, 2018 20:30

greg0ire changed the base branch from master to 2.7 January 11, 2018 22:51

greg0ire force-pushed the validate_xml_against_xsd branch from 0a1bca7 to 36f01cd Compare January 11, 2018 22:52

greg0ire mentioned this pull request Apr 17, 2018

Inherit from homonym exception in parent package #7199

Merged

greg0ire dismissed SenseException’s stale review via 1b94f9f April 23, 2022 07:41

greg0ire requested a review from derrabus April 23, 2022 10:08

beberlei previously approved these changes Apr 23, 2022

View reviewed changes

derrabus previously approved these changes Apr 23, 2022

View reviewed changes

derrabus removed WIP Rebase needed labels Apr 23, 2022

derrabus modified the milestones: 3.0.0, 2.13.0 Apr 23, 2022

greg0ire requested a review from lcobucci April 24, 2022 09:55

lcobucci previously approved these changes Apr 25, 2022

View reviewed changes

tests/Doctrine/Tests/ORM/Mapping/XmlMappingDriverTest.php Outdated Show resolved Hide resolved

tests/Doctrine/Tests/ORM/Mapping/XmlMappingDriverTest.php Outdated Show resolved Hide resolved

greg0ire dismissed stale reviews from lcobucci, derrabus, and beberlei via 1364650 April 25, 2022 20:42

greg0ire force-pushed the validate_xml_against_xsd branch from 69b16b9 to 116a632 Compare April 25, 2022 20:44

Validate XML mapping against XSD file

ab3a255

Co-Authored-By: Axel Venet <avenet@wamiz.com> Co-authored-by: Luís Cobucci <lcobucci@gmail.com>

greg0ire force-pushed the validate_xml_against_xsd branch from 116a632 to ab3a255 Compare April 25, 2022 20:55

greg0ire merged commit f4585b9 into doctrine:2.13.x Apr 25, 2022

greg0ire deleted the validate_xml_against_xsd branch April 25, 2022 21:04

dmaicher mentioned this pull request Feb 28, 2023

Fix all deprecations doctrine/DoctrineBundle#1634

Merged

dmaicher mentioned this pull request Apr 14, 2023

Fix XSD valdiation deprecation from ORM doctrine/DoctrineBundle#1647

Closed

blue3121 mentioned this pull request Jul 24, 2023

500 Internal Server Error Part-DB/Part-DB-server#332

Closed

mase76 mentioned this pull request Aug 8, 2023

Error 500 when logging in Part-DB/Part-DB-server#343

Closed

myval mentioned this pull request Aug 19, 2023

PartKeepr to Part-DB migration in docker Part-DB/Part-DB-server#334

Closed

This was referenced Nov 16, 2023

Enable XSD validation for Doctrine ORM 3.0 compat PhilETaylor/webauthn-framework#1

Closed

Enable XSD validation for Doctrine ORM 3.0 compat web-auth/webauthn-framework#522

Closed

oleg-andreyev mentioned this pull request Mar 11, 2024

XSD Validation extention point. #11349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate XML mapping against XSD file #6728

Validate XML mapping against XSD file #6728

greg0ire commented Sep 25, 2017 •

edited

Loading

greg0ire Sep 25, 2017 •

edited

Loading

alcaeus Sep 26, 2017

greg0ire Sep 26, 2017

greg0ire commented Sep 26, 2017 •

edited

Loading

Majkl578 left a comment

Majkl578 Oct 1, 2017

greg0ire Oct 1, 2017

Majkl578 Oct 1, 2017

greg0ire Oct 1, 2017

greg0ire Oct 1, 2017

Majkl578 Oct 1, 2017

greg0ire Oct 1, 2017

Majkl578 Oct 1, 2017

Majkl578 commented Oct 1, 2017

greg0ire commented Oct 1, 2017

greg0ire commented Oct 1, 2017 •

edited

Loading

greg0ire commented Oct 1, 2017

SenseException Oct 2, 2017

alcaeus Oct 2, 2017

SenseException Oct 2, 2017

alcaeus Oct 2, 2017 •

edited

Loading

greg0ire Oct 2, 2017

alcaeus Oct 2, 2017

Majkl578 Dec 18, 2017

Ocramius Dec 20, 2017

Majkl578 commented Dec 8, 2017

greg0ire commented Apr 17, 2018

beberlei left a comment

greg0ire commented Apr 24, 2022

lcobucci commented Apr 25, 2022

lcobucci left a comment

greg0ire commented Apr 25, 2022

Validate XML mapping against XSD file #6728

Validate XML mapping against XSD file #6728

Conversation

greg0ire commented Sep 25, 2017 • edited Loading

TODO

greg0ire Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Sep 26, 2017 • edited Loading

Majkl578 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 commented Oct 1, 2017

greg0ire commented Oct 1, 2017

greg0ire commented Oct 1, 2017 • edited Loading

greg0ire commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 commented Dec 8, 2017

greg0ire commented Apr 17, 2018

beberlei left a comment

Choose a reason for hiding this comment

greg0ire commented Apr 24, 2022

lcobucci commented Apr 25, 2022

lcobucci left a comment

Choose a reason for hiding this comment

greg0ire commented Apr 25, 2022

greg0ire commented Sep 25, 2017 •

edited

Loading

greg0ire Sep 25, 2017 •

edited

Loading

greg0ire commented Sep 26, 2017 •

edited

Loading

greg0ire commented Oct 1, 2017 •

edited

Loading

alcaeus Oct 2, 2017 •

edited

Loading