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 themes to inherit module templates #1174

Merged

Conversation

@ghalse
Copy link
Contributor

commented Jul 30, 2019

When developing themes it is common to want to make only minor cosmetic changes to the existing template, such as adding additional CSS. Under the old theming system it was possible to inherit the stock theme and alter it by simply include()ing the PHP.

Similar functionality doesn't exist in Twig, but arguably makes more sense here where we can now override a single block. This functionality is already used by themes to override the default theme, but the model doesn't extend to modules. Thus this commit introduces a special __parent__ namespace in Twig to allow theme developers to reference the original module templates from within their own theme's template.

E.g if I wanted to inheret the entire discopower module's template save for injecting one additional script into it, rather than copying the whole disco.twig template to my theme and editing it, I can merely override the postload block by creating a new mymodule/themes/mytheme/discopower/disco.twig like this:

{% extends "@__parent__/disco.twig" %}
{% block postload %}
{{ parent() }}{# include the paren't postload block #}
<!-- this is all we're changing -->
<script src="/path/to/extra/javascript.js"></script>
{% endblock %}

That's a whole lot less maintenance for me as a theme developer when the module's template changes.

@tvdijen

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Hey Guy,

How is this different from the already existing functionality?
I think the exact same thing you're proposing is demonstrated here:
https://github.com/simplesamlphp/simplesamlphp/blob/simplesamlphp-1.17/modules/admin/templates/authsource_list.twig

@ghalse

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

I think the exact same thing you're proposing is demonstrated here:
https://github.com/simplesamlphp/simplesamlphp/blob/simplesamlphp-1.17/modules/admin/templates/authsource_list.twig

That's not quite the same thing - it is including content from within the same module and with a different name. That namespace, as you correctly point out, already exists as @admin and can be used as such.

To replicate the problem I am trying to solve with that example you would need to theme the admin module such that you had a file:

modules/mymodule/themes/mytheme/admin/authsource_list.twig

being a theme that replaces:

modules/admin/authsource_list.twig

from your theme. In your theme, you then try and include the original modules/admin/authsource_list.twig so that you can extend/alter a small part of it (e.g. adding a postload block) without copying the whole original file. Logically something like this:

{# this is modules/mymodule/themes/mytheme/admin/authsource_list.twig #} 
{% extends "@admin/authsource_list.twig" %}{# <-- this wants modules/admin/authsource_list.twig #}
{% block postload %}
{{ parent() }}
<script src="/path/to/extra/javascript.js"></script>
{% endblock %}

However, that doesn't work and creates a circular reference back to modules/mymodule/themes/mytheme/admin/authsource_list.twig. The reason is that, in this situation, the @admin/authsource_list.twig entry searches modules/mymodule/themes/mytheme/admin first and finds the authsource_list.twig there, and so never tries the original module's template. There is no currently-existant namespace that allows me to access modules/admin/authsource_list.twig.

@tvdijen

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Make sense now! Thanks for the explanation!

@tvdijen

tvdijen approved these changes Aug 6, 2019

@tvdijen tvdijen added this to the 1.18 milestone Aug 6, 2019

@tvdijen tvdijen added the enhancement label Aug 6, 2019

@jaimeperez

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Thanks for the further explanation Guy! I was confused as well about this. It's indeed such a useful addition!

@jaimeperez jaimeperez merged commit caa16d3 into simplesamlphp:master Aug 6, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.003%) to 35.197%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.