Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose field type constant and update documentation #6123

Merged
merged 1 commit into from May 30, 2020

Conversation

VincentLanglet
Copy link
Member

@VincentLanglet VincentLanglet commented May 29, 2020

Subject

I am targeting this branch, because BC.

This allow to be used in project or others bundle.
Maybe these constant are better somewhere else, you'll say.

This will avoid using non-existent field type, like
https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Guesser/TypeGuesser.php#L36
https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Guesser/TypeGuesser.php#L68

I also updated lot of documentation.

Changelog

### Added
- Added `TemplateRegistry::TYPE_*` constant to be used instead of string value.
- Added `format` option for `time` field type.

### Deprecated
- Deprecated `smallint` type for template ; use `integer` instead.
- Deprecated `bigint` type for template ; use `integer` instead.
- Deprecated `decimal` type for template ; use `float` instead.
- Deprecated `text` type for template ; use `string` instead.

@VincentLanglet
Copy link
Member Author

@greg0ire I added a note

UPGRADE-3.x.md Outdated Show resolved Hide resolved
UPGRADE-3.x.md Outdated Show resolved Hide resolved
UPGRADE-3.x.md Outdated Show resolved Hide resolved
UPGRADE-3.x.md Outdated Show resolved Hide resolved
greg0ire
greg0ire previously approved these changes May 30, 2020
@VincentLanglet VincentLanglet requested a review from a team May 30, 2020 10:37
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Maybe these constant are better somewhere else, you'll say.

What about create something like ShowMapperInterface for these constants?

UPGRADE-3.x.md Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Member Author

Maybe these constant are better somewhere else, you'll say.

What about create something like ShowMapperInterface for these constants?

These constant will be both used for the showMapper and the listMapper. I don't think creating two string constants (and so on) in two differents classes would be benefit.

For SHOW and LIST, I think that type is mainly (only ?) used to select the right template. In this bundle, we currently used the constants only to register the available ones.

So I tried to use a single class, with template-context.
I also prefer to use a real class rather an interface, because if someone try to implement all our interface in a different way and use a different syntax, these templates may not exist anymore.

@phansys
Copy link
Member

phansys commented May 30, 2020

I understood. Just I think the templating concept is not the best choice here, but I really don't have any better option at first glance 🤔

@VincentLanglet
Copy link
Member Author

I understood. Just I think the templating concept is not the best choice here, but I really don't have any better option at first glance 🤔

Yeah for sure.

In FieldDescriptionInterface comment say:

Returns the field type, the type is a mandatory field as it used to select the correct template or the logic associated to the current FieldDescription object.

For List/show, type is used

  • Fore some 'actions' === $type or \in_array($type, ['actions', 'batch', 'select'], true) checks
  • For $this->builder->addField($this->list, $type, $fieldDescription, $this->admin) which is implemented in DoctrineORMAdminBundle for example, and only set the template.
  • For setting a class class="sonata-ba-list-field-header-{{ field_description.type }}

I see a big difference between filter and form in which type are real class string like StringFilter::class or DateType::class and between show and list in which type are just a keyword to select the right template.

In a dream world, maybe we should expose and use some StringShowType::class, StringListType::class, where a Type class would refer to a template. But that's so much refactoring and possible BC-break for not so much benefit.

Creating these constants seems to be the best benefit/work ratio.

@VincentLanglet
Copy link
Member Author

Do you still request change @phansys ? :)

UPGRADE-3.x.md Outdated Show resolved Hide resolved
UPGRADE-3.x.md Outdated Show resolved Hide resolved
UPGRADE-3.x.md Outdated Show resolved Hide resolved
UPGRADE-3.x.md Outdated Show resolved Hide resolved
phansys
phansys previously approved these changes May 30, 2020
UPGRADE-3.x.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants