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

Let XML driver treat <id> field attributes same as regular <field> tag #10813

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

Greg0
Copy link
Contributor

@Greg0 Greg0 commented Jul 4, 2023

According to other drivers, id field, should be treated as regular field and marked as an id.
In XML driver for id-field, there was code duplication in parsing field attributes. However, it was not updated at all for a while, so regular field growth with new attributes (e.g. enum-type) but id-field not.

Similar to other drivers, id-field attributes are parsed by same method in regular-field and enriched by id flag.

For example, without that fix, it is impossible to use XML driver and enum field as part of composite primary key.

Let me know if you need more details for that change.

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Please run vendor/bin/phpcbf and amend your commit.

if (isset($idElement->options)) {
$mapping['options'] = $this->parseOptions($idElement->options->children());
}
$mapping = $this->columnToArray($idElement);
Copy link
Member

@greg0ire greg0ire Jul 5, 2023

Choose a reason for hiding this comment

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

I am not sure this is correct. Before columnToArray() was introduced, the id mapping evaluation looked like this:

if (isset($idElement['type'])) {
$mapping['type'] = (string)$idElement['type'];
}
if (isset($idElement['column'])) {
$mapping['columnName'] = (string)$idElement['column'];
}
if (isset($idElement['column-definition'])) {
$mapping['columnDefinition'] = (string)$idElement['column-definition'];
}

On the other hand, the field mapping evaluation looked like this:

if (isset($fieldMapping['type'])) {
$mapping['type'] = (string)$fieldMapping['type'];
}
if (isset($fieldMapping['column'])) {
$mapping['columnName'] = (string)$fieldMapping['column'];
}
if (isset($fieldMapping['length'])) {
$mapping['length'] = (int)$fieldMapping['length'];
}
if (isset($fieldMapping['precision'])) {
$mapping['precision'] = (int)$fieldMapping['precision'];
}
if (isset($fieldMapping['scale'])) {
$mapping['scale'] = (int)$fieldMapping['scale'];
}
if (isset($fieldMapping['unique'])) {
$mapping['unique'] = ((string)$fieldMapping['unique'] == "false") ? false : true;
}
if (isset($fieldMapping['nullable'])) {
$mapping['nullable'] = ((string)$fieldMapping['nullable'] == "false") ? false : true;
}
if (isset($fieldMapping['version']) && $fieldMapping['version']) {
$metadata->setVersionMapping($mapping);
}
if (isset($fieldMapping['column-definition'])) {
$mapping['columnDefinition'] = (string)$fieldMapping['column-definition'];
}
if (isset($fieldMapping->options)) {
$mapping['options'] = $this->_parseOptions($fieldMapping->options->children());
}

That being said, you mention the other drivers already do what you describe, so I guess we could do this for the sake of consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you said XmlDriver lack of consistency in comparison to other drivers. With annotations/attributes, we declare a standard field and only mark it as ID.

@Id()
@Column(...)

In XML, we must declare the whole field as a different tag and it should accept all standard field parameters.

@Greg0
Copy link
Contributor Author

Greg0 commented Jul 5, 2023

Please run vendor/bin/phpcbf and amend your commit.

@greg0ire code style issues fixed

@Greg0 Greg0 requested a review from greg0ire July 5, 2023 19:17
@greg0ire greg0ire added this to the 2.15.4 milestone Jul 6, 2023
@greg0ire greg0ire added the Bug label Jul 6, 2023
@greg0ire greg0ire merged commit 514f6b8 into doctrine:2.15.x Jul 6, 2023
@greg0ire
Copy link
Member

greg0ire commented Jul 6, 2023

Thanks @Greg0 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants