Manual merge by franmomu · Pull Request #7099 · sonata-project/SonataAdminBundle

franmomu · 2021-04-21T07:42:32Z

No description provided.

* 3.97 * Update CHANGELOG.md Co-authored-by: Javier Spagnoletti <phansys@gmail.com> * Upgrade note Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Instead of passing `breadcrumbs_builder` parameter to all templates, it uses `render_breadcrumbs` and `render_breadcrumbs_for_title` twig functions. For keeping BC, these functions accept the breadcrumbs builder as last argument.

assets/scss/layout.scss

franmomu · 2021-04-21T07:54:17Z

src/FieldDescription/FieldDescriptionInterface.php

 *  type?: string,
 *  virtual_field?: bool
- * }&array<string, mixed>|array<empty, empty>
+ * }|array<empty, empty>


I don't know if it's de proper fix, but phpstan does not complain

This is not valid IMHO, and we'll get some others errors.
Any option should be valid.

For instance, in my code, I'm using

->add('clients', null, [ 'template' => 'admin/sonata/list/contract_clients.html.twig', 'link' => 'admin_assurancevie_client_edit', ])

Maybe the issue should be reported to phpstan

VincentLanglet · 2021-04-21T09:50:18Z

src/FieldDescription/FieldDescriptionInterface.php

 *  type?: string,
 *  virtual_field?: bool
- * }&array<string, mixed>|array<empty, empty>
+ * }|array<empty, empty>


This is not valid IMHO, and we'll get some others errors.
Any option should be valid.

For instance, in my code, I'm using

->add('clients', null, [ 'template' => 'admin/sonata/list/contract_clients.html.twig', 'link' => 'admin_assurancevie_client_edit', ])

franmomu · 2021-04-21T10:14:00Z

Why any option should be valid? I think we should restrict the options, otherwise (as it's happening now) it is impossible to control, in fact there was a starting PR to fix this: #6078

VincentLanglet · 2021-04-21T10:24:44Z

Why any option should be valid? I think we should restrict the options, otherwise (as it's happening now) it is impossible to control, in fact there was a starting PR to fix this: #6078

I think this is not a good idea. In fact, I think we should remove this custom type and prefer array<string, mixed>.

Restricting the option in the interface FieldDescriptionInterface, it's like restricting the option in the FormTypeInterface.
The options resolver is specific to a FormType. Each FormType has custom options.
It's the same here: each FieldDescription could have custom option.

For instance the virtual_field options is used by the BaseFieldDescription, but I could write a different FieldDescription which use a different options.
And when using a custom template, options are passed to the template, so it's a way to pass special values to the view, so basically any value could be possible.

franmomu · 2021-04-21T11:05:39Z

I think this is not a good idea. In fact, I think we should remove this custom type and prefer array<string, mixed>.

IMO that would be a mistake.

Restricting the option in the interface FieldDescriptionInterface, it's like restricting the option in the FormTypeInterface.
The options resolver is specific to a FormType. Each FormType has custom options.
It's the same here: each FieldDescription could have custom option.

I think it's quite different, by general, there is only one or two implementations (one per persistence bundle) of FieldDescriptionInterface, you don't create an implementation of FieldDescriptionInterface for a custom field.

The options, as stated in the code, are dependent of the type of the context.

Something I had in mind since a long time (I think I wrote about it somewhere) is to create classes for the list and show context, but that's another story.

For instance the virtual_field options is used by the BaseFieldDescription, but I could write a different FieldDescription which use a different options.
And when using a custom template, options are passed to the template, so it's a way to pass special values to the view, so basically any value could be possible.

For passing values to the template there should be an option for that, to use options that way it's just a hack and I think there is no documentation about that.

But as this is going nowhere, best if @sonata-project/contributors decide.

VincentLanglet · 2021-04-21T12:01:33Z

Restricting the option in the interface FieldDescriptionInterface, it's like restricting the option in the FormTypeInterface.
The options resolver is specific to a FormType. Each FormType has custom options.
It's the same here: each FieldDescription could have custom option.

I think it's quite different, by general, there is only one or two implementations (one per persistence bundle) of FieldDescriptionInterface, you don't create an implementation of FieldDescriptionInterface for a custom field.

Yes, but someone could use his own persistence or his own FieldDescription with more features (and options).
This should be allowed.

The options, as stated in the code, are dependent of the type of the context.

Some options are specific to only some FieldDescription type ; for instance timezone, which is only used for date values.

SonataAdminBundle/src/FieldDescription/FieldDescriptionInterface.php

Line 52 in 4b1a41d

* timezone?: string|\DateTimeZone,

or currency

SonataAdminBundle/src/Resources/views/CRUD/display_currency.html.twig

Line 16 in 2c07756

which only related to the TYPE_CURRENCY and is not in the FieldDescriptionOptions currently.

another example inverse:

SonataAdminBundle/src/Resources/views/CRUD/display_boolean.html.twig

Line 19 in 2c07756

{% if field_description.options.inverse|default(false) ? not value : value %}

format:

SonataAdminBundle/src/Resources/views/CRUD/display_time.html.twig

Line 17 in 2c07756

    
           {{ value|date(field_description.options.format|default('H:i:s'), field_description.options.timezone|default(null)) }}

I'm sure I can find others options used for specific FieldDescription types.
And since we're allowing custom types, we shouldn't restrict the options for them.

And when using a custom template, options are passed to the template, so it's a way to pass special values to the view, so basically any value could be possible.

For passing values to the template there should be an option for that, to use options that way it's just a hack and I think there is no documentation about that.

I just follow the way we're doing for all the existing templates.

VincentLanglet · 2021-04-21T23:02:45Z

tests/Admin/AdminTest.php

        $perPageOptions = $admin->getPerPageOptions();

-        $this->assertSame([10, 25, 50, 100, 250], $perPageOptions);
+        $this->assertSame([16, 32, 64, 128, 256], $perPageOptions);


We should update the two constants of the AbstractAdmin instead of the tests.

This was the values of our persistent bundle
https://github.com/sonata-project/SonataDoctrineMongoDBAdminBundle/blob/c18eb59881d7ebd7aeca92de9bf9e74c0e812090/src/Model/ModelManager.php#L613
https://github.com/sonata-project/SonataDoctrineMongoDBAdminBundle/blob/c18eb59881d7ebd7aeca92de9bf9e74c0e812090/src/Model/ModelManager.php#L629

The constants are private.

The values of those method were never used because are overridden in

SonataAdminBundle/src/Admin/AbstractAdmin.php

Lines 700 to 709 in a8731dc

final public function getDefaultFilterParameters(): array

{

return array_merge(

/* @phpstan-ignore-next-line */

$this->getModelManager()->getDefaultSortValues($this->getClass(), 'sonata_deprecation_mute'), // NEXT_MAJOR: Remove this line.

$this->datagridValues, // NEXT_MAJOR: Remove this line.

$this->getDefaultSortValues(),

$this->getDefaultFilterValues()

);

}

The values of those method were never used because are overridden in

SonataAdminBundle/src/Admin/AbstractAdmin.php

Lines 700 to 709 in a8731dc

final public function getDefaultFilterParameters(): array

{

return array_merge(

/* @phpstan-ignore-next-line */

$this->getModelManager()->getDefaultSortValues($this->getClass(), 'sonata_deprecation_mute'), // NEXT_MAJOR: Remove this line.

$this->datagridValues, // NEXT_MAJOR: Remove this line.

$this->getDefaultSortValues(),

$this->getDefaultFilterValues()

);

}

I remember that in some issue "business" values were preferred over "binary" values.
They were meant to be used, that's why it was fixed in the master branch in the same time that the deprecation of getDefaultSortValues.
Currently the values in master are [10, 25, 50, 100, 250], the introduction of a constant to not use "magic numbers" shouldn't change the values.

Well, to be fair, in master tests were changed here, but I don't find any comment about that:

0353882#diff-988f567ac1553662391a6e5079ce038f8f58cf2f1e7aafba378ce38eb2b9aaecR718-R728

Comments were added in the AbstractAdmin https://github.com/sonata-project/SonataAdminBundle/pull/7102/files#diff-0850515af24b81ce716f53b8eb0b71fae91dcbdcc88b28d95a83886504e1c8f4R2660-R2663 and maybe the comment were missing in the test, but there was updated accordingly to the value change in the AbstractAdmin.

This is why I posted #7102 (comment)
To add a NEXT_MAJOR comment here: #7102 (comment)

private const DEFAULT_LIST_PER_PAGE_RESULTS = 32; // NEXT_MAJOR: Change to 25 private const DEFAULT_LIST_PER_PAGE_OPTIONS = [16, 32, 64, 128, 256]; // NEXT_MAJOR: Change to [10, 25, 50, 100, 250]

I thought you said you'll modify them https://github.com/sonata-project/SonataAdminBundle/pull/7102/files#issuecomment-824092510

VincentLanglet · 2021-04-22T13:06:46Z

@franmomu This seems to be the way to fix the type https://phpstan.org/r/e3aa9a0c-66d3-4e80-a3a3-882d13fc14fc

( and ) should be added.

VincentLanglet and others added 4 commits April 19, 2021 12:41

3.97 (sonata-project#7091)

cc514b7

* 3.97 * Update CHANGELOG.md Co-authored-by: Javier Spagnoletti <phansys@gmail.com> * Upgrade note Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Fix matching filters links in search results

01677b2

Merge remote-tracking branch 'upstream/3.x'

a90b204

franmomu commented Apr 21, 2021

View reviewed changes

assets/scss/layout.scss Outdated Show resolved Hide resolved

franmomu commented Apr 21, 2021

View reviewed changes

VincentLanglet requested changes Apr 21, 2021

View reviewed changes

franmomu requested review from a team and jordisala1991 April 21, 2021 10:14

Fix parent selector

b0cd623

franmomu mentioned this pull request Apr 21, 2021

Fix phpstan field options #7101

Closed

franmomu force-pushed the master branch from fa63b5a to b0cd623 Compare April 21, 2021 12:01

Extract magic numbers

a8731dc

VincentLanglet reviewed Apr 21, 2021

View reviewed changes

franmomu added 3 commits April 22, 2021 23:39

Import from proper location

af14cc4

Fix phpstan

9350386

Merge remote-tracking branch 'upstream/3.x'

e6976f2

franmomu force-pushed the master branch from fbbba1b to e6976f2 Compare April 22, 2021 21:48

franmomu requested a review from a team April 22, 2021 21:49

VincentLanglet approved these changes Apr 22, 2021

View reviewed changes

franmomu merged commit e6976f2 into sonata-project:master Apr 22, 2021

franmomu deleted the master branch April 22, 2021 22:01

This was referenced Apr 27, 2021

[PoC] Use options resolver in BaseFieldDescription #6078

Closed

Improve FieldDescription options. #4558

Closed

VincentLanglet added the pedantic label May 2, 2021

	final public function getDefaultFilterParameters(): array
	{
	return array_merge(
	/* @phpstan-ignore-next-line */
	$this->getModelManager()->getDefaultSortValues($this->getClass(), 'sonata_deprecation_mute'), // NEXT_MAJOR: Remove this line.
	$this->datagridValues, // NEXT_MAJOR: Remove this line.
	$this->getDefaultSortValues(),
	$this->getDefaultFilterValues()
	);
	}

Uh oh!

Conversation

franmomu commented Apr 21, 2021

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!

franmomu commented Apr 21, 2021

Uh oh!

VincentLanglet commented Apr 21, 2021

Uh oh!

franmomu commented Apr 21, 2021

Uh oh!

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

VincentLanglet commented Apr 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

VincentLanglet commented Apr 21, 2021 •

edited

Loading