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

[TwigBundle] Make twig extension handle custom template escaping guesser #7479

Merged
merged 1 commit into from
Apr 21, 2013

Conversation

maxbeutel
Copy link

I wanted to add a custom template escaping guesser for twig like in http://twig.sensiolabs.org/doc/recipes.html#using-the-template-name-to-set-the-default-escaping-strategy

This pull request allows you register a service id and a method as a custom guesser:

twig:
    autoescape: my_service:guess

Documentation is missing for this PR and the unit tests are not that good. I´d work on it if this PR has any chance of getting merged?

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR (missing)

@lstrojny
Copy link
Contributor

👏

@fabpot
Copy link
Member

fabpot commented Apr 1, 2013

Looks like a good addition to me.

I would use a notation that would respect how we reference services elsewhere:

twig:
    autoescape_service: @my_service
    autoescape_service_method: guess

Also, beside documentation, you should add a note in the component CHANGELOG describing the new feature.

@maxbeutel
Copy link
Author

I added the new config settings.

@maxbeutel
Copy link
Author

Added documentation, see above

@stof
Copy link
Member

stof commented Apr 3, 2013

@fabpot I would remove the @.

  • @ has no special meaning in the semantic config
  • @ is not used in XML
  • we would need to strip it before using
  • we don't use the @ in existing places allowing to configure a service id (in SecurityBundle for instance)

@maxbeutel
Copy link
Author

ping @fabpot any desicion on this? Currently its implemented that the @ is stripped.

Also another question: Is there any chance of getting this PR also backported to 2.1. or 2.2. ?

@canni
Copy link
Contributor

canni commented Apr 4, 2013

@maxbeutel no, there is no chance for this to be backported, only bug-fixes can be applied to older branches

IMO we could consider to structure this similarly to what we have (ie form fields type guessers) It would be great that guessers can be classes that implement common interface and are registered as a tagged services, and passed to something like a chained guesser, then each bundle will be able to easily define its own guessers (with possibly its own escaping rules) - for now this can be little overhead, but will save us enough playground and flexibility for future

@lstrojny
Copy link
Contributor

lstrojny commented Apr 4, 2013

@canni this sounds way too complicated for such a simple task.

@stof
Copy link
Member

stof commented Apr 5, 2013

@canni Twig expects a callable. The goal here is simply to define a callable as array($service, $method)

@lstrojny
Copy link
Contributor

lstrojny commented Apr 6, 2013

What about the @? Can we reach a final decision here and go on?

@fabpot
Copy link
Member

fabpot commented Apr 20, 2013

Let's remove the @ altogether.

@maxbeutel After removing the @, can you submit a PR on the documentation so that I can merge?

fabpot added a commit that referenced this pull request Apr 21, 2013
This PR was merged into the master branch.

Discussion
----------

[TwigBundle] Make twig extension handle custom template escaping guesser

I wanted to add a custom template escaping guesser for twig like in http://twig.sensiolabs.org/doc/recipes.html#using-the-template-name-to-set-the-default-escaping-strategy

This pull request allows you register a service id and a method as a custom guesser:

    twig:
        autoescape: my_service:guess

Documentation is missing for this PR and the unit tests are not that good. I´d work on it if this PR has any chance of getting merged?

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | (missing)

Commits
-------

c2c1ed0 make twig extension handle custom template escaping guesser
@fabpot fabpot merged commit c2c1ed0 into symfony:master Apr 21, 2013
@fabpot
Copy link
Member

fabpot commented Apr 21, 2013

I've made the needed changes and changed the XSD to use - instead of _ as a word separator.

@maxbeutel Can you submit a PR on the documentation? Thanks.

@maxbeutel
Copy link
Author

@fabpot I added docs here symfony/symfony-docs#2459 thanks for merging and doing the changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants