Skip to content
This repository was archived by the owner on Jul 28, 2022. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@
],
"require": {
"php": "^7.2",
"doctrine/inflector": "^1.4 || ^2.0",
"symfony/config": "^3.4 || ^4.4 || ^5.0",
"symfony/dependency-injection": "^3.4 || ^4.4 || ^5.0",
"symfony/form": "^3.4 || ^4.4 || ^5.0",
"symfony/http-kernel": "^3.4 || ^4.4 || ^5.0"
},
"require-dev": {
"doctrine/orm": "^2.4",
"doctrine/orm": "^2.7",
"symfony/phpunit-bridge": "^5.0"
},
"config": {
Expand Down
21 changes: 21 additions & 0 deletions src/Exception/NoValueException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php

declare(strict_types=1);

/*
* This file is part of the Sonata Project package.
*
* (c) Thomas Rabaix <thomas.rabaix@sonata-project.org>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Sonata\DatagridBundle\Exception;

/**
* @author Thomas Rabaix <thomas.rabaix@sonata-project.org>
*/
class NoValueException extends \Exception
{
}
332 changes: 332 additions & 0 deletions src/Field/BaseFieldDescription.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,332 @@
<?php

declare(strict_types=1);

/*
* This file is part of the Sonata Project package.
*
* (c) Thomas Rabaix <thomas.rabaix@sonata-project.org>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Sonata\DatagridBundle\Field;

use Doctrine\Inflector\InflectorFactory;
use Sonata\DatagridBundle\Exception\NoValueException;

/**
* @author Thomas Rabaix <thomas.rabaix@sonata-project.org>
*/
abstract class BaseFieldDescription implements FieldDescriptionInterface
{
/**
* @var string|null the field name
*/
protected $name;

/**
* @var string|null the field name (of the form)
*/
protected $fieldName;

/**
* @var string|int|null the type
Copy link
Member

Choose a reason for hiding this comment

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

Why int? Can you explain what are possible int values?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just basically moved the file from the AdminBundle to the DatagridBundle
https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Admin/BaseFieldDescription.php#L70

I don't know why int. But it was allowed before.

Copy link
Member Author

Choose a reason for hiding this comment

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

I add explanation. For example, ClassMetadata::ONE_TO_MANY is allowed.

Copy link
Member

@core23 core23 May 27, 2020

Choose a reason for hiding this comment

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

This looks like an architecture problem we should fix when moving this to the right place.

The type property name is to generic to guess the right value. Someone could misuse this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like an architecture problem

Maybe, this seems to works well like this currently.

After some investigation,
the type is a string like orm_many_to_one

The only reason to allow int was because of the implementation

public function setAssociationMapping($associationMapping)
    {
        if (!\is_array($associationMapping)) {
            throw new \RuntimeException('The association mapping must be an array');
        }

        $this->associationMapping = $associationMapping;

        $this->type = $this->type ?: $associationMapping['type'];
        $this->mappingType = $this->mappingType ?: $associationMapping['type'];
        $this->fieldName = $associationMapping['fieldName'];
    }

Maybe this implementation is wrong and we should remove the line

$this->type = $this->type ?: $associationMapping['type'];

Maybe we should also makes field private in order to force the usage of getter/setter.
I'll do it tonight.

We should fix when moving this to the right place.

I disagree.

Keep in mind there is mutiple of issue we could fix for datagrid, field descriptions and more.
I can't fix them all.

Fixing some of them is still better than fixing nothing.
I would prefer to focus about the duplicated code between the two bundles.

Copy link
Member Author

Choose a reason for hiding this comment

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

After some more investigation, there some times where the type is set as an integer (for orm mapping), when I remove this behaviour, the type is null.

The type is use in DoctrineORMAdmin

$fieldDescription->setTemplate($this->getTemplate($fieldDescription->getType()));

If no template is found (which occur for 1, 2, 3 or null), it then use the Mapping type.

A possible solution would be to restrict type to ?string and mapping type to ?int.
And we'll have to stop throwing error when type is not found.

WDYT @core23 ?

*/
protected $type;

/**
* @var string|int|null the original mapping type
Copy link
Member

Choose a reason for hiding this comment

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

Same for this

*/
protected $mappingType;

/**
* @var array the ORM association mapping
*/
protected $associationMapping = [];

/**
* @var array the ORM field information
*/
protected $fieldMapping = [];

/**
* @var array the ORM parent mapping association
*/
protected $parentAssociationMappings = [];

/**
* @var string the template name
*/
protected $template;

/**
* @var array the option collection
*/
protected $options = [];

/**
* @var array[] cached object field getters
*/
private static $fieldGetters = [];

public function setFieldName(string $fieldName): void
{
$this->fieldName = $fieldName;
}

public function getFieldName(): ?string
{
return $this->fieldName;
}

public function setName(string $name): void
{
$this->name = $name;

if (!$this->getFieldName()) {
$this->setFieldName(substr(strrchr('.'.$name, '.'), 1));
}
}

public function getName(): ?string
{
return $this->name;
}

public function setTemplate(string $template): void
{
$this->template = $template;
}

public function getTemplate(): ?string
{
return $this->template;
}

/**
* @param int|string $type
*/
public function setType($type): void
{
$this->type = $type;
}

public function getType()
{
return $this->type;
}

/**
* @param mixed $default
*
* @return mixed
*/
public function getOption(string $name, $default = null)
{
return isset($this->options[$name]) ? $this->options[$name] : $default;
}

/**
* @param mixed $value
*/
public function setOption(string $name, $value): void
{
$this->options[$name] = $value;
}

public function setOptions(array $options): void
{
// set the type if provided
if (isset($options['type'])) {
$this->setType($options['type']);
unset($options['type']);
}

// set the template if provided
if (isset($options['template'])) {
$this->setTemplate($options['template']);
unset($options['template']);
}

$this->options = $options;
}

public function getOptions(): array
{
return $this->options;
}

public function mergeOption(string $name, array $options = []): void
{
if (!isset($this->options[$name])) {
$this->options[$name] = [];
}

if (!\is_array($this->options[$name])) {
throw new \RuntimeException(sprintf('The key `%s` does not point to an array value', $name));
}

$this->options[$name] = array_merge($this->options[$name], $options);
}

public function mergeOptions(array $options = []): void
{
foreach ($options as $name => $option) {
if (\is_array($option)) {
$this->mergeOption($name, $option);
} else {
$this->setOption($name, $option);
}
}
}

public function getAssociationMapping(): array
{
return $this->associationMapping;
}

public function getFieldMapping(): array
{
return $this->fieldMapping;
}

public function getParentAssociationMappings(): array
{
return $this->parentAssociationMappings;
}

/**
* @param int|string $mappingType
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this doc block? Ain't it the same on the interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't easier to have the phpdoc in the file you're reading when you're coding/looking in your vendor, etc ?
I think it's always useful

Copy link
Member

Choose a reason for hiding this comment

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

Most IDEs can use autocompletion / show methods docs from upper interfaces. The problem is that duplicate docblocks will one day get out of sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand.

But because there was no docblocks in the same class in the SonataAdminBundle, a bug was easily introduce in master by restricting the value to ?string.
https://github.com/sonata-project/SonataAdminBundle/blob/master/src/Admin/BaseFieldDescription.php#L227

You proved me that I made the right things by adding this tricky docblock: The first time you read it, you thought that int was a mistake.
Without this docblock I can bet that someone gonna restrict the value to string.

*/
public function setMappingType($mappingType): void
{
$this->mappingType = $mappingType;
}

public function getMappingType()
{
return $this->mappingType;
}

public function isVirtual(): bool
{
return false !== $this->getOption('virtual_field', false);
}

/**
* @throws NoValueException
*
* @return mixed
*/
public function getFieldValue(?object $object, ?string $fieldName)
{
if ($this->isVirtual() || null === $object) {
return null;
}

$getters = [];
$parameters = [];

// prefer method name given in the code option
if ($this->getOption('code')) {
$getters[] = $this->getOption('code');
}
// parameters for the method given in the code option
if ($this->getOption('parameters')) {
$parameters = $this->getOption('parameters');
}

if (null !== $fieldName && '' !== $fieldName) {
if ($this->hasCachedFieldGetter($object, $fieldName)) {
return $this->callCachedGetter($object, $fieldName, $parameters);
}

$camelizedFieldName = InflectorFactory::create()->build()->classify($fieldName);

$getters[] = 'get'.$camelizedFieldName;
$getters[] = 'is'.$camelizedFieldName;
$getters[] = 'has'.$camelizedFieldName;
}

foreach ($getters as $getter) {
if (method_exists($object, $getter) && \is_callable([$object, $getter])) {
$this->cacheFieldGetter($object, $fieldName, 'getter', $getter);

return $object->{$getter}(...$parameters);
}
}

if (null !== $fieldName) {
if (method_exists($object, '__call')) {
$this->cacheFieldGetter($object, $fieldName, 'call');

return $object->{$fieldName}(...$parameters);
}

if ('' !== $fieldName && isset($object->{$fieldName})) {
$this->cacheFieldGetter($object, $fieldName, 'var');

return $object->{$fieldName};
}
}

throw new NoValueException(sprintf(
'Neither the property "%s" nor one of the methods "%s()" exist and have public access in class "%s".',
$this->getName(),
implode('()", "', $getters),
\get_class($object)
));
}

private function getFieldGetterKey(object $object, ?string $fieldName): ?string
{
if (null === $fieldName) {
return null;
}

$components = [\get_class($object), $fieldName];

$code = $this->getOption('code');
if (\is_string($code) && '' !== $code) {
$components[] = $code;
}

return implode('-', $components);
}

private function hasCachedFieldGetter(object $object, string $fieldName): bool
{
return isset(
self::$fieldGetters[$this->getFieldGetterKey($object, $fieldName)]
);
}

private function callCachedGetter(object $object, string $fieldName, array $parameters = [])
{
$getterKey = $this->getFieldGetterKey($object, $fieldName);

if ('getter' === self::$fieldGetters[$getterKey]['method']) {
return $object->{self::$fieldGetters[$getterKey]['getter']}(...$parameters);
}

if ('call' === self::$fieldGetters[$getterKey]['method']) {
return $object->__call($fieldName, $parameters);
}

return $object->{$fieldName};
}

private function cacheFieldGetter(object $object, ?string $fieldName, string $method, ?string $getter = null): void
{
$getterKey = $this->getFieldGetterKey($object, $fieldName);

if (null !== $getterKey) {
self::$fieldGetters[$getterKey] = ['method' => $method];
if (null !== $getter) {
self::$fieldGetters[$getterKey]['getter'] = $getter;
}
}
}
}
Loading