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

Field mapping DTO #10607

Merged
merged 2 commits into from Mar 31, 2023
Merged

Field mapping DTO #10607

merged 2 commits into from Mar 31, 2023

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Mar 31, 2023

In the past, it has been decided to use arrays for this out of
legitimate performance concerns. But PHP has evolved, and now, it is
more performant and memory efficient to use objects.

We are going to have many DTOs, all of which will need an array access
implementation as a backward compatibility layer.
@greg0ire greg0ire changed the title Field mapping dto Field mapping DTO Mar 31, 2023
@greg0ire greg0ire marked this pull request as draft March 31, 2023 07:18
@greg0ire greg0ire mentioned this pull request Mar 31, 2023
12 tasks
@greg0ire greg0ire marked this pull request as ready for review March 31, 2023 10:22
lib/Doctrine/ORM/Mapping/FieldMapping.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Mapping/FieldMapping.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Mapping/FieldMapping.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Mapping/FieldMapping.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Mapping/FieldMapping.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Mapping/FieldMapping.php Outdated Show resolved Hide resolved
In the past, it has been decided to use arrays for this out of
legitimate performance concerns. But PHP has evolved, and now, it is
more performant and memory efficient to use objects.
@@ -257,7 +257,7 @@ private function formatMappings(array $propertyMappings): array
foreach ($propertyMappings as $propertyName => $mapping) {
$output[] = $this->formatField(sprintf(' %s', $propertyName), '');

foreach ($mapping as $field => $value) {
foreach ((array) $mapping as $field => $value) {
Copy link
Member

Choose a reason for hiding this comment

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

I assume having an IteratorAggregate would be too much and is supposed to be kept simple.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and it's not like this is a collection or something.

@SenseException
Copy link
Member

Thank you for the effort of splitting up the former PR. ❤️

@greg0ire greg0ire dismissed derrabus’s stale review March 31, 2023 21:34

The requested changes have been addressed.

@greg0ire greg0ire added this to the 3.0.0 milestone Mar 31, 2023
@greg0ire greg0ire merged commit e4a7403 into doctrine:3.0.x Mar 31, 2023
37 checks passed
@greg0ire greg0ire deleted the field-mapping-dto branch March 31, 2023 21:34
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.

None yet

3 participants