Skip to content

[FrameworkBundle][Template name] avoid error message for the shortcut n... #12894

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 8, 2014

Conversation

aitboudad
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets #12249, #12254
Tests pass? yes
License MIT

@fabpot
Copy link
Member

fabpot commented Dec 8, 2014

Thank you @aitboudad.

@fabpot fabpot merged commit 055129c into symfony:2.3 Dec 8, 2014
fabpot added a commit that referenced this pull request Dec 8, 2014
…he shortcut n... (aitboudad)

This PR was merged into the 2.3 branch.

Discussion
----------

[FrameworkBundle][Template name] avoid  error message for the shortcut n...

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Fixed tickets  | #12249,  #12254
| Tests pass?   | yes
| License       | MIT

Commits
-------

055129c [FrameworkBundle][Template name] avoid  error message for the shortcut notation.
@aitboudad aitboudad deleted the ticket_12249_23 branch December 8, 2014 14:55
@stof
Copy link
Member

stof commented Dec 23, 2014

@fabpot there is actually a BC break in this change: parent::parse() returns a TemplateReference from the component, not from the bundle, and calling $ref->get('bundle') on it throws an exception, because the component does not know about the bundle parameter (see the code of the TemplateReference for the exception being thrown).
I see 2 ways to fix this:

  • return null for other parameters rather than throwing an exception in TemplateReference (especially given that the outer code has no way to know which parameters are known other than catching this exception)
  • change the template name parser to replace the TemplateReference by a FrameworkBundle one

what would be your preferred way to fix this ?
the second option would be the closest to BC, while the first option would be the one making the API more dev-friendly

@stof
Copy link
Member

stof commented Dec 23, 2014

Note that AsseticBundle is affected by this break for instance (this is how I found it)

@fabpot
Copy link
Member

fabpot commented Dec 23, 2014

I think option 2 would be better.

@stof
Copy link
Member

stof commented Dec 29, 2014

Actually, the option 2 is not good, because some of the parent references cannot be represented with a FrameworkBundle reference (it forces the logical name to have template.format.engine while the component reference does not have any restriction).
So here are the 2 solutions I see:

  • the option 1 above: return null for unknown parameters
  • restrict the FrameworkBundle parser to parsing template.format.engine when it cannot parse it as the bundle:folder:template.format.engine notation instead of using the component implementation

Btw, the name parameter has a different meaning in the bundle and the component, which means that passing a bundle TemplateReference to the component PHP FilesystemLoader will break

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.

3 participants