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

Registered all namespaces in ModuleTemplateLoader class #9173

Merged
merged 5 commits into from Jun 12, 2018

Conversation

Projects
None yet
6 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Jun 7, 2018

Questions Answers
Branch? 1.7.4.x
Description? We were unable to override each template of Back Office from a module because of only the namespace PrestaShop was registered.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
How to test? In any module already registered, create a new template called products_table.html.twig in modules/your-module/views/PrestaShop/Admin/Product/CatalogPage/Lists/products_table.html.twig with empty content. You should see no products anymore in your products catalog page

Important guidelines


This change is Reviewable

@prestonBot prestonBot added the 1.7.4.x label Jun 7, 2018

@mickaelandrieu mickaelandrieu added the Bug label Jun 7, 2018

*/
public function setRegisteredPaths($registeredPaths)
{
dump($registeredPaths);

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 7, 2018

Contributor

Nope

This comment has been minimized.

@mickaelandrieu
$templatePaths[] = $dir;
}
}
$this->setPaths($templatePaths, $namespace);
}
}
/**
* @return array

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 7, 2018

Contributor

Why add this two functions? Where are they used?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 7, 2018

Contributor

some tests, removed 👍

@PierreRambaud

See comments

<tr><td colspan="11">
{% endblock %}
{% else %}
<tr><td colspan="11">

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 7, 2018

Contributor

Weird indent

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 7, 2018

Contributor

was here previously, improving it now may make the contribution unreadable

"icon": "remove_red_eye",
"label": "Preview"|trans({}, 'Admin.Actions')
}
{

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 7, 2018

Contributor

👍

{% if activate_drag_and_drop and has_category_filter %}class="sortable"{% endif %}
last_sql="{{ last_sql_query|escape('html_attr') }}"
{% if activate_drag_and_drop and has_category_filter %}class="sortable"{% endif %}
last_sql="{{ last_sql_query|escape('html_attr') }}"

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 7, 2018

Contributor

Maybe should be a data- attribute?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 7, 2018

Contributor

nope! we dont change anything here: not the scope of this contribution.

HALTE AU GRABUGE !

'Product': '/Admin/Product'
'Twig': '/Admin/TwigTemplateForm'
'AdvancedParameters': '/Admin/Configure/AdvancedParameters'
'ShopParameters': '/Admin/Configure/ShopParameters'

This comment has been minimized.

@eternoendless

eternoendless Jun 7, 2018

Member

Why not store this list elsewhere, like in a config or something?

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 7, 2018

Contributor

It's used just once, not sure it's necessary

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 7, 2018

Contributor

because this won't be re-used.

At start, I wanted to re-use the paths used in framework configuration, but thinking about it it's not a good idea. The 'array' look like the same but they have really different purposes.

If we have another place/service when we want to re-use it, of course I'm also in favor of extracting it into a parameter.

Note you can access to all registered Twig paths and namespaces using php bin/console debug:twig but not us, cause of weird call of Legacy Context in a Twig extension. This is a really minor issue we should try to fix asap.

This comment has been minimized.

@eternoendless

eternoendless Jun 7, 2018

Member

But we'll need to add new namespaces as we migrate new sections, won't we?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 7, 2018

Contributor

yes, so we will append new namespaces here.

But before I need to document which namespaces should be added as there is no real consistency actually: some templates use them, others don't, some namespace refers to a page, others to sections or even categories...

Once we will have consistency, I'm pretty sure we won't even need to configure an Array, but maybe loop into actual folders and declare namespaces dynamically ^^

This comment has been minimized.

@eternoendless

eternoendless Jun 8, 2018

Member

As long as it isn't done in runtime...

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 9, 2018

Contributor

array
nope, it's converted in PHP function in container cache: I don't think we can make the process faster 👍

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 9, 2018

Contributor

yes everything setting in yml files are automatically converted in string / array in the cache file =)

mickaelandrieu added some commits Jun 7, 2018

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 12, 2018

@PierreRambaud LGTM? ;)

@PierreRambaud PierreRambaud merged commit 1c6d820 into PrestaShop:1.7.4.x Jun 12, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu deleted the mickaelandrieu:improved-module-template-loader branch Jun 13, 2018

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 13, 2018

@matks matks added the migration label Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment