Keep only common option in FieldDescriptionOptions type by VincentLanglet · Pull Request #7103 · sonata-project/SonataAdminBundle

VincentLanglet · 2021-04-22T08:09:14Z

Subject

Follow up of the discussion
#7101

This type is currently not describing correctly the possible options ATM.
Since @franmomu is not agreeing with this, I prefer to wait for his approval before any merge.

Changelog

### Fixed
- FieldDescriptions options phpdoc to keep only common options.

franmomu · 2021-04-22T09:10:47Z

you are right, I don't agree with this, I think we are missing information and going backwards, if we need more options we can add them, if we want to allow users to add any option (not documented), we should add an option for that, otherwise it's a mess.

Anyway, I do not tend to block things because it's my opinion and I might be wrong, so if there are people that agree with this I'm fine.

jordisala1991 · 2021-04-22T09:54:22Z

IMO if it causes some kind of regression to current code or we are not all agree with that type, we should first revert it and have green builds again. And then discuss about how this can be done.

franmomu · 2021-04-22T10:05:13Z

But it's not because of this that test are not passing, it is after the addition of https://github.com/sonata-project/SonataAdminBundle/pull/7040/files#diff-dba1a2c25ea11b814213238e6c62006084f246e9be5cd8ca251b4f9a65a3a292R56 and the upgrade of phpstan (they can be ignored too)

jordisala1991 · 2021-04-22T10:25:04Z

Then it would fall under the "we are not all agree this is the correct move".

We should agree on: Maybe this is a good move, but the current code does not reflects this behavior, and must be changed before enforcing some options that are not realistic. wdyt?

VincentLanglet · 2021-04-22T13:34:37Z

From the slack discussion:

I think we should only keep the option use by the DatagridMapper, ListMapper, ShowMapper and FormMapper

DatagridMapper use field_name, label, role
ListMapper use associated_property, sortable, sort_field_mapping, actions, label, role, virtual_field
FormMapper use property_path , role, property_path, type, translation_domain, label
ShowMapper use label role safe
So the only common option are label and role
So FieldDescriptionOptions = array{label: ..., role: ...}&array<string, mixed>

But we could create
ListFieldDescriptionOptions = FieldDescriptionOptions&array{sortable: ...., ...}
and so on

Type-specific options like timezone won't still be added to these types, but at least it allows us to restrict some values.
WDYT @franmomu ?

VincentLanglet · 2021-04-22T19:32:40Z

I made some research about the current typed options:

 * @psalm-type FieldDescriptionOptions = array{
 *  accessor?: string|callable|PropertyPathInterface,               // Used by BaseFieldDescription
 *  actions?: array,                                                // Used by a specific type of the List
 *  admin_code?: string,                                            // Used by List, Show and Filter for field with relation
 *  associated_property?: string|callable|PropertyPathInterface,    // Used by List and show for relation
 *  block_name?: string|false,                                      // Used by Form
 *  catalogue?: string,                                             // Used by a specific type of the List/Show
 *  data_transformer?: DataTransformerInterface,                    // Used by Form
 *  edit?: string,                                                  // Used by Form
 *  editable?: bool,                                                // Used by some type of the List
 *  field_name?: string,                                            // Used by Filter
 *  field_options?: array,                                          // Used by Filter and form
 *  field_type?: string,                                            // Used by Filter and form
 *  header_class?: string,                                          // Used by List
 *  identifier?: bool,                                              // Used by List field with relation
 *  inline?: string,                                                // Used by Show/List/Form
 *  label?: string|false|null,                                      // Used by all
 *  link_parameters?: array,                                        // Used by List, Show and Filter for field with relation
 *  multiple?: bool,                                                // Used by some type of the List (related to editable)
 *  placeholder?: string|false,                                     // Used by Form with relation
 *  required?: bool,                                                // Used by List field with relation (related to editable)
 *  role?: string|string[],                                         // Used by all
 *  route?: array,                                                  // Used (differently ?) for type url and some relations of List/Show
 *  safe?: bool,                                                    // Used by some type of the show
 *  sort_field_mapping?: array,                                     // Used by BaseFieldDescription, but only for the List
 *  sort_parent_association_mappings?: array,                       // Used by BaseFieldDescription, but only for the List
 *  sortable?: bool,                                                // Used by BaseFieldDescription, but only for the List
 *  template?: string,                                              // Used by BaseFieldDescription
 *  timezone?: string|\DateTimeZone,                                // Use by a specific type of show/list
 *  translation_domain?: string,                                    // Used by BaseFieldDescription
 *  type?: string,                                                  // Used by BaseFieldDescription
 *  virtual_field?: bool                                            // Used by BaseFieldDescription
 * }&array<string, mixed>

IMHO:

Template-type specific options should be removed
Options used by BaseFieldDescriptions can be kept in the FieldDescriptionOptions
label and role should be kept too.
placeholder and link_parameters shouldn't be set by default in the BaseFieldDescriptions since it's only used for some type/situations.
I'm not sure it makes sens to have isSortable, getSortFieldMapping and getSortParentAssociationMapping in the FieldDescriptionInterface since it's only related to the List...

jordisala1991 · 2021-04-26T17:16:48Z

src/Resources/views/CRUD/Association/edit_many_to_one.html.twig

+                        'linkParameters': sonata_admin.field_description.option('link_parameters', {})
                    })) }}
-                {% elseif sonata_admin.field_description.option('placeholder') %}
+                {% elseif sonata_admin.field_description.option('placeholder', 'short_object_description_placeholder') %}


this can't be correct, otherwise it is an always true if and can be removed with just else

To disabled the placeholder, 'false' should be use.

jordisala1991 · 2021-04-26T17:16:56Z

src/Resources/views/CRUD/Association/edit_one_to_one.html.twig

+                        'linkParameters': sonata_admin.field_description.option('link_parameters', {}))
                    })) }}
-                {% elseif sonata_admin.field_description.option('placeholder') %}
+                {% elseif sonata_admin.field_description.option('placeholder', 'short_object_description_placeholder') %}


jordisala1991 · 2021-04-26T17:17:45Z

src/FieldDescription/BaseFieldDescription.php


-        // set default placeholder
+        // NEXT_MAJOR: Remove this.
        if (!isset($options['placeholder'])) {


if not set, it will set a default, but if it is set to null, it won't, I think the new behavior you implemented don't cover this case

isset(null) is false. So null wasn't the way to disable the placeholder. false is the way.

SonataCI · 2021-04-26T17:31:18Z

Could you please rebase your PR and fix merge conflicts?

VincentLanglet · 2021-04-26T19:28:45Z

I'll try to fix the psalm issue with empty array vimeo/psalm#5677

VincentLanglet · 2021-04-27T00:25:17Z

Use both phpstan-type and psalm-type is a temporary workaroung since psalm need array<empty, empty> (it's a bug) but phpstan does not support this.

franmomu · 2021-04-27T06:38:36Z

src/FieldDescription/BaseFieldDescription.php

- *   - type (m): define the field type (use to tweak the form or the list)
- *   - template (o) : the template used to render the field
- *   - name (o) : the name used (label in the form, title in the list)
- *   - link_parameters (o) : add link parameter to the related Admin class when


Apparently link_parameters it us used in the route generator, so I guess is kind of global:

SonataAdminBundle/src/Route/DefaultRouteGenerator.php

Line 98 in 137ffd9

$parameters = array_merge($parameters, $admin->getParentFieldDescription()->getOption('link_parameters', []));

franmomu · 2021-04-27T06:45:34Z

src/FieldDescription/BaseFieldDescription.php

- *   - associated_tostring : (deprecated, use associated_property option)
- *                           the method to retrieve the "string" representation
- *                           of the collection element.
- *   - associated_property : property path to retrieve the "string" representation


associated_property it is used in RenderElementExtension which means it is used in render_relation_element Twig filter that it is used globally: https://github.com/sonata-project/SonataAdminBundle/search?q=render_relation_element

src/FieldDescription/BaseFieldDescription.php

VincentLanglet marked this pull request as ready for review April 22, 2021 08:09

VincentLanglet requested review from a team and franmomu April 22, 2021 08:09

VincentLanglet marked this pull request as draft April 22, 2021 19:29

VincentLanglet marked this pull request as ready for review April 23, 2021 08:26

VincentLanglet changed the title ~~Remove FieldDescriptionOptions~~ Keep only common option in FieldDescriptionOptions type Apr 24, 2021

VincentLanglet requested a review from jordisala1991 April 26, 2021 13:43

jordisala1991 reviewed Apr 26, 2021

View reviewed changes

SonataCI added pending author and removed pending author labels Apr 26, 2021

jordisala1991 previously approved these changes Apr 26, 2021

View reviewed changes

VincentLanglet dismissed jordisala1991’s stale review via 3a4e53c April 27, 2021 00:23

VincentLanglet mentioned this pull request Apr 27, 2021

3.x into Master #7119

Merged

VincentLanglet requested review from a team and jordisala1991 April 27, 2021 00:25

jordisala1991 previously approved these changes Apr 27, 2021

View reviewed changes

jordisala1991 requested a review from a team April 27, 2021 05:09

franmomu reviewed Apr 27, 2021

View reviewed changes

src/FieldDescription/BaseFieldDescription.php Outdated Show resolved Hide resolved

VincentLanglet dismissed jordisala1991’s stale review via f159240 April 27, 2021 07:50

VincentLanglet added 3 commits April 27, 2021 10:11

Only keep common options

4a451f5

Temporary fix

3ede327

Keep associated_property and link_parameters

5850a50

VincentLanglet requested review from dmaicher, franmomu, jordisala1991 and phansys April 27, 2021 08:47

VincentLanglet mentioned this pull request Apr 27, 2021

[PoC] Use options resolver in BaseFieldDescription #6078

Closed

2 tasks

franmomu approved these changes Apr 27, 2021

View reviewed changes

VincentLanglet merged commit 1e90e64 into sonata-project:3.x Apr 27, 2021

VincentLanglet deleted the removeOptions branch April 27, 2021 14:00

VincentLanglet added the patch label Apr 27, 2021

VincentLanglet mentioned this pull request May 1, 2021

Improve FieldDescription options. #4558

Closed

Uh oh!

Conversation

VincentLanglet commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Subject

Changelog

Uh oh!

franmomu commented Apr 22, 2021

Uh oh!

jordisala1991 commented Apr 22, 2021

Uh oh!

franmomu commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jordisala1991 commented Apr 22, 2021

Uh oh!

VincentLanglet commented Apr 22, 2021

Uh oh!

VincentLanglet commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jordisala1991 Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

jordisala1991 Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

jordisala1991 Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Apr 26, 2021 • 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!

SonataCI commented Apr 26, 2021

Uh oh!

VincentLanglet commented Apr 26, 2021

Uh oh!

VincentLanglet commented Apr 27, 2021

Uh oh!

franmomu Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

franmomu Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

VincentLanglet commented Apr 22, 2021 •

edited

Loading

franmomu commented Apr 22, 2021 •

edited

Loading

VincentLanglet commented Apr 22, 2021 •

edited

Loading

VincentLanglet Apr 26, 2021 •

edited

Loading