Add support for custom types with requireSQLConversion and ResultSetMappingBuilder::generateSelectClause()

[kimhemsoe]

No description provided.

[derrabus]

Can you elaborate what this change does?

[kimhemsoe]

When using ResultSetMappingBuilder::generateSelectClause() with custom types with Type::canRequireSQLConversion() => true, we would not call Type::convertToPHPValueSQL() to inject the required sql.

I am trying to fix that here when using ResultSetMappingBuilder::*EntityFromClassMetadata() methods. ResultSetMappingBuilder can help with some of the pain of building native queries.

There is still the case when using ResultSetMappingBuilder::addFieldResult() and etc which is not covered. i am affraid that changing those could maybe break peoples hacks out there.

Not sure if i can/should fix thee psalm issues.

Also could be nice if we could do this one place one day. I stole some of this from

orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

Line 1463 in 96bc214

protected function getSelectColumnSQL($field, ClassMetadata $class, $alias = 'r')

PS. Sorry for jumping the gun with review request with failing tests.

[kimhemsoe]

Added test using addFieldResult(), works fine and i was wrong that *EntityFromClassMetadata() is a speciel case.

[kimhemsoe]

@derrabus How do i solve the psalm issue?

[greg0ire]

@kimhemsoe the field you are accessing is documented as possibly unset:

orm/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

Line 76 in 760397c

* columnName?: string,

in #8409 , but looking at the codebase, there are several places where it is assumed to exist, and then the resulting Psalm/Phpstan errors are ignored in the baseline.

I think that after this piece of code is executed, it's always set:

orm/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

Lines 1554 to 1573 in 4117ca3

	protected function validateAndCompleteFieldMapping(array $mapping): array
	{
	// Check mandatory fields
	if (! isset($mapping['fieldName']) || ! $mapping['fieldName']) {
	throw MappingException::missingFieldName($this->name);
	}
	if ($this->isTypedProperty($mapping['fieldName'])) {
	$mapping = $this->validateAndCompleteTypedFieldMapping($mapping);
	}
	if (! isset($mapping['type'])) {
	// Default to string
	$mapping['type'] = 'string';
	}
	// Complete fieldName and columnName mapping
	if (! isset($mapping['columnName'])) {
	$mapping['columnName'] = $this->namingStrategy->propertyToColumnName($mapping['fieldName'], $this->name);
	}

[greg0ire]

@orklah , do you remember why you marked that key as possibly unset?

[beberlei]

PR is good! This makes sense to add

You should update the baseline of psalm with these two errors included as @greg0ire said its done in other places already, we can then independently look into making it an always existing column in our psalm mapping.

You can update the baseline with ./vendor/bin/psalm --set-baseline=psalm-baseline.xml

[beberlei]

I believe this is unused, not sure why phpcs/psalm don't complain

[greg0ire]

That would be because of this:

orm/phpcs.xml.dist

Lines 34 to 35 in 760397c

	<!-- quite overkill to ignore this, but its necessary since Attributes are not detected yet -->
	<exclude name="SlevomatCodingStandard.Namespaces.UnusedUses.UnusedUse" />

[beberlei]

Ah forgot about having to add that myself :)

[greg0ire]

Fixed in #9338

[orklah]

@orklah , do you remember why you marked that key as possibly unset?

Probably a lack of knowledge of Doctrine. What I can see is a property initialized to empty array and not initialized in constructor. But maybe Doctrine make sure to initialize some properties before anything happens with the property

[greg0ire]

What I can see is a property initialized to empty array and not initialized in constructor.

That's fieldMappings with an s, which contains an array of FieldMapping, which is true even if it's an empty array.

I think I'll just try removing that ? and see how SA reacts.

[kimhemsoe]

@beberlei What we if i add a FieldMapping class with array access in another PR? How much will break? Or is there another better plan to get rid of some of the many arrays?

[beberlei]

that is a 3.0 task

[greg0ire]

Addressed in #9339

[derrabus]

This does not look like a bugfix. Let's target 2.11.x instead?

[kimhemsoe]

"Use var_dump()" removed, rebased and retarget to 2.11.x. psalm issues should be solved been when 2.10.x is merged up to 2.11.x

[greg0ire]

Thanks @kimhemsoe !