Skip to content

Added syntax for obtaining the original bundle #2202

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Added syntax for obtaining the original bundle #2202

wants to merge 3 commits into from

Conversation

hason
Copy link
Contributor

@hason hason commented Sep 17, 2011

One of the possible ways to solve #1966

@lsmith77
Copy link
Contributor

I think its good to address this missing feature. I am not sure I like the syntax as ! to me is too closely associated with negation. But this is of course only a minor though important detail.

henrikbjorn
henrikbjorn reviewed Sep 19, 2011
View reviewed changes
src/Symfony/Component/HttpKernel/Kernel.php
Copy link
Contributor

Choose a reason for hiding this comment

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

Should proberly be next instead of end.

@henrikbjorn
Copy link
Contributor

As noted in the comment on you diff i think it should take the next match and not the last match. This would make more sense and not ignore kill the cascading lookup.

@lsmith77
Copy link
Contributor

If its next then maybe a git-sh ^ would work? aka git reset HEAD^

@hason
Copy link
Contributor Author

hason commented Sep 19, 2011

@henrikbjorn I think that next is too magic and correct implementation is complicated. Consider the case:

    app/Resources/FooBarBundle/index.html.twig
    bundles/FooBarBundle/Resources/index.html.twig  // FooBarBundle getParent === FooBundle
    bundles/FooBundle/Resources/index.html.twig

Each template extends the previous template:

{# app/Resources/FooBarBundle/index.html.twig #}
{% extends "^FooBundle::index.html.twig" %}
---
{# bundles/FooBarBundle/Resources/index.html.twig #}
{% extends "^FooBundle::index.html.twig" %} {# next() return wrong bundle #}
---
{# bundles/FooBundle/Resources/index.html.twig #}
<html>

The simplest and explicit solution is via syntax for original bundle:

{# app/Resources/FooBarBundle/index.html.twig #}
{% extends ["!FooBarBundle::index.html.twig", "!FooBundle::index.html.twig"] %}
---
{# bundles/FooBarBundle/Resources/index.html.twig #}
{% extends "!FooBundle::index.html.twig" %}
---
{# bundles/FooBundle/Resources/index.html.twig #}
<html>

@henrikbjorn
Copy link
Contributor

It shouldnt change the bundle then. Just jump to the next path in the list. {# bundles/FooBarBundle/Resources/index.html.twig #} would be able to extend its parent if unless it is an extending bundle.

@schmittjoh
Copy link
Contributor

Some ideas for notation: @!FooBundle or @FooBundle!

@ghost
Copy link

ghost commented Sep 19, 2011

or just :FooBarBundle::index.html.twig

@stof
Copy link
Member

stof commented Sep 19, 2011

: will cause an issue as it is already the separator

@Richtermeister
Copy link
Contributor

how about ..FooBarBundle::index.html.twig?
I know that's 2 characters, but it mimics the "previous directory" syntax from file systems..

@HelloGrayson
Copy link

( ^3^)/FooBundle anyone?

@jmikola
Copy link
Contributor

jmikola commented Sep 29, 2011

I'm a fan of using one character to get to the end of the inheritance tree (to the discussion of end() vs. next()) above. I don't think there's a big use case for extending a template in the middle of the chain - that would require at least four templates in the entire chain to be worthwhile, I believe. Otherwise, if you had three templates, the normal extension behavior (from the child-most template) would target the parent instead of grandparent.

Regarding syntax, ! reminds me CSS's !important syntax, which is about overriding in a different direction. My vote is for ^ as a single-character prefix.

@fabpot
Copy link
Member

fabpot commented Oct 6, 2011

During today's IRC meeting, it has been decided to not add this feature as we don't have that many use cases and it adds some non-trivial complexity. I've added a doc ticket to update documentation about how to solve the issue described here (symfony/symfony-docs#752).

@fabpot fabpot closed this Oct 6, 2011
@helmer
Copy link
Contributor

helmer commented Oct 7, 2011

@fabpot I would say there are very many use cases for this feature, basically every bundle which defines an "entrypoint" template (by entrypoint I mean a template which is rendered directly) is a subject to this feature. Introducing a one-liner side-template is an overkill, imo.

What do you mean under increased complexity? That it is hard to understand for a newcomer (can be easily taken care of with additional documentation under template inheritance) or that it adds an overhead to locating resources (haven't benchmarked but I doubt it's significant)?

I encourage to re-address this issue, seems like a should-have to me ..

@lsmith77
Copy link
Contributor

lsmith77 commented Oct 7, 2011

The issue is that if we do introduce such a syntax for the "simple case", we also need to ensure that it remains consistent for the "complex case".

The simple case:
There is a template and someone wants to extend it with some stuff.

Here its obvious which template is meant when referencing the previous one, since there is only one.

The complex case:
Someone wants to extend a template that some other bundle has already extended.

Now does the person mean to reference the original or the previous extended template? As you can imagine it can get more complex if the chain consists of 3 or more templates.

@helmer
Copy link
Contributor

helmer commented Oct 7, 2011

Well, just because we haven't yet found a nice solution for complex scenarios does not mean we should discard the feature altogether.

Your own proposed git-style caret seems already a good start. Put as many as you'd like and as many steps ahead the lookup is done. While I agree, that it might be a bit fragile in case intermediary layers are introduced, that is a drawback we need to find a solution for or just accept.

Another idea just off the top of my head, which I think solves the fragility mentioned above. How about prefixing the caret with desired bundle name:

  1. Bundle inheritance tree is MyBundle -> FooBundle -> BarBundle -> SuperBundle;
  2. All bundles have index.html.twig, extending similarly to the bundle inheritance tree;
  3. Wanting to cut out FooBundle overrides, you'd write: {% extends BarBundle^SuperBundle:index.html.twig %} in your own index.html.twig.

@hason hason mentioned this pull request Feb 9, 2014
Closed
@hason hason deleted the originalbundle branch June 27, 2014 07:31
@marijnotte
Copy link

It's now a few years later and the use composer with vendors bundles a lot. We currently don't have a way to extend one block of a vendor template (even when we are the vendor ourselfes). We always have to copy the complete template, if we want to add something, for example an extra information block in a CMS for a specific website.

So maybe it's time to reopen this issue and provide a way to extend a vendor template?

@nick4fake
Copy link

+1 for reopening. @fabpot Could you check this again? There are LOTS of usecases when this is needed. For example, we override many templates of SonataAdmin. Without this, we should copy every single file and override few lines.

@boekkooi
Copy link
Contributor

@marijnotte and @nick4fake Take a look at the TemplateNameParser in boekkooi/twig-jack-bundle

@marijnotte
Copy link

@boekkooi thank you very much, this is exactly what we need

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

Reviewers

No reviews

Assignees

No one assigned

Labels

HttpKernel

Projects

None yet

Milestone

2.1

Development

Successfully merging this pull request may close these issues.