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

Allow overriding of every part of Grid templates #9281

Merged
merged 11 commits into from Sep 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -50,3 +50,11 @@ services:
- "@=service('prestashop.adapter.legacy.context').getContext().employee?service('prestashop.core.admin.module.repository'):null"
tags:
- { name: twig.extension }

prestashop.bundle.twig.extension.column:
class: 'PrestaShopBundle\Twig\Extension\GridExtension'
arguments:
- '@twig'
- '@prestashop.static_cache.adapter'
tags:
- { name: twig.extension }
Expand Up @@ -26,13 +26,13 @@
{% if grid.filter_form|length > 1 %}
<tr class="column-filters {% if 0 == grid.data.records_total and grid.filters is empty %}d-none{% endif %}">
{% for column in grid.columns %}
<th>
<td>
{% if grid.column_filters[column.id] is defined %}
{% for filter_name in grid.column_filters[column.id] %}
{{ form_widget(grid.filter_form[filter_name]) }}
{% endfor %}
{% endif %}
</th>
</td>
{% endfor %}
</tr>
{% endif %}
Expand Up @@ -24,15 +24,9 @@
*#}

<tr class="column-headers">
{% set orderBy, orderWay = grid.sorting.order_by, grid.sorting.order_way %}

{% for column in grid.columns %}
<th scope="col">
{% if column.options.sortable is defined and column.options.sortable and grid.data.records_total > 0 %}
{{ ps.sortable_column_header(column.name, column.id, orderBy, orderWay) }}
{% else %}
{{ column.name }}
{% endif %}
{{ column_header(column, grid) }}
</th>
{% endfor %}
</tr>
Expand Up @@ -41,7 +41,7 @@
<tr>
{% for column in grid.columns %}
<td>
{{ include('@PrestaShop/Admin/Common/Grid/Columns/'~column.type~'.html.twig', {'grid': grid, 'column': column, 'record': record}) }}
{{ column_content(record, column, grid) }}
</td>
{% endfor %}
</tr>
Expand All @@ -51,7 +51,6 @@
{% endif %}
{% endblock %}
</tbody>

{% block grid_table_footer %}{% endblock %}
</table>
{{ form_end(grid.filter_form) }}
@@ -0,0 +1,34 @@
{#**
* 2007-2018 PrestaShop
*
* NOTICE OF LICENSE
*
* This source file is subject to the Open Software License (OSL 3.0)
* that is bundled with this package in the file LICENSE.txt.
* It is also available through the world-wide-web at this URL:
* https://opensource.org/licenses/OSL-3.0
* If you did not receive a copy of the license and are unable to
* obtain it through the world-wide-web, please send an email
* to license@prestashop.com so we can send you a copy immediately.
*
* DISCLAIMER
*
* Do not edit or add to this file if you wish to upgrade PrestaShop to newer
* versions in the future. If you wish to customize PrestaShop for your
* needs please refer to http://www.prestashop.com for more information.
*
* @author PrestaShop SA <contact@prestashop.com>
* @copyright 2007-2018 PrestaShop SA
* @license https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
* International Registered Trademark & Property of PrestaShop SA
*#}

{% import '@PrestaShop/Admin/macros.html.twig' as ps %}

{% set orderBy, orderWay = grid.sorting.order_by, grid.sorting.order_way %}

{% if column.options.sortable is defined and column.options.sortable and grid.data.records_total > 0 %}
{{ ps.sortable_column_header(column.name, column.id, orderBy, orderWay) }}
{% else %}
{{ column.name }}
{% endif %}
199 changes: 199 additions & 0 deletions src/PrestaShopBundle/Twig/Extension/GridExtension.php
@@ -0,0 +1,199 @@
<?php
/**
* 2007-2018 PrestaShop.
*
* NOTICE OF LICENSE
*
* This source file is subject to the Open Software License (OSL 3.0)
* that is bundled with this package in the file LICENSE.txt.
* It is also available through the world-wide-web at this URL:
* https://opensource.org/licenses/OSL-3.0
* If you did not receive a copy of the license and are unable to
* obtain it through the world-wide-web, please send an email
* to license@prestashop.com so we can send you a copy immediately.
*
* DISCLAIMER
*
* Do not edit or add to this file if you wish to upgrade PrestaShop to newer
* versions in the future. If you wish to customize PrestaShop for your
* needs please refer to http://www.prestashop.com for more information.
*
* @author PrestaShop SA <contact@prestashop.com>
* @copyright 2007-2018 PrestaShop SA
* @license https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
* International Registered Trademark & Property of PrestaShop SA
*/

namespace PrestaShopBundle\Twig\Extension;

use RuntimeException;
use Symfony\Component\Cache\Adapter\AdapterInterface;
use Twig\Environment;
use Twig\Extension\AbstractExtension;
use Twig_SimpleFunction as SimpleFunction;

/**
* Class GridExtension is responsible for providing grid helpers functions.
*
* - column_content(column, record, grid): renders column content based on column type.
* - column_header(column, grid): renders column header based on column type.
*/
class GridExtension extends AbstractExtension
{
const BASE_COLUMN_CONTENT_TEMPLATE_PATH = '@PrestaShop/Admin/Common/Grid/Columns/Content';
const BASE_COLUMN_HEADER_TEMPLATE_PATH = '@PrestaShop/Admin/Common/Grid/Columns/Header/Content';

/**
* @var Environment
*/
private $twig;

/**
* @var AdapterInterface
*/
private $cache;

/**
* @param Environment $twig
* @param AdapterInterface $cache
*/
public function __construct(Environment $twig, AdapterInterface $cache)
{
$this->twig = $twig;
$this->cache = $cache;
}

/**
* {@inheritdoc}
*/
public function getFunctions()
{
return [
new SimpleFunction('column_content', [$this, 'renderColumnContent'], [
'is_safe' => ['html'],
]),
new SimpleFunction('column_header', [$this, 'renderColumnHeader'], [
'is_safe' => ['html'],
]),
];
}

/**
* Render column content.
*
* @param array $record
* @param array $column
* @param array $grid
*
* @return string
*
* @throws RuntimeException when template cannot be found for column
*/
public function renderColumnContent(array $record, array $column, array $grid)
{
$templateCacheKey = sprintf('column_%s_%s_%s_content', $grid['id'], $column['id'], $column['type']);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding defensive validation here ?

$this->validateInput($record, $column, $grid);

private function validateInput(array $record, array $column, array $grid)
{
    $this->arrayMustContains($grid, 'id');
    $this->arrayMustContains($column, ['id', 'type]);
}

// might be an util function so could come from another class
/**
 * @param array $array
 * @param string|string[] $keys
 *
 * @throws \InvalidArgumentException
 */
protected function arrayMustContains(array $array, $keys)
{
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i guess we could do that, but if you pass wrong data, you will instantly that your grid is broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want the validation done directly in the class (this breaks SRP).

Note that we have complete control on grid, column, and record (see GridPresenter class).

So: we inject a Validator (or an options resolver) or we do that later, your decision @matks !

Copy link
Contributor

Choose a reason for hiding this comment

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

We do that later :)


if (false === $this->cache->hasItem($templateCacheKey)) {
$template = $this->getTemplatePath(
$column,
$grid,
self::BASE_COLUMN_CONTENT_TEMPLATE_PATH
);

if (null === $template) {
throw new RuntimeException(
sprintf('Content template for column type "%s" was not found', $column['type'])
);
}

$this->cache->save(
$this->cache
->getItem($templateCacheKey)
->set($template)
);
}

return $this->twig->render($this->cache->getItem($templateCacheKey)->get(), [
'column' => $column,
'record' => $record,
'grid' => $grid,
]);
}

/**
* Render column header.
*
* @param array $column
* @param array $grid
*
* @return string
*/
public function renderColumnHeader(array $column, array $grid)
{
$templateCacheKey = sprintf(
'column_%s_%s_%s_header',
$grid['id'],
$column['id'],
$column['type']
);

if (!$this->cache->hasItem($templateCacheKey)) {
$template = $this->getTemplatePath(
$column,
$grid,
self::BASE_COLUMN_HEADER_TEMPLATE_PATH,
'default.html.twig'
);

$this->cache->save(
$this->cache
->getItem($templateCacheKey)
->set($template)
);
}

return $this->twig->render($this->cache->getItem($templateCacheKey)->get(), [
'column' => $column,
'grid' => $grid,
]);
}

/**
* Get template for column.
*
* @param array $column
* @param array $grid
* @param string $basePath
* @param string|null $defaultTemplate
*
* @return string|null
*/
private function getTemplatePath(array $column, array $grid, $basePath, $defaultTemplate = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a function that could be useful in other Twig extensions. Maybe we could move it into a class like GridExtensionUtils ?
Wdyt ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, but later: we really need this pull request to be merged today. Refactoring can happens later, as it's a private function this won't change the public API :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, let's get this merged ! :goberserk:

{
$gridId = $grid['id'];
$columnId = $column['id'];
$columnType = $column['type'];

$columnGridTemplate = sprintf('%s/%s_%s_%s.html.twig', $basePath, $gridId, $columnId, $columnType);
$gridTemplate = sprintf('%s/%s_%s.html.twig', $basePath, $gridId, $columnType);
$columnTemplate = sprintf('%s/%s.html.twig', $basePath, $columnType);

if ($this->twig->getLoader()->exists($columnGridTemplate)) {
return $columnGridTemplate;
}

if ($this->twig->getLoader()->exists($gridTemplate)) {
return $gridTemplate;
}

if ($this->twig->getLoader()->exists($columnTemplate)) {
return $columnTemplate;
}

if (null !== $defaultTemplate) {
return sprintf('%s/%s', $basePath, $defaultTemplate);
}

return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

@mickaelandrieu did'nt you say that you wanted that we stop returning null and throw exceptions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a private function, you can see that exception is thrown above. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

still we could return '' or throw the exception here ahahah

}
}