XSD definition improvements #6389

mikeSimonson · 2017-04-09T20:17:34Z

No description provided.

Ocramius · 2017-04-09T20:30:01Z

doctrine-mapping.xsd

@@ -200,6 +200,13 @@
    <xs:anyAttribute namespace="##other"/>
  </xs:complexType>

+  <xs:simpleType name="TABLENAME" id="TABLENAME">
+    <xs:restriction base="xs:token">
+      <xs:pattern value="[-._:A-Za-z0-9\`]" id="TABLENAME.pattern">


This is way too restrictive

more something like that /[a-zA-Z_\x01-\xff\`]+/ ?
Based on https://dev.mysql.com/doc/refman/5.7/en/identifiers.html

Ocramius · 2017-04-09T20:30:54Z

doctrine-mapping.xsd

  </xs:complexType>

+  <xs:simpleType name="CLASSPATH" id="CLASSPATH">
+    <xs:restriction base="xs:token">
+      <xs:pattern value="([a-zA-Z_$][a-zA-Z\d_$\\]*\.)*[a-zA-Z_$][a-zA-Z\d_$\\]*" id="CLASSPATH.pattern">


The regex for class names is something like /^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]+$/ - not just ascii

Ocramius · 2017-04-09T20:31:32Z

doctrine-mapping.xsd

@@ -200,6 +200,13 @@
    <xs:anyAttribute namespace="##other"/>
  </xs:complexType>

+  <xs:simpleType name="TABLENAME" id="TABLENAME">


What's up with the uppercase naming?

Ocramius · 2017-04-09T20:31:36Z

doctrine-mapping.xsd

  </xs:complexType>

+  <xs:simpleType name="CLASSPATH" id="CLASSPATH">


What's up with the uppercase naming?

mikeSimonson · 2017-04-10T16:48:27Z

@Ocramius updated. Is there an existing place to add tests or should I create it ?

Ocramius · 2017-04-10T19:57:44Z

Is there an existing place to add tests or should I create it ?

Good question. Wondering if there's an XSD validation tool anywhere (not an online tool)

mikeSimonson · 2017-04-10T20:06:16Z

@Ocramius I was thinking using something along the line of http://php.net/manual/en/domdocument.schemavalidate.php

mikeSimonson · 2017-04-10T20:07:26Z

and it seems that this mechanism is already used.

Ocramius · 2017-04-10T20:28:29Z

If you want to create some valid/invalid documents, you can give it a shot. I was more wondering about the XSD itself :-) Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

…

On Mon, Apr 10, 2017 at 10:07 PM, mikeSimonson ***@***.***> wrote: and it seems that this mechanism is already used. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6389 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakIWLQu-R_7V2J1XNCgQeYDxPB5vFks5ruov_gaJpZM4M4I3r> .

mikeSimonson · 2017-04-11T07:41:28Z

doctrine-mapping.xsd

@@ -412,9 +419,16 @@
    <xs:sequence>
      <xs:any minOccurs="0" maxOccurs="unbounded" namespace="##other"/>
    </xs:sequence>
-    <xs:attribute name="class" type="xs:string" use="required" />
+    <xs:attribute name="class" type="orm:classpath" use="required" />


fqcn is probably better here no ?

lcobucci · 2017-04-11T11:05:15Z

If you want to create some valid/invalid documents, you can give it a shot.
I was more wondering about the XSD itself :-)

@Ocramius AFAIK we would have issues when trying to validate a document with an invalid XSD, so I'd be fine once the test suite is passing

Ocramius

LGTM! 👍

Ocramius · 2017-06-21T06:45:41Z

@mikeSimonson this started failing in master, but I really have no clue why: all XML files in that specific test seem to be fine :-\

mikeSimonson · 2017-06-21T10:19:18Z

I also have no clue why and can't reproduce it locally. Le 21 juin 2017 08:45, "Marco Pivetta" <notifications@github.com> a écrit :

…

@mikeSimonson <https://github.com/mikesimonson> this started failing in master, but I really have no clue why: all XML files in that specific test seem to be fine :-\ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6389 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3ZXcTKAPIgQwKiOIRa-Omb2aNbufmHks5sGLwbgaJpZM4M4I3r> .

Ocramius · 2017-06-21T10:36:00Z

Same here - can't reproduce, neither on PHP 7.0 nor on 7.1 :-\ Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

…

On Wed, Jun 21, 2017 at 12:19 PM, mikeSimonson ***@***.***> wrote: I also have no clue why and can't reproduce it locally. Le 21 juin 2017 08:45, "Marco Pivetta" ***@***.***> a écrit : > @mikeSimonson <https://github.com/mikesimonson> this started failing in > master, but I really have no clue why: all XML files in that specific > test seem to be fine :-\ > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#6389 (comment) >, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AA3ZXcTKAPIgQwKiOIRa- Omb2aNbufmHks5sGLwbgaJpZM4M4I3r> > . > — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#6389 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakF2rsIRqp2LNM8RgHs9grnzsX9brks5sGO4ngaJpZM4M4I3r> .

guilliamxavier · 2017-06-21T17:09:15Z

(Sorry if this seems not very constructive, but if it may help a little:)

Is "[a-zA-Z_u01-uff.]+" (and "[a-zA-Z_u01-uff][a-zA-Z0-9_u01-uff]+") really the intended pattern?

About the u01-uff part, isn't it missing backslashes like \u01-\uff, or probably more like \u0001-\u00ff (or \x01-\xff)? (As it is, I interpret it as "u or 0 or a char in the range 1-u (which feels like a pretty weird range) or f or f (bis)"...)
- But wait, [a-zA-Z_\x01-\xff.] would be in fact the same as [\x01-\xff] (which already includes a-z, A-Z, _ and .), which is probably not the intent?
- Moreover, according to http://www.regular-expressions.info/xml.html, "XML regular expressions don't have any tokens like \xFF or \uFFFF to match particular (non-printable) characters", and e.g. http://www.utilities-online.info/xsdvalidation/ indeed flags it as invalid XSD schema. But it seems you could use Unicode blocks like \p{IsBasicLatin} or \p{IsLatin-1Supplement}.

lcobucci · 2017-06-22T08:10:03Z

@guilliamxavier @mikeSimonson mistery solved on #6515, ancient libxml versions (but now we're facing some other issues with MySQL/MariaDB on Trusty env 😜 )

guilliamxavier · 2017-06-22T09:15:15Z

@lcobucci After looking deeper into the test failures, I have indeed realized that they were actually not related to pattern validation...
But, independently of the tests, I still wonder: is "[u01-uff]" actually a desired character class?

lcobucci · 2017-06-22T09:59:02Z

@guilliamxavier tbh I'm not sure, I'll leave that question to @mikeSimonson

mikeSimonson · 2017-06-28T20:55:14Z

@guilliamxavier The charachter class is based on what mysql accept as identifier (https://dev.mysql.com/doc/refman/5.7/en/identifiers.html). If you have a better proposition that also take into account other dbms fire away or better pull request away.

guilliamxavier · 2017-06-29T17:56:11Z

@mikeSimonson Thanks for the feedback. My first and main concern is actually about the syntax of the pattern:

[a-zA-Z_u01-uff.]+

which, as shown below, doesn't accept e.g. "`my-foo`".

TL;DR: u01-uff is not "the range of Unicode characters 01 to ff" (and there are additional issues). Explanations (and possible correction) follow, with test scripts at bottom.

1. XML pattern syntax for Unicode characters

As I said in my first (i.e. second-to-last) comment, the bare (non-escaped) u01 and uff just look wrong to me, and some testing (script further below) actually confirmed that the above pattern means this:

([a-z]|[A-Z]|_|u|0|[1-u]|f|f|\.)+

(where, notably, f is duplicated and [1-u] is an unusual ASCII range corresponding to the following characters: 123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstu) which is very probably not what was intended. For example, it doesn't accept the legit quoted table name `my-foo` in the XML line

    <entity name="Foo" table="`my-foo`">

and gives errors:

[ERROR 1839] Element '{http://doctrine-project.org/schemas/orm/doctrine-mapping}entity', attribute 'table': [facet 'pattern'] The value '`my-foo`' is not accepted by the pattern '[a-zA-Z_u01-uff.]+'.
[ERROR 1824] Element '{http://doctrine-project.org/schemas/orm/doctrine-mapping}entity', attribute 'table': '`my-foo`' is not a valid value of the atomic type '{http://doctrine-project.org/schemas/orm/doctrine-mapping}tablename'.

(because - isn't included in the character class) while it accepts the disputable unquoted table name my;foo in

    <entity name="Foo" table="my;foo">

(because ; is).

I guess that the part u01-uff was actually intended to mean "the range of Unicode characters 01 to ff", so I tried different backslash-escaped forms, e.g. for u01 for U+0001 I tested: \x01, \x{1}, \u0001, \u{1} (and similarly for uff for U+00FF), both lower- and upper-case, with and without braces, varying the amount of leading zeros... but each time got the following kind of errors:

Warning: DOMDocument::schemaValidate(): Invalid Schema in .../test.php on line 10

[FATAL 1450] failed to compile: Wrong escape sequence, misuse of character '\'
[FATAL 1450] failed to compile: xmlFAParseCharClass: ']' expected
[FATAL 1450] failed to compile: xmlFAParseRegExp: extra characters
[ERROR 1756] Element '{http://www.w3.org/2001/XMLSchema}pattern': The value '[a-zA-Z_\x01-\xff.]+' of the facet 'pattern' is not a valid regular expression.

Note that those errors (and PHP warning) occur when libxml is parsing the XSD Schema, before even trying to validate any XML document.

The correct syntax in "XML Regex" is actually to use XML entities e.g.  (or  or ). But now I get:

Warning: DOMDocument::schemaValidate(): Invalid Schema in .../test.php on line 10

[FATAL 9] xmlParseCharRef: invalid xmlChar value 1
[ERROR 3067] Failed to parse the XML resource '.../doctrine-mapping.xsd'.

because U+0001 is not a valid character in XML 1.0 ^{(note: but it is in XML 1.1)}. It seems that the only valid chars before U+0020 (space) are U+0009 (\t), U+000A (\n) and U+000D (\r). So if I change the pattern to

[a-zA-Z_	-ÿ.]+

then it "works", i.e. no more XSD errors, and the quoted table name `my-foo` is accepted (which was the purpose of your commit f537eb2 after all).

2. Actual meaning

But you must realize that 	-ÿ already includes all the chars in a-zA-Z_., so the previous pattern actually boils down to

[	-ÿ]+

which one might find unexpected.

3. [My]SQL identifier

Now, according to your link, the correct pattern for a MySQL identifier would be more like

[0-9a-zA-Z$_-]+|`[--]+`

(NB: FFFF, not 00FF which was meant by ff (ignoring the case)). But again, XML (1.0) doesn't like 0001, and also not FFFF neither FFFE; plus we can merge the ranges ...-007F and 0080-...; so:

[0-9a-zA-Z$_-�]+|`[	-�]+`

which "works". But it is still not complete (emphasis mine):

Identifiers may begin with a digit but unless quoted may not consist solely of digits.

Database, table, and column names cannot end with space characters.

Identifier quote characters can be included within an identifier if you quote the identifier. (...) then you need to double the character. The following statement creates a table named a`b (...): CREATE TABLE `a``b` (...);

(and not accounting that with ANSI_QUOTES you can also use " as the quote character).

And as you said, there are other DBMS, with possibly different accepted chars and restrictions...

Moreover, you seemed to want to accept the dot character, e.g. table="myschema.mytable"?

(At this point, maybe one could as well consider going with just type="xs:string" (as was done in commit fafb816)?)

4. FQCN

The same concerns apply to your second commit b433257, and the correct character class for a FQCN will of course be different than for a MySQL identifier (e.g. it must accept \, and should not accept `, - or ;...)

Finally here is my test.php script ^{(inspired from https://github.com/doctrine/doctrine2/blob/388afb46d0cb3ed0c51332e8df0de9e942c2690b/tests/Doctrine/Tests/ORM/Mapping/XmlMappingDriverTest.php#L151 + PHP manual)}: https://pastebin.com/YEreZ8xC

And some minimalist XML test file examples (change xxx to the values to test):

for table (tablename): https://pastebin.com/DenZs3Rb
for class (fqcn): https://pastebin.com/VfDY7qqg

What do you think?

Ocramius suggested changes Apr 9, 2017

View reviewed changes

mikeSimonson force-pushed the xml-dtd-improvements branch from 4cdfbf0 to 2717aaf Compare April 10, 2017 16:47

mikeSimonson commented Apr 11, 2017

View reviewed changes

mikeSimonson added 3 commits April 11, 2017 09:43

Add a table name type that accept quoted table name

f537eb2

Add a type for the class FQCN

b433257

Do not force an order of definition in the entity elements

0865181

mikeSimonson force-pushed the xml-dtd-improvements branch from 2717aaf to 0865181 Compare April 11, 2017 07:44

Ocramius approved these changes Jun 21, 2017

View reviewed changes

Ocramius changed the title ~~Xml dtd improvements~~ XSD definition improvements Jun 21, 2017

Ocramius added the Improvement label Jun 21, 2017

Ocramius added this to the 2.6.0 milestone Jun 21, 2017

Ocramius self-assigned this Jun 21, 2017

Ocramius merged commit 33cee11 into doctrine:master Jun 21, 2017

mikeSimonson deleted the xml-dtd-improvements branch June 28, 2017 20:55

guilliamxavier mentioned this pull request Jul 26, 2018

Fix XSD regex for fqcn and tablename patterns #7324

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XSD definition improvements #6389

XSD definition improvements #6389

mikeSimonson commented Apr 9, 2017

Ocramius Apr 9, 2017

mikeSimonson Apr 10, 2017

Ocramius Apr 9, 2017

Ocramius Apr 9, 2017

Ocramius Apr 9, 2017

mikeSimonson commented Apr 10, 2017

Ocramius commented Apr 10, 2017

mikeSimonson commented Apr 10, 2017

mikeSimonson commented Apr 10, 2017

Ocramius commented Apr 10, 2017 via email

mikeSimonson Apr 11, 2017

lcobucci commented Apr 11, 2017

Ocramius left a comment

Ocramius commented Jun 21, 2017

mikeSimonson commented Jun 21, 2017 via email

Ocramius commented Jun 21, 2017 via email

guilliamxavier commented Jun 21, 2017

lcobucci commented Jun 22, 2017

guilliamxavier commented Jun 22, 2017

lcobucci commented Jun 22, 2017

mikeSimonson commented Jun 28, 2017

guilliamxavier commented Jun 29, 2017 •

edited

Loading

		</xs:complexType>

		<xs:simpleType name="CLASSPATH" id="CLASSPATH">

XSD definition improvements #6389

XSD definition improvements #6389

Conversation

mikeSimonson commented Apr 9, 2017

Ocramius Apr 9, 2017

Choose a reason for hiding this comment

mikeSimonson Apr 10, 2017

Choose a reason for hiding this comment

Ocramius Apr 9, 2017

Choose a reason for hiding this comment

Ocramius Apr 9, 2017

Choose a reason for hiding this comment

Ocramius Apr 9, 2017

Choose a reason for hiding this comment

mikeSimonson commented Apr 10, 2017

Ocramius commented Apr 10, 2017

mikeSimonson commented Apr 10, 2017

mikeSimonson commented Apr 10, 2017

Ocramius commented Apr 10, 2017 via email

mikeSimonson Apr 11, 2017

Choose a reason for hiding this comment

lcobucci commented Apr 11, 2017

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius commented Jun 21, 2017

mikeSimonson commented Jun 21, 2017 via email

Ocramius commented Jun 21, 2017 via email

guilliamxavier commented Jun 21, 2017

lcobucci commented Jun 22, 2017

guilliamxavier commented Jun 22, 2017

lcobucci commented Jun 22, 2017

mikeSimonson commented Jun 28, 2017

guilliamxavier commented Jun 29, 2017 • edited Loading

1. XML pattern syntax for Unicode characters

2. Actual meaning

3. [My]SQL identifier

4. FQCN

guilliamxavier commented Jun 29, 2017 •

edited

Loading