Describe iterable type arrays

[franmomu]

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

This describes better every array type in ClassMetadata.

In the FieldMapping psalm type, I've put together all the options I've seen (maybe there are more), the good thing about creating a type is that we can reuse it in more places (like UnitOfWork which some of its methods receive an array $mapping).

[franmomu]

I've used this psalm type to define the property fieldMappings and also as argument for some methods. Maybe this is not totally accurated since for fieldMappings I guess there are more mandatory keys than type, but I think for now is fine.

We could maybe create a FieldMappingParameter with all the optional keys and then FieldMapping restricting the mandatory ones, something like this:

/**
 * @psalm-type FieldMappingParameter = array{
 *     type: string, 
 *     fieldName?: string, 
 *     name?: string, 
 *     strategy?: string, 
 *     ...
 * }
 * @psalm-type FieldMapping = FieldMappingParameter&array{
 *     fieldName: string, 
 *     name: string
 * }
*/

[alcaeus]

Wow, this is cool stuff! I've left some questions below.

As for required and optional keys in the field mapping, there are most likely more required fields other than type, but I'd have to go through this myself to figure that out. I can do so once you're done making changes, but if you feel brave, I'll gladly let you figure out what the code assumes to be there ;)

[alcaeus]

I'm wondering if we should define types for this. Consider:

Suggested change

	* @psalm-var array<array{keys: array<string, mixed>, options: array<string, mixed>}>
	* @psalm-var array<IndexMapping>

Defining an IndexMapping psalm type elsewhere that makes use of other types (e.g. IndexKeys and IndexOptions) would make for easier reading and understanding of what is actually stored.

[franmomu]

Good idea! Done, I've defined them in ClassMetadata, but I guess maybe should belong somewhere else.

[franmomu]

This is how far I've arrived, I had to repeat the keys as defining them as intersection of FielMappingConfig&{mandatory_field: type} doesn't seem to work properly.

I've copied the mandatory keys at the beginning of the definitions.

[alcaeus]

I left a note on the type, but I believe that what we have here is an improvement worth merging already. I was wanting to experiment with Psalm types a little more, so that will be a nice exercise when refactoring the Metadata class as alluded to in another PR.

[alcaeus]

Looking at this, I believe we need two types:

• The FieldMapping type: this type defines how we store the field metadata internally
• The FieldMappingConfig type: this type defines what users can pass to the mapField method.

For example, the mapField method requires either fieldName or name property to be given, but once the mapping is stored internally, both fields will be set. Having them optional in the internal type requires more type checks than necessary, hence the suggestion to define separate types.

[franmomu]

Yep, this is how is done, mapField is defined like:

/**     
 * Map a field.     
 *     
 * @psalm-param FieldMappingConfig $mapping     
 *     
 * @psalm-return FieldMapping     
 */

[alcaeus]

@franmomu can you rebase to resolve the conflicts?

[franmomu]

I had to make type also optional in FieldMappingConfig because these methods

mongodb-odm/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

Lines 1252 to 1257 in f225a0c

	public function mapOneEmbedded(array $mapping): void
	{
	$mapping['embedded'] = true;
	$mapping['type'] = 'one';
	$this->mapField($mapping);
	}

do not require type.

[alcaeus]

Thanks @franmomu!