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

Add XSD "orm:columntoken" type in order to support reserved words in column names #9123

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Oct 13, 2021

I don't know if this is the proper way to support backticks in the column definition from the XML perspective, but currently if they are used, the schema validation fails (see this sample action):

element field: Schemas validity error : Element '{http://doctrine-project.org/schemas/orm/doctrine-mapping}field', attribute 'column': '`count`' is not a valid value of the atomic type 'xs:NMTOKEN'.

See Quoting Reserved Words.

@phansys phansys force-pushed the quotes_in_column_names branch 3 times, most recently from a4cdee6 to e872386 Compare October 14, 2021 17:43
@phansys phansys changed the title Add a test case using reserved words in column names Use "xs:string" instead of "xs:NMTOKEN" for column name definitions in order to allow backticks for reserved words Oct 14, 2021
@phansys phansys force-pushed the quotes_in_column_names branch 3 times, most recently from 21947f0 to fdf8b71 Compare October 14, 2021 17:53
@phansys phansys marked this pull request as ready for review October 14, 2021 18:54
@wickedOne
Copy link
Contributor

have you considered making a custom simple type which just adds backticks to the pattern of NMTOKEN and use that as type for column definitions?

xs:string also allows tabs, carriage returns, etc which you don't want in your column names...

possible implementation would be:

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

@phansys phansys force-pushed the quotes_in_column_names branch from fdf8b71 to e662483 Compare October 28, 2021 16:44
@phansys phansys force-pushed the quotes_in_column_names branch from e662483 to 705d88e Compare October 28, 2021 17:00
Copy link
Contributor Author

@phansys phansys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for your help @wickedOne.
I've changed the type as suggested.

@phansys phansys changed the title Use "xs:string" instead of "xs:NMTOKEN" for column name definitions in order to allow backticks for reserved words Add XSD "orm:columntoken" type in order to support reserved words in column names Oct 28, 2021
@phansys
Copy link
Contributor Author

phansys commented Oct 28, 2021

Since the support for the backticks in the column name definition were already supported and the issue is present only in the schema validation, I think this is a bugfix instead of improvement.

@wickedOne
Copy link
Contributor

nitpicking here, but the xsd didn't "support" backticks, the xsd simply isn't enforced (i.e. doctrine doesn't stop processing your mapping because it doesn't validate against the xsd). so "improvement" seems about right to me as it didn't stop you from using this library without this adjustment

@phansys
Copy link
Contributor Author

phansys commented Oct 28, 2021

it didn't stop you from using this library without this adjustment

In my case, this situation is preventing my private builds to pass, since I started to check my schema against the XSD definition.
Obviously, my app was working regardless this problem.

@greg0ire greg0ire merged commit 94bc137 into doctrine:2.10.x Oct 29, 2021
@greg0ire greg0ire added this to the 2.10.3 milestone Oct 29, 2021
@greg0ire
Copy link
Member

Thanks @phansys! Kind of on the fence regarding the label

@phansys phansys deleted the quotes_in_column_names branch October 29, 2021 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants