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

Removed templating.helper.assets service configuration #14956

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

Closed
wants to merge 1 commit into from
Closed

Removed templating.helper.assets service configuration #14956

wants to merge 1 commit into from

Conversation

peterrehm
Copy link
Contributor

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

Relates to #14940

@dosten
Copy link
Contributor

dosten commented Jun 12, 2015

Why we need to delete the service? Do you know?

@peterrehm
Copy link
Contributor Author

There has been quit some refactoring due to the Asset Component, so there is a new service available, assets.packages which provides you that functionality.

@dosten
Copy link
Contributor

dosten commented Jun 12, 2015

Yes, but templating.helper.assets is a public templating helper, what happens if someone wants to use assets.packages in a PHP template? IMO we only need to remove the BC layer in the helper.

@peterrehm
Copy link
Contributor Author

Good point, this must be validated, I was just following that up based on the referenced PR. It would be great if someone could provide further feedback, I was just removing the configuration, the actual service has been removed alfready before.

@dosten
Copy link
Contributor

dosten commented Jun 12, 2015

The class has been removed in #13666, the PR doesn't have any discussion, maybe it's a mistake. cc/ @fabpot

@stof
Copy link
Member

stof commented Jun 12, 2015

hmm, looks like the there is templating helper based on the new component. This looks like a mistake.

@dosten
Copy link
Contributor

dosten commented Jun 13, 2015

@stof sorry, i don't understand what you're saying..

@peterrehm
Copy link
Contributor Author

What he means is that it seems to be wrong for him, that there is no AssetsHelper based on the new component.

@fabpot The current deprecation message relates just to the new Asset Component. The Asset component however has no such helper. Do you want to have a helper again in the asset component with the same functionality? What was the intention?

The decision should be reflected in the docs.

@dosten
Copy link
Contributor

dosten commented Jun 14, 2015

@peterrehm Thanks for the explanation :) IMO we should keep the helper, see #14972.

@peterrehm
Copy link
Contributor Author

@dosten So I would assume this PR should be fine, as this is deprecating the helper service not the Helper which will be eventually moved to another component which is however at the current moment already removed.

@peterrehm peterrehm closed this Nov 19, 2015
@peterrehm peterrehm deleted the remove-assets-helper branch November 26, 2015 08:50
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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.