APY / APYDataGridBundle

Symfony Datagrid Bundle
MIT License
492 stars 344 forks source link

DataGridExtension improvements #986

Open plfort opened 6 years ago

plfort commented 6 years ago

Hi @APY/collaborators , I want to propose some improvements to the DataGridExtension, this is why I added tests with #983 PR, I want to test the current behavior before working on it.

First, the Twig blocks order for cell and filter is inconsistent: Quote from https://github.com/APY/APYDataGridBundle/blob/master/Resources/doc/template/cell_rendering.md#overriding-block-names-ordered

Overriding block names (ordered)

You can override the default block grid_columntype%column_type%_cell or use one of these following blocks. They are called before the default block.

grid_%id%_columnid%column_id%cell grid%id%_columntype%column_type%cell grid%id%_columntype%column_parent_type%_cell grid_columnid%column_id%_cell grid_columntype%column_type%_cell grid_columntype%column_parent_type%_cell

Note 1: It is also possible to name blocks using ...column... instead of ..._columnid... and ..._columntype.... However this naming convention is not advised as it is ambiguous. It is only supported for backward compatibility.

As said in "Note 1" the "gridcolumn%id|type%_cell" notation is "not advised". This notation is less discriminative than "grid_column_id..." and "grid_column_type..." but it is tested first in getGridCell and getGridFilter: https://github.com/APY/APYDataGridBundle/blob/91b3f7f312c8994771b844318932aa5c087a9113/Twig/DataGridExtension.php#L245-L256

My proposal is to trigger a deprecation when this block name is used, and then we can plan to remove it.

Second, the column's getParentType method is totally useless: it returns an empty string and is never used. Moreover with this logic we can defined a block named "grid_column__cell" and it will be used for all columns. We should remove this method.

Finally, we should not rely on the internal class Twig_Template for template rendering but on Twig_TemplateWrapper.