Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[TwigBundle/Brige] catch missing requirements to throw meaningful exceptions #25601

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

Merged
merged 1 commit into from
Dec 30, 2017

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 25, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25256, symfony/webpack-encore#173
License MIT
Doc PR -

It is possible to register a handler for undefined twig functions/filters.
IMHO, this is the hook point we should leverage to throw meaningful exception messages.

This works well on its own. We now just need to list the functions/filters with appropriate messages.

There is one case we could enhance: at warmup time, Twig exceptions are swallowed, thus not visible.
Shouldn't we make these visible instead?

ping @weaverryan since this is related to two issues of yours.

@weaverryan
Copy link
Member

Thanks for this Nicolas. Obviously, this is a one-off solution, but it IS one of the most common situations. So... are you thinking w could merg this and start with this alone? Or do you intend for this to turn into something bigger?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Dec 26, 2017

Implementation now ready with the full list of Twig functions/filters handled by the bridge.
@weaverryan yes, I think this is enough to close #25256 - we don't want to create "autoconf for PHP" (because this is what it could look like :) )

Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

The form_enctype() function is missing. What about the filters from the CodeExtension?

'workflow_transitions' => 'workflow',
'workflow_has_marked_place' => 'workflow',
'workflow_marked_places' => 'workflow',
'yaml_encode' => 'yaml',
Copy link
Member

Choose a reason for hiding this comment

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

yaml_encode and yaml_dump are filters

'logout_url' => 'security-http',
'logout_path' => 'security-http',
'is_granted' => 'security-core',
'trans' => 'translation',
Copy link
Member

Choose a reason for hiding this comment

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

trans and transchoice are filters

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Dec 27, 2017

@xabbuh thanks, comments addressed.

form_enctype() function is missing

deprecated in 2.3, removed in 3.0

What about the filters from the CodeExtension?

http-kernel/foundation/routing are always installed with Flex, no need for special care

return false;
}

throw new SyntaxError(sprintf('Did you forget to run "composer require symfony/%s"? Unknown filter "%s".', $name, self::$filterComponents[$name]));
Copy link
Member Author

Choose a reason for hiding this comment

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

Unknown filter "%s".

this must be last in the message because the twig file/line is appended later on
([...] Unknown filter "%s" in foo.html.twig on line 123)

Copy link
Member

Choose a reason for hiding this comment

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

I would add a comment in the code directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

comment added in the code

@fabpot
Copy link
Member

fabpot commented Dec 30, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit ac891ac into symfony:3.4 Dec 30, 2017
fabpot added a commit that referenced this pull request Dec 30, 2017
…ningful exceptions (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[TwigBundle/Brige] catch missing requirements to throw meaningful exceptions

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25256, symfony/webpack-encore#173
| License       | MIT
| Doc PR        | -

It is possible to register a handler for undefined twig functions/filters.
IMHO, this is the hook point we should leverage to throw meaningful exception messages.

This works well on its own. We now just need to list the functions/filters with appropriate messages.

There is one case we could enhance: at warmup time, Twig exceptions are swallowed, thus not visible.
Shouldn't we make these visible instead?

ping @weaverryan since this is related to two issues of yours.

Commits
-------

ac891ac [TwigBundle/Brige] catch missing requirements to throw meaningful exceptions
@nicolas-grekas nicolas-grekas deleted the twig-suggests branch January 4, 2018 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.