Add FieldDescriptionInterface by VincentLanglet · Pull Request #234 · sonata-project/SonataDatagridBundle

VincentLanglet · 2020-05-21T20:11:51Z

Subject

I am targeting this branch, because BC.

Related to #233

I want to

Add this FieldDescriptionInterface in Datagrid:3.x
Merge into master
Update the DatagridInterface, PagerInterface etc in Datagrid:4.0
Require datagrid-bundle@dev-master in admin-bundle@dev-master
Use/Extends the Datagrid interfaces in the AdminBundle
Deprecate some of the AdminBundle interface

The original AdminBundle FieldDescription expose others methods

Some methods related to the admin, I'll extends the Datagrid-FieldDescriptionInterface in teh Admin-FieldDescriptionInterface.
getLabel, getTranslationDomain, getHelp methods. I will deprecate these methods and recommend to use getOption($name) instead. Because if we want to introduce label_translation_parameters, help_translation_domain, help_translation_parameters, etc we don't want to create a new method every time and add it in the interface. You can notice that getHelp was not added in the Interface but it still used in the twig.

Changelog

### Added
- Added `FieldDescription` and `FieldDescriptionInterface`.

VincentLanglet · 2020-05-21T21:35:15Z

I fixed all the tests I could @greg0ire.

The only one I don't know how to deal with is the Phpstan one.

It's maybe related with
doctrine/orm#7955
doctrine/orm#7953

Since you worked on this, maybe can you help me ?

VincentLanglet · 2020-05-22T10:17:51Z

@greg0ire Seems like doctrine/orm:2.7.3 is needed for fixing Phpstan.

Is it ok tu use 2.7.x-dev ATM, since it's in the require-dev section ? @sonata-project/contributors

greg0ire · 2020-05-23T21:25:05Z

Are you talking about this: https://github.com/sonata-project/SonataDatagridBundle/runs/697689115?check_suite_focus=true ? I don't reproduce that issue locally on 3.x

Trying to reproduce it on fb32707, but composer update sure takes a looooong time

VincentLanglet · 2020-05-23T21:34:02Z

Are you talking about this: https://github.com/sonata-project/SonataDatagridBundle/runs/697689115?check_suite_focus=true ?

Yes, I had to require "doctrine/orm": "2.7.x-dev" in the require-dev section in order to fix this issue on the CI.

I thinks doctrine/orm#7953 fix the Phpstan error so requiring doctrine/orm: ^2.7.3 will be a more stable fix (when the release will exists).

I didn't find any other way to fix the build. I would say it could be ok to require a non-stable branch, since it's only in the require-dev section and only for a limited time.

greg0ire · 2020-05-23T21:38:49Z

Ok now I'm reproducing the issue. It's very simple, and it's unrelated to my PR. Rather, it's related to this one: doctrine/orm@26806d0.

A simple fix would be to require doctrine/persistence, but we would have to revert it later once we can use ^2.7.3, which is due soon I think.

VincentLanglet · 2020-05-23T22:00:25Z

A simple fix would be to require doctrine/persistence, but we would have to revert it later once we can use ^2.7.3, which is due soon I think.

Indeed ! Ready to be reviewed then :)

greg0ire · 2020-05-24T07:50:32Z

What I still don't understand is why this doesn't happen on 3.x? Can you please investigate? I think the doctrine/persistence should be in a separate commit with a message that explains why we do this.

VincentLanglet · 2020-05-24T08:26:34Z

@greg0ire More fun, bumping to doctrine/orm ^2,5 is enough 😅

Maybe thanks to this change

I made the bump in a second commit.
Is it enough for you to approve this PR ? :)

greg0ire · 2020-05-24T08:39:32Z

Ok, if you don't want to investigate, say so, I will do it… Looks like doctrine/orm has the requirement on 2.7.2: https://github.com/doctrine/orm/blob/v2.7.2/composer.json#L28

I won't understand why doctrine/orm@26806d0 is not annotated with tags on GH, but:

$ git tag --contains 26806d08eb181d5254c8cd9b8ba0e82bcbc9c284                                        
v2.7.0
v2.7.1
v2.7.2

So you want to bump the dependency to ^2.7

VincentLanglet · 2020-05-24T08:47:02Z

What I don't understand is why ^2,4 installs the 2.4.8 version and why ^2.5 installs the 2.7.2 one.

VincentLanglet · 2020-05-24T09:01:45Z

So you want to bump the dependency to ^2.7

Done

…anager` autoload issue

greg0ire · 2020-05-24T09:04:03Z

What I don't understand is why ^2,4 installs the 2.4.8 version and why ^2.5 installs the 2.7.2 one.

That would be because there are far fewer requirements in 2.4.8: https://github.com/doctrine/orm/blob/v2.4.8/composer.json , which mean Composer can remove lots of dependencies, and use a more recent version of the inflector:

  - Removing symfony/polyfill-php73 (v1.17.0)
  - Removing psr/event-dispatcher (1.0.0)
  - Removing ocramius/package-versions (1.8.0)
  - Removing doctrine/reflection (1.2.1)
  - Removing doctrine/persistence (1.3.7)
  - Removing doctrine/lexer (1.2.0)
  - Removing doctrine/instantiator (1.3.0)
  - Removing doctrine/annotations (1.10.2)
  - Removing symfony/error-handler (v5.0.8)
  - Removing symfony/var-dumper (v5.0.8)
  - Removing doctrine/common (2.13.0)
  - Updating doctrine/inflector (1.4.1 => 2.0.1): Loading from cache
  - Installing symfony/debug (v3.0.9): Loading from cache
 Extracting archive  - Downgrading symfony/console (v5.0.8 => v2.8.52): Loading from cache
  - Downgrading doctrine/orm (v2.7.2 => v2.4.8): Loading from cache
  - Downgrading symfony/http-foundation (v5.0.8 => v4.4.8): Loading from cache
  - Downgrading symfony/event-dispatcher-contracts (v2.0.1 => v1.1.7): Loading from cache
  - Downgrading symfony/event-dispatcher (v5.0.8 => v4.4.8): Loading from cache
  - Downgrading symfony/http-kernel (v5.0.8 => v3.4.21): Loading from cache
  - Downgrading symfony/property-access (v5.0.8 => v4.4.8): Loading from cache
  - Downgrading symfony/options-resolver (v5.0.8 => v4.4.8): Loading from cache
  - Downgrading symfony/intl (v5.0.8 => v4.4.8): Loading from cache
  - Downgrading symfony/form (v5.0.8 => v3.4.40): Loading from cache

phansys · 2020-05-25T14:36:54Z

src/Field/BaseFieldDescription.php

+
+    public function mergeOptions(array $options = []): void
+    {
+        $this->setOptions(array_merge_recursive($this->options, $options));


Maybe we could try to avoid calling array_merge_recursive() (see sonata-project/SonataAdminBundle#6078 (comment)).

I didn't understood the issue with array_merge_recursive. Can you tell me more about it ?

My main concern was to create a FieldDescriptionInterface with the same behaviour than the AdminBundle one in order to stop using some duplicated classes and start using the DatagridBundle instead.

This PR sonata-project/SonataAdminBundle#6078 could be done on this bundle when ready I think.

I didn't understood the issue with array_merge_recursive. Can you tell me more about it ?

The "issue" with array_merge_recursive() is the result it produces if some key is repeated between their arguments. In this example, it converts a value that should be a boolean into an array:

The option "virtual_field" with value array is expected to be of type "bool", but is of type "array".

See https://travis-ci.org/github/sonata-project/SonataAdminBundle/jobs/683117311#L758.

Given that situation, I'd like to avoid the propagation of that issue.

WDYT about my solution @phansys ? :)

WDYT about my solution @phansys ? :)

I agree.

core23 · 2020-05-26T13:59:49Z

src/Field/BaseFieldDescription.php

+    protected $fieldName;
+
+    /**
+     * @var string|int|null the type


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

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.

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

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.

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.

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 ?

core23 · 2020-05-26T13:59:55Z

src/Field/BaseFieldDescription.php

+    protected $type;
+
+    /**
+     * @var string|int|null the original mapping type


Same for this

core23 · 2020-05-26T14:01:01Z

src/Field/BaseFieldDescription.php

+    }
+
+    /**
+     * @param int|string $mappingType


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

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

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

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.

VincentLanglet · 2020-05-31T20:29:28Z

Closing in favor of #239

greg0ire previously approved these changes May 21, 2020

View reviewed changes

VincentLanglet dismissed greg0ire’s stale review via b1e6d15 May 21, 2020 20:47

VincentLanglet closed this May 21, 2020

VincentLanglet reopened this May 21, 2020

VincentLanglet requested a review from a team May 23, 2020 18:17

VincentLanglet requested a review from greg0ire May 23, 2020 22:01

Add FieldDescriptionInterface

7f3202c

Bump doctrine/orm version to fix `Doctrine\Common\Persistence\ObjectM…

f778119

…anager` autoload issue

greg0ire previously approved these changes May 24, 2020

View reviewed changes

greg0ire requested a review from a team May 24, 2020 09:04

VincentLanglet mentioned this pull request May 25, 2020

Replace some references to "entity" term with "model" sonata-project/SonataAdminBundle#6108

Merged

Remove ModelManager-related methods

1423ac7

VincentLanglet dismissed greg0ire’s stale review via 1423ac7 May 25, 2020 06:58

phansys reviewed May 25, 2020

View reviewed changes

phansys added the minor label May 25, 2020

core23 suggested changes May 26, 2020

View reviewed changes

VincentLanglet added 2 commits May 26, 2020 20:55

Add explanations

e25ee1d

Fix mergeOptions

2fbbff0

VincentLanglet mentioned this pull request May 31, 2020

Add FieldDescriptionInterface #239

Closed

VincentLanglet closed this May 31, 2020

Conversation

VincentLanglet commented May 21, 2020

Subject

Changelog

Uh oh!

VincentLanglet commented May 21, 2020

Uh oh!

VincentLanglet commented May 22, 2020

Uh oh!

greg0ire commented May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VincentLanglet commented May 23, 2020

Uh oh!

greg0ire commented May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VincentLanglet commented May 23, 2020

Uh oh!

greg0ire commented May 24, 2020

Uh oh!

VincentLanglet commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VincentLanglet commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VincentLanglet commented May 24, 2020

Uh oh!

greg0ire commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

core23 May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VincentLanglet commented May 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

greg0ire commented May 23, 2020 •

edited

Loading

greg0ire commented May 23, 2020 •

edited

Loading

VincentLanglet commented May 24, 2020 •

edited

Loading

greg0ire commented May 24, 2020 •

edited

Loading

VincentLanglet commented May 24, 2020 •

edited

Loading

greg0ire commented May 24, 2020 •

edited

Loading

core23 May 27, 2020 •

edited

Loading