Add tests to ModelManager and DateFilter

[franmomu]

Subject

This is a part of #360, there are some comments there about these changes.

I am targeting this branch, because this is BCish.

Changelog

### Deprecated
- Deprecated `AbstractDateFilter::typeRequiresValue`.
- Deprecated `ModelManager::camelize`.
- Deprecated constructing `ModelManager` without passing an instance of `PropertyAccessorInterface` as second argument.

[greg0ire]

Coverage increased (+10.6%) to 35.962%

Holy shit! Nice! (and also, the current coverage is so, so low…)

[core23]

AFAIK this is a BC break, because you introduce new API methods without that MUST be implemented.

[VincentLanglet]

See #368 (comment)

[phansys]

Should we add at least one case using \DateTimeImmutable?

[franmomu]

Looks like it won't work: https://github.com/sonata-project/SonataDoctrineMongoDBAdminBundle/blob/3.x/src/Filter/AbstractDateFilter.php#L127

[phansys]

We should change that for the next major.

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[phansys]

Since the checked element here is a property, I think \BadMethodCallException is not the proper exception.

[franmomu]

Now that you point this out, rethinking about this, maybe we can just use PropertyAccessor class instead of all this code.

-        $reflClass = $metadata->reflClass;
+        $propertyAccesor = PropertyAccess::createPropertyAccessorBuilder()
+            ->enableMagicCall()
+            ->getPropertyAccessor();
+
         foreach ($array as $name => $value) {
-            $reflection_property = false;
             // property or association ?
             if (\array_key_exists($name, $metadata->fieldMappings)) {
                 $property = $metadata->fieldMappings[$name]['fieldName'];
-                $reflection_property = $metadata->reflFields[$name];
             } elseif (\array_key_exists($name, $metadata->associationMappings)) {
                 $property = $metadata->associationMappings[$name]['fieldName'];
             } else {
                 $property = $name;
             }

-            $setter = 'set'.$this->camelize($name);
-
-            if ($reflClass->hasMethod($setter)) {
-                if (!$reflClass->getMethod($setter)->isPublic()) {
-                    throw new \BadMethodCallException(sprintf('Method "%s()" is not public in class "%s"', $setter, $reflClass->getName()));
-                }
-
-                $instance->$setter($value);
-            } elseif ($reflClass->hasMethod('__set')) {
-                // needed to support magic method __set
-                $instance->$property = $value;
-            } elseif ($reflClass->hasProperty($property)) {
-                if (!$reflClass->getProperty($property)->isPublic()) {
-                    throw new \BadMethodCallException(sprintf('Property "%s" is not public in class "%s". Maybe you should create the method "set%s()"?', $property, $reflClass->getName(), ucfirst($property)));
-                }
-
-                $instance->$property = $value;
-            } elseif ($reflection_property) {
-                $reflection_property->setValue($instance, $value);
-            }
+            $propertyAccesor->setValue($instance, $property, $value);
         }

The problem I see is that I don't know how to reach this case:

$reflection_property->setValue($instance, $value);

Or change the exception, any suggestion? I've looking for one, but haven't found one that I like.

[phansys]

I don't know if there is any chance to identify the mentioned case through the PropertyAccessor class. Did you find another approach?

[franmomu]

So I think we could replace this with the PropertyAccess class, because what I understand is...

it checks first if there is a public setter:

$setter = 'set'.$this->camelize($name);

if ($reflClass->hasMethod($setter)) {
    if (!$reflClass->getMethod($setter)->isPublic()) {
        throw new \BadMethodCallException(sprintf('Method "%s()" is not public in class "%s"', $setter, $reflClass->getName()));
    }

    $instance->$setter($value);

then checks the __set method:

} elseif ($reflClass->hasMethod('__set')) {
    // needed to support magic method __set
    $instance->$property = $value;

and then a public property:

} elseif ($reflClass->hasProperty($property)) {
    if (!$reflClass->getProperty($property)->isPublic()) {
        throw new \BadMethodCallException(sprintf('Property "%s" is not public in class "%s". Maybe you should create the method "set%s()"?', $property, $reflClass->getName(), ucfirst($property)));
    }

    $instance->$property = $value;

and finally this:

} elseif ($reflection_property) {
    $reflection_property->setValue($instance, $value);
}

I think that $reflection_property->setValue($instance, $value); is covered by the previous cases and it would fail if the property is not accesible. So basically I think it could be all replaced by PropertyAccess and the current exceptions wouldn't be a problem since the classes don't exist anymore.

[VincentLanglet]

You could add the injection in https://github.com/sonata-project/SonataDoctrineMongoDBAdminBundle/blob/3.x/src/Resources/config/doctrine_mongodb.xml#L4

[VincentLanglet]

And what about requiring the param in the next major ? (And add a deprecation in 3.x if it's not provided)

[phansys]

Suggested change

	if (null === $propertyAccessor) {
	@trigger_error(sprintf(
	'Constructing "%s" without passing an instance of "%s" as second argument is deprecated since'
	.' sonata-project/doctrine-mongodb-admin-bundle 3.x and will be mandatory in 4.0',
	static::class,
	PropertyAccessorInterface::class
	if (!$propertyAccessor instanceof PropertyAccessorInterface) {
	@trigger_error(sprintf(
	'Not passing an object implementing "%s" as argument 2 for "%s()" is deprecated since'
	.' sonata-project/doctrine-mongodb-admin-bundle 3.x and will throw a %s error in 4.0.',
	PropertyAccessorInterface::class,
	__METHOD__,
	\TypeError::class

[jordisala1991]

Thank you @franmomu