-
Notifications
You must be signed in to change notification settings - Fork 341
Description
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_column_type_%column_type%_cell or use one of these following blocks.
They are called before the default block.grid_%id%column_id%column_id%cell
grid%id%column_type%column_type%cell
grid%id%column_type%column_parent_type%cell
grid_column_id%column_id%cell
grid_column_type%column_type%cell
grid_column_type%column_parent_type%_cellNote 1: It is also possible to name blocks using ...column... instead of ...column_id... and ...column_type.... However this naming convention is not advised as it is ambiguous. It is only supported for backward compatibility.
As said in "Note 1" the "grid_column_%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:
APYDataGridBundle/Twig/DataGridExtension.php
Lines 245 to 256 in 91b3f7f
if (($id != '' && ($this->hasBlock($environment, $block = 'grid_' . $id . '_column_' . $column->getRenderBlockId() . '_cell') | |
|| $this->hasBlock($environment, $block = 'grid_' . $id . '_column_' . $column->getType() . '_cell') | |
|| $this->hasBlock($environment, $block = 'grid_' . $id . '_column_' . $column->getParentType() . '_cell') | |
|| $this->hasBlock($environment, $block = 'grid_' . $id . '_column_id_' . $column->getRenderBlockId() . '_cell') | |
|| $this->hasBlock($environment, $block = 'grid_' . $id . '_column_type_' . $column->getType() . '_cell') | |
|| $this->hasBlock($environment, $block = 'grid_' . $id . '_column_type_' . $column->getParentType() . '_cell'))) | |
|| $this->hasBlock($environment, $block = 'grid_column_' . $column->getRenderBlockId() . '_cell') | |
|| $this->hasBlock($environment, $block = 'grid_column_' . $column->getType() . '_cell') | |
|| $this->hasBlock($environment, $block = 'grid_column_' . $column->getParentType() . '_cell') | |
|| $this->hasBlock($environment, $block = 'grid_column_id_' . $column->getRenderBlockId() . '_cell') | |
|| $this->hasBlock($environment, $block = 'grid_column_type_' . $column->getType() . '_cell') | |
|| $this->hasBlock($environment, $block = 'grid_column_type_' . $column->getParentType() . '_cell') |
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.