Skip to content

[FrameworkBundle] remove unused property in DelegatingLoader #6298

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

Conversation

Tobion
Copy link
Contributor

@Tobion Tobion commented Dec 12, 2012

bc break: theoretically yes because its a protected property, but practically no as it's probably not used anywhere

@fabpot
Copy link
Member

fabpot commented Dec 12, 2012

Does it hurt? What if someone extended this? That would break BC for no obvious reasons. I think we can keep it the current way.

@Tobion
Copy link
Contributor Author

Tobion commented Dec 12, 2012

Well, according to the history the logger was never used in the class. So it was an error in the first place.
Otherwise we could inject the logger in every class because it could potentially be useful.

@fabpot
Copy link
Member

fabpot commented Dec 12, 2012

That's not what I'm saying. If someone uses it in a child class, this is a BC break.

@stof
Copy link
Member

stof commented Dec 12, 2012

and it is also a BC break if someone instantiate the class as it changes the signature of the constructor

@Tobion
Copy link
Contributor Author

Tobion commented Dec 12, 2012

Yes too bad this unused property made it into the core. Instead, a potential subclass should have overwritten the service and injected the logger himself if its needed.

@fabpot fabpot closed this Dec 12, 2012
@Tobion
Copy link
Contributor Author

Tobion commented Dec 13, 2012

How about deprecating this for 2.2? So we could remove it from 2.3.

@fabpot
Copy link
Member

fabpot commented Dec 13, 2012

Deprecating something leads to a BC break at some point. We already do too many BC breaks if you ask me, so let's really break BC when it really make sense.

@Tobion
Copy link
Contributor Author

Tobion commented Dec 13, 2012

ok

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